Read and write the regular runfiles variables - #373
Draft
fmeum wants to merge 3 commits into
Draft
Conversation
…ables `RUNFILES_MANIFEST_ONLY` is derived from `--enable_runfiles`, an analysis-time flag, and thus doesn't account for the runfiles directory being materialized by the sandbox or by remote execution. When it is set but the runfiles directory has been materialized, no manifest has been staged and the library fails to initialize even though all runfiles are right there. Determine the implementation from the variables that name the two candidates instead, as all other runfiles libraries do: use the manifest if `RUNFILES_MANIFEST_FILE` names an existing file, otherwise use the runfiles directory if `RUNFILES_DIR` (or `JAVA_RUNFILES`) names an existing directory. Whoever sets up the environment already knows which of the two it staged and only names the one that is usable, so a value that names something that isn't there is dropped rather than trusted. Also pass the runfiles directory from the environment on to `getEnvVars` instead of always deriving it from the manifest path, which only works if the two are adjacent.
…y is used A subprocess that inherits `RUNFILES_MANIFEST_FILE` or `RUNFILES_MANIFEST_ONLY` from an ancestor process resolves its runfiles through a manifest that describes how that process' runfiles were staged, not how the current process' runfiles were, and thus disagrees with the runfiles directory passed to it in `RUNFILES_DIR`. Set both variables to the empty string, which every runfiles library treats as unset, so that the implementation this process selected is also the one its subprocesses select.
The launcher script exports `JAVA_RUNFILES`, but not `RUNFILES_DIR`, so a subprocess whose runfiles library only knows about the latter can't find the runfiles directory. It also exports `RUNFILES_MANIFEST_FILE` and `RUNFILES_MANIFEST_ONLY` based on `--enable_runfiles` alone, which tells subprocesses to use a manifest that hasn't been staged whenever the sandbox or remote execution materialized the runfiles directory instead. This makes a `java_binary` used as a tool fail with `--noenable_runfiles`, since even its own `rlocation` can't resolve the JVM launcher. Export `RUNFILES_DIR` alongside `JAVA_RUNFILES`, and only point subprocesses at the manifest if it has actually been staged. `rlocation` now resolves against the runfiles directory in that case, which is what it already did whenever runfiles were enabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Runfiles libraries decide between resolving runfiles paths against the runfiles directory and resolving them through the runfiles manifest. The Java library makes that decision based on
RUNFILES_MANIFEST_ONLY, which Bazel derives from--enable_runfilesalone. That flag is evaluated at analysis time and thus doesn't account for the runfiles directory being materialized by the sandbox or by remote execution, in which case no manifest is staged at all. Thejava_binarylauncher script has the same problem, one level out: it bakes the value of the flag into the script and points subprocesses at a manifest that may not be there.This makes the library read and write the variables that name the runfiles directory and the runfiles manifest, and select the implementation based on which of the two actually exists — the way every other runfiles library does it. Whoever sets up the environment already knows which of the two it staged, and communicates that by naming only the one that is usable, so a value that names something that isn't there is dropped rather than trusted.
Concretely, with
--noenable_runfiles, ajava_binaryused as a tool of a sandboxed action fails to run today:Both the launcher script and the runfiles library insist on a manifest, while the sandbox has materialized the entire runfiles directory. After this change the same target builds, and the binary resolves its runfiles against the directory.
RUNFILES_MANIFEST_ONLYis still written bygetEnvVarsand by the launcher script whenever the manifest is the implementation in use, for the benefit of subprocesses whose runfiles library still reads it. It is never propagated from the environment, since an inherited value describes how some ancestor process' runfiles were staged.The commits are independent and can be taken separately:
Select the runfiles implementation based on the regular runfiles variables— the read side of the library.Clear the manifest variables in getEnvVars if the runfiles directory is used— the write side of the library.Export the regular runfiles variables from the java_binary launcher— the launcher script, which is what fixes the failure above end to end.Follow-up, out of scope here: bazelbuild/bazel is adding a
_runfiles_enabledmarker file to every runfiles tree, which is present if and only if the directory has been fully materialized. With it, the selection can prefer a materialized runfiles directory over an available manifest instead of the other way around, which avoids parsing a manifest that isn't needed. That is a one-line addition on top of this change and needs a Bazel release that stages the marker.Tested manually on macOS with
bazel run,bazel run --noenable_runfiles,bazel buildof a genrule whose tool is ajava_binary(sandboxed, with and without--noenable_runfiles), plus the new unit tests.//test/...passes.