Create and stage symlink artifacts with unmodified target path

[fmeum]

Unresolved symlink artifacts created with ctx.actions.symlink(target_path = ...) are now created without making the target path absolute by prepending the exec root, which diverged from the documentation and intended goal and also gave rise to hermeticity issues as such symlinks would regularly resolve outside the sandbox.

Furthermore, in order to bring local execution in line with other execution types, the runfiles manifest entry (and thus the runfiles directory contents) for symlink artifacts are now their target paths rather than their exec paths, which along the way resolves another soure of non-hermetic resolution outside the runfiles directory.

This requires making symlink artifacts (but not artifacts representing regular files) inputs of SourceManifestAction, which previously had no inputs. The change flattens a nested set in getInputs, but benchmarks confirm that this does not result in a performance regression. Alternatives considered either result in a substantially worse Starlark API or wouldn't work for symlinks created by spawns.

Work towards #10298

[fmeum]

I can also make this throw an Exception if you prefer.

[fmeum]

Dropped it for now as per fmeum@a2fdd4a#r83921660.

[fmeum]

@lberki @alexjski

[lberki]

From my POV, this looks good; @alexjski , mind taking a look whether you can live with the way getInputs() is implemented now? If so, let's merge this and call this work well done.

[gregmagolan]

Tested this change against rules_js here aspect-build/rules_js#453 with a release from our bazel fork https://github.com/aspect-build/bazel/releases/tag/6.0.0-pre_20220823_1_aspect. Working as expected. Great work @fmeum!

[fmeum]

From my POV, this looks good; @alexjski , mind taking a look whether you can live with the way getInputs() is implemented now? If so, let's merge this and call this work well done.

Based on fmeum@a2fdd4a#r84014073, I would leave this PR as is then, tight?

[lberki]

Yeah, let's merge this as it is then.

[coeuvre]

cc @tjgq as this is relevant to our discussion about metadata handling for symlinks.

[larsrc-google]

Going to give this a test against some internal benchmarks.

[larsrc-google]

This seems to allow creating symlinks to anywhere, including outside of the Bazel tree. Do we really want that? Are there use cases that require that?

[fmeum]

This seems to allow creating symlinks to anywhere, including outside of the Bazel tree. Do we really want that? Are there use cases that require that?

This has been possible even before this commit and in fact happened more easily by accident since the symlink action created absolute symlinks that pointed outside the sandbox.

Generally speaking, I would expect most symlinks to be relative and stay within the tree. I do see use cases for absolute symlinks (e.g. LLVM ships symlinks to /usr/bin/ld).

[lberki]

Sponsor of this change here! Yes, this allows creating symlinks to anywhere, but that, as @fmeum says, is not a new behavior; any action was able to do that before this change.

The reason why this is not a problem is that an "unresolved symlink" means that actions should only do readlink() and lstat() (but not open() or stat()) on the artifact. It's intended for things like creating file system images in the output tree, where it's perfectly valid to have a symlink usr/bin/bazel -> /usr/bin/bazel-real because the intent is not to chase the symlink, but to eventually package it up.

Of course, if an action breaks this contract, it's a problem. That's the same kind of issue as reading a file that's not a declared input of the action: not allowed by contract and if there is a sandbox, it may prevent doing that mistake.

[larsrc-google]

Ok, thank you. I have gained further understanding now.