bazel-contrib / bazel-lib

Common useful functions for writing BUILD files and Starlark macros/rules
Apache License 2.0
131 stars 87 forks source link

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

Open alexeagle opened 11 months ago

alexeagle commented 11 months ago

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 commented 11 months ago

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

alexeagle commented 11 months ago

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 commented 11 months ago

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

alexeagle commented 10 months ago

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

liningpan commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

manuelnaranjo commented 3 months ago

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

manuelnaranjo commented 3 months ago

I quickly hacked this https://github.com/bookingcom/bazel-lib/commit/HEAD, I'm going to test it in our use case, what you think? Does it look like something we could merge?

alexeagle commented 3 months ago

Ooh thanks! @thesayyn could you TAL?

manuelnaranjo commented 3 months ago

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

thesayyn commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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