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

Do not force unresolved symlinks to be absolute #15982

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jul 26, 2022

The documentation of ctx.actions.symlink(target_path = ...) states
that the given path is used as the symlink target without any changes,
but in reality this path has so far always been converted into an
absolute path by prepending the exec root. This is changed by this
commit, which gets very close to the documented behavior by preserving
the target path as is except for the standard normalization applied by
PathFragment. Improving the situation even further would require
modifying or adding to Bazel's core file system API, which may be done
in a follow-up PR.

Since relative symlinks only resolve correctly at the location they were
originally created, they have to be handled specially when staged as
runfiles. This commit adds a new declared_symlinks parameter to the
rule context's runfiles method that takes in symlink artifacts
declared via ctx.actions.declare_symlink. These symlinks are staged at
their runfiles path directly, with no further processing of their target
and without any intermediate runfiles pointing back to the artifact's
location under the exec root. This has to main benefits:

  • With local execution, symlinks are resolved within the runfiles tree,
    which is more hermetic than following the runfiles symlink back into
    the exec root and resolving the symlink artifact there.
  • Actions can expect symlink artifacts to be stages as is without
    intermediate symlinks with local and sandboxed execution, both inside
    and outside the runfiles tree. This is important for packaging actions
    as well as rulesets sensitive to symlinks (e.g. rules_js).

As a side-effect of the switch to relative symlinks, this commit
resolves a non-hermeticity issue observed in
#10298 (comment)
Integration tests are added to verify that symlinks staged in the
sandbox are no longer resolved non-hermetically.

Fixes #14224

@fmeum
Copy link
Collaborator Author

fmeum commented Jul 26, 2022

@gregmagolan Could you give this a try? It implements #15963 (comment), but with a catch: ctx.runfiles(files = ...) no longer accepts symlinks, they have to be passed into the new declared_symlinks parameter. This is because the action creating the runfiles directory needs to read the symlinks and thus has to declare them as an input, which requires singling them out from files. For the first iteration at least, I decided to make this distinction explicit - this makes it clear that adding symlinks via transitive_files is not supported. You will thus have to make some changes to your branch of rules_js to be able to use this PR.

@fmeum fmeum force-pushed the direct-symlink-staging branch 2 times, most recently from ab1ca26 to 028a59a Compare July 26, 2022 16:29
@gregmagolan
Copy link
Contributor

For sure. I'll try it out this morning.

The documentation of `ctx.actions.symlink(target_path = ...)` states
that the given path is used as the symlink target without any changes,
but in reality this path has so far always been converted into an
absolute path by prepending the exec root. This is changed by this
commit, which gets very close to the documented behavior by preserving
the target path as is except for the standard normalization applied by
`PathFragment`. Improving the situation even further would require
modifying or adding to Bazel's core file system API, which may be done
in a follow-up PR.

Since relative symlinks only resolve correctly at the location they were
originally created, they have to be handled specially when staged as
runfiles. This commit adds a new `declared_symlinks` parameter to the
rule context's `runfiles` method that takes in symlink artifacts
declared via `ctx.actions.declare_symlink`. These symlinks are staged at
their runfiles path directly, with no further processing of their target
and without any intermediate runfiles pointing back to the artifact's
location under the exec root. This has to main benefits:
* With local execution, symlinks are resolved within the runfiles tree,
  which is more hermetic than following the runfiles symlink back into
  the exec root and resolving the symlink artifact there.
* Actions can expect symlink artifacts to be stages as is without
  intermediate symlinks with local and sandboxed execution, both inside
  and outside the runfiles tree. This is important for packaging actions
  as well as rulesets sensitive to symlinks (e.g. rules_js).

As a side-effect of the switch to relative symlinks, this commit
resolves a non-hermeticity issue observed in
bazelbuild#10298 (comment)
Integration tests are added to verify that symlinks staged in the
sandbox are no longer resolved non-hermetically.

Fixes bazelbuild#14224
@fmeum fmeum force-pushed the direct-symlink-staging branch from 028a59a to f6bef33 Compare July 26, 2022 16:44
@gregmagolan
Copy link
Contributor

gregmagolan commented Jul 26, 2022

Hmmm. The distinction is going to be problematic I think as it will leak out beyond the point where declare_symlink is called and into other rules that will not know which input is a symlink and which is a file. When downstream rules create runfiles they'll hit errors such as,

ERROR: /Users/greg/aspect/rules/js/rules_js/examples/webpack_cli/BUILD.bazel:37:16: in _copy_to_bin rule //examples/webpack_cli:bundle_copy_srcs_to_bin: 
Traceback (most recent call last):
	File "/private/var/tmp/_bazel_greg/7aed427a991d86f99332ab79b9b11780/external/aspect_bazel_lib/lib/private/copy_to_bin.bzl", line 117, column 32, in _impl
		runfiles = ctx.runfiles(files = files),
Error in runfiles: declared symlink 'File:[[<execution_root>]bazel-out/darwin-fastbuild/bin]node_modules/.aspect_rules_js/mathjs@10.6.1/node_modules/@babel/runtime' is invalid for files, add via declared_symlinks instead
ERROR: /Users/greg/aspect/rules/js/rules_js/examples/webpack_cli/BUILD.bazel:37:16: Analysis of target '//examples/webpack_cli:bundle_copy_srcs_to_bin' failed

Can the differentiate be done implicitly in the runfiles action by checking which is a symlink and which isn't? Perhaps an is_symlink function added to File would help and be useful in this case.

@gregmagolan
Copy link
Contributor

I have the changes to rules_js to go with this branch in a Draft PR here aspect-build/rules_js#313 if you wanted to try the out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ctx.actions.symlink doesn't make relative symlink, contrary to docs
2 participants