[FR]: tar rule should support runfiles with symlinks

[alexeagle]

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]

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

[alexeagle]

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]

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

[alexeagle]

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]

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]

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]

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

[manuelnaranjo]

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

[manuelnaranjo]

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

[alexeagle]

Ooh thanks! @thesayyn could you TAL?

[manuelnaranjo]

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]

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]

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]

@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]

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

[thesayyn]

I have a WIP here; bazel-contrib/bazel-lib#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]

@thesayyn, I continued working on bazel-contrib/bazel-lib#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]

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

[ewianda]

I ended with this implementation, but I am not sure if this is a reasonable direction to take.
bazel-contrib/bazel-lib#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.

[thesayyn]

We support symlinks now but I’ll keep this issue open until we solve the awk toolchain problem.

[Silic0nS0ldier]

I've observed a difference between Bazel's own runfile links and those created by the tar rule.

For a symlink at //web/node_modules/tsconfig-paths, the link is;

• tar rule, bazel-out/k8-fastbuild/bin/node_modules/.aspect_rules_js/[email protected]/node_modules/tsconfig-paths
• bazel, ../../node_modules/.aspect_rules_js/[email protected]/node_modules/tsconfig-paths

This is on the latest version (2.14.0).

[Silic0nS0ldier]

readlink -f being preferred appears to be part of the issue (it is resolving ../../node_modules/... to an absolute path which is then striped down so it starts with bin_dir).

The bigger issue is distinguishing between symlinks that are implementation details (e.g. in the bazel sandbox, source files linked into execroot). Ideally Bazel would use a hardlink, copy-on-write, or just copy so that any symlinks present are easily processed (just readlink). Second best would be getting a runfiles manifest so the implementation detail symlinks can be skipped over (map file in manifest and then readlink the target).

Addressing this issue may result in behaviour changes. For example, currently the test case //lib/tests/tar:test_17_after_processing has the following;

lib/tests/tar/native_binary_bin -> ../native_binary_bin.runfiles/_main/lib/tests/tar/executable.sh

Runfile links are symlinks to the files they are mapping. In the example above, the relation is the wrong way around.

Runfile links also use absolute paths, so for portability runfile links would need to be made relative.

EDIT: _.runfiles_manifest can be retrieved via [[Target]].files_to_run.runfiles_manifest. Repo mapping manifest is also available.

[aignas]

I was trying the readlink based solution and the performance for py_binary from rules_python was not great.

The workaround that I went with in the end was to just fudge the tar manifest: bazel-contrib/rules_python#2489 (comment)

The native.genrule snippet is:

# This is part of the macro
native.genrule(
    name = name + ".fix",
    srcs = [original_spec],
    outs = [name + ".fix"],
    cmd = "sed {} $< >$@".format(
        " ".join([
            # This is to ensure that the symlink to Python is preserved when building the
            # image. The magic number 3 is deduced by carefully observation of the errors.
            "-e 's|type=file.*.venv/bin/python3|type=link link={}/$(PYTHON3)|g'".format(
                "/".join([".."] * (3 + len(native.package_name().split("/")))),
            ),
            # It seems that the `$(PYTHON3)` had the `external` prefix and my runfiles did
            # not. So slap an extra adjustment of the output.
            "-e 's|../external/|../|g'",
        ]),
    ),
    toolchains = [
        # This is to ensure that the $(PYTHON3) variable can be expanded in the genrule
        "@rules_python//python:current_py_toolchain",
    ],
)

The issue boils down to the fact that I need to specify the list of files to scan with readlink to see if they are symlinks. For rules_python py_binary target this does not scale well because if I pass the py_binary itself, that is creating a single symlink, it will also scan all of the included source files (which could be many).

Am I missing something here?

[thesayyn]

The issue boils down to the fact that I need to specify the list of files to scan with readlink to see if they are symlinks. For rules_python py_binary target this does not scale well because if I pass the py_binary itself, that is creating a single symlink, it will also scan all of the included source files (which could be many).

For this reason js_image_layer introduced a preserve_symlinks attribute that decides which paths to check symlinks for.

Something similar can be done here as well. Ideal solution would be though Bazel telling us that ctx.actions.declare_file is actually a symlink. eg (bazel 8 file.is_symlink).

Speaking of which, bazel 8 file.is_symlink could be used as a nice hint to narrow down the number of stat calls we make.

I don't have time to implement this myself, but i would be happy to review a PR fixing this problem.

readlink -f being preferred appears to be part of the issue (it is resolving ../../node_modules/... to an absolute path which is then striped down so it starts with bin_dir).

bazel-contrib/bazel-lib#1052 (comment)

https://github.com/bazel-contrib/bazel-lib/pull/1052/files could be what fixes this.

[ewianda]

I can take a stab at this. But as @aignas mentioned, it is incredibly slow to a point where I had switch to something similar to @aignas implementation.

[thesayyn]

We could make it be the same way js_image_layer where preserve_symlinks takes a regex and matches that against paths that needs to preserve symlinks. there is room for improvement, still but its a good start. PRs are highly welcome, i'll personally review.