Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FR]: tar rule should support runfiles with symlinks #603

Open
alexeagle opened this issue Oct 9, 2023 · 19 comments
Open

[FR]: tar rule should support runfiles with symlinks #603

alexeagle opened this issue Oct 9, 2023 · 19 comments
Assignees
Labels
enhancement New feature or request untriaged Requires traige
Milestone

Comments

@alexeagle
Copy link
Collaborator

What is the current behavior?

We had to do some stuff to make js_image_layer work with rules_pkg.

Describe the feature

Should have feature parity with those, and be able to make clean bazelbuild/examples directories showing container builds for JS, Python, Java, etc with no userland starlark code.

@alexeagle alexeagle added the enhancement New feature or request label Oct 9, 2023
@alexeagle alexeagle added this to the 2.0 milestone Oct 9, 2023
@alexeagle alexeagle self-assigned this Oct 9, 2023
@github-actions github-actions bot added the untriaged Requires traige label Oct 9, 2023
@alexeagle
Copy link
Collaborator Author

I think it's okay for this to slip to 2.1 as it's not a breaking change.

@alexeagle alexeagle modified the milestone: 2.0 Oct 10, 2023
@alexeagle
Copy link
Collaborator Author

I'm on the fence about shipping 2.0 final so long as you can't replace rules_pkg in our js_image_layer example. @thesayyn WDYT?

@thesayyn
Copy link
Collaborator

we can land that after 2.0. it's non-breaking change.

@alexeagle
Copy link
Collaborator Author

IIUC, We'll probably want to wait for bazelbuild/bazel#19944 to be available in Bazel 7.0 and direct users to upgrade so that we can handle the cases correctly under bzlmod.

@liningpan
Copy link

Hi! Is there a plan for implementing this? In the absence of this feature, is the recommended way to use a custom binary like js_image_layer?

@thesayyn
Copy link
Collaborator

If you don't care about symlinks tar rule should work for you already, js_image_layer is special as symlinks needs to be preserved in the resulting layers.

@liningpan
Copy link

IIUC, the tar rule basically dereferences symlinks, which causes a lot of duplication in the resulting archive.

@manuelnaranjo
Copy link

It sounds like https://github.com/aspect-build/bazel-lib/blob/main/lib/private/tar.bzl#L227 is missing a bit for symlinks

@manuelnaranjo
Copy link

manuelnaranjo commented Jun 18, 2024

I quickly hacked this bookingcom@HEAD, I'm going to test it in our use case, what you think? Does it look like something we could merge?

@alexeagle
Copy link
Collaborator Author

Ooh thanks! @thesayyn could you TAL?

@manuelnaranjo
Copy link

FYI I tested it for our case and made a few improvements:

  • I made the symlinks relative so that if I inspect the .tar the links make sense
  • Symlinks can't have path being absolute
  • For type=link we need link=target
    BTW I love this approach, it makes way more sense to express our container images with rules_oci and mtree manipulation than what the inspect and filter layer with rules_docker. At least now I can easily explain my peers what it does

@thesayyn
Copy link
Collaborator

I quickly hacked this bookingcom@HEAD, I'm going to test it in our use case, what you think? Does it look like something we could merge?

Ah unfortunately, this does not work if something is reported as symlink but not added to the runfiles.symlinks. We need to figure out symlinks are runtime as a post process action, probably using awk to make some system calls to mutate mtree to reflect the symlinks which can't detected at analysis time.

This is why we need a js_image_layer rule that has its own tar generator. https://github.com/aspect-build/rules_js/blob/98081d0fb66b3619f5a191ea29765012849d13f7/js/private/image/index.ts#L84-L104

@thesayyn
Copy link
Collaborator

awk has a system("readlink") function which can used to do a similar thing, i am not sure how easy it would be though, but technically possible.

@manuelnaranjo
Copy link

@thesayyn I see, so we would need something like a genrule or some kind of action that can look into the generated manifest correct? I need to verify if the runfiles_manifest contains our symlinks at all. Maybe we need a small program that can do the thing, is there a policy against that kind of thing in this repo? I'm thinking a small go or python program that can compute it

@alexeagle
Copy link
Collaborator Author

We have a bunch of go programs in this repo already. That's an option if there isn't a simpler solution.

@thesayyn
Copy link
Collaborator

I have a WIP here; #967 which i will try to look into more coming days. awk has a system function which permits call to external programs such as readlink. I don't think we need a Go as this can be done with awk.

@ewianda
Copy link

ewianda commented Jan 10, 2025

@thesayyn, I continued working on #967 because of the related issue in bazelbuild/rules_python#2489. However, I’ve run into a problem: the genrule doesn’t have access to the binary’s runfiles, so using system(readlink) isn’t feasible. Do you have any suggestions or alternative approaches for handling this?

@thesayyn
Copy link
Collaborator

Hmm, not really, we have to include binary in srcs to be able to resolve symlinks.

@ewianda
Copy link

ewianda commented Jan 20, 2025

I ended with this implementation, but I am not sure if this is a reasonable direction to take.
#1036

This could be potentially implemented using awk but it felt very convoluted, particularly referencing the symlink without processing the entire mtree file.

If this approach makes sense, I can continue to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request untriaged Requires traige
Projects
None yet
Development

No branches or pull requests

5 participants