Prefer the runfiles manifest over the runfiles directory - #68
Merged
Wyverald merged 1 commit intoAug 12, 2026
Merged
Conversation
`runfiles_rlocation_checked` looked up a path in the runfiles directory before consulting the manifest. Whether the runfiles directory is populated is a property of how the action or test is executed and is not known at analysis time, so the directory may exist while holding the stale contents of a previous execution. Consult the manifest first and only fall back to the runfiles directory if no manifest exists. This also fixes `runfiles_current_repository` on Windows with `--enable_runfiles`. That mode is the only one in which both `RUNFILES_DIR` and `RUNFILES_MANIFEST_FILE` are set: Windows does not sandbox, so the runfiles tree used at runtime contains the `MANIFEST` file that `runfiles_export_envvars` promotes to `RUNFILES_MANIFEST_FILE`. `rlocation` then returned a path inside the runfiles tree, but the manifest maps rlocation paths to the locations of the original files, so the caller could never be the target of a manifest entry and `runfiles_current_repository` reported every caller as belonging to the main repository. With the manifest taking precedence, callers are manifest targets again and the lookup succeeds. Note that runfiles that are empty files are recorded in the manifest with an empty target, so they no longer resolve when a manifest is present. This matches the existing behavior with `--noenable_runfiles`. `runfiles_current_repository` had no test coverage at all, which is why this went unnoticed. Cover all three combinations of the two envvars: directory only, manifest only, and both set at the same time. The last case also asserts that a stale copy in the runfiles directory is never preferred over the manifest.
Wyverald
approved these changes
Aug 12, 2026
This was referenced Aug 13, 2026
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, and the library records it once on load, next to the other `_RLOCATION_` values it derives from the environment. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess - which has to trust the value it is given - to resolve runfiles paths that don't exist. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, and the library records it once on load, next to the other `_RLOCATION_` values it derives from the environment. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess - which has to trust the value it is given - to resolve runfiles paths that don't exist. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, so the library records it once on load rather than repeating it per lookup. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess to resolve runfiles paths that don't exist. With both exported variables usable on their own, a subprocess needs no knowledge of which one this process picked, so that choice stays private to it. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, so the library records it once on load rather than repeating it per lookup. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess to resolve runfiles paths that don't exist. With both exported variables usable on their own, a subprocess needs no knowledge of which one this process picked, so that choice stays private to it. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, so the library records it once on load rather than repeating it per lookup. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess to resolve runfiles paths that don't exist. With both exported variables usable on their own, a subprocess needs no knowledge of which one this process picked, so that choice stays private to it. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, so the library records it once on load rather than repeating it per lookup. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess to resolve runfiles paths that don't exist. With both exported variables usable on their own, a subprocess needs no knowledge of which one this process picked, so that choice stays private to it. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, so the library records it once on load rather than repeating it per lookup. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess to resolve runfiles paths that don't exist. With both exported variables usable on their own, a subprocess needs no knowledge of which one this process picked, so that choice stays private to it. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, so the library records it once on load rather than repeating it per lookup. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess to resolve runfiles paths that don't exist. With both exported variables usable on their own, a subprocess needs no knowledge of which one this process picked, so that choice stays private to it. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, so the library records it once on load rather than repeating it per lookup. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess to resolve runfiles paths that don't exist. With both exported variables usable on their own, a subprocess needs no knowledge of which one this process picked, so that choice stays private to it. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98
fmeum
added a commit
to fmeum/rules_shell
that referenced
this pull request
Aug 13, 2026
`runfiles_rlocation_checked` and `runfiles_current_repository` have to agree on where they look up runfiles: the output of the former is the input of the latter, and the manifest maps rlocation paths to the locations of the *original* files, so a path resolved against the runfiles directory is never the target of a manifest entry. bazel-contrib#68 made them agree on the manifest, at the cost of scanning it on every lookup and of returning paths outside the runfiles tree. Instead, have both use the runfiles directory whenever it has been materialized and the manifest otherwise, without ever falling back from one to the other. Whether the directory has been materialized is a property of how the action or test is executed rather than of the build: `--noenable_runfiles` leaves behind a directory that contains only the `MANIFEST` file and the subdirectory of the main repository, but the sandbox and remote execution materialize it in full regardless (they don't stage the manifest at all). The presence of this library in the directory distinguishes the two cases: it is an ordinary runfile, so it is materialized by exactly the mechanisms that also materialize every other runfile, and it is in the runfiles of every user of the library, which all load it via the initialization snippet that looks it up at this very path. The snippet therefore already made this determination when it sourced the library, so the library records it once on load rather than repeating it per lookup. `runfiles_export_envvars` in turn no longer passes on a runfiles directory it hasn't verified. It used to export any `RUNFILES_DIR` it could derive from the manifest path as long as that directory existed, which with `--noenable_runfiles` is the near-empty one, leaving a subprocess to resolve runfiles paths that don't exist. With both exported variables usable on their own, a subprocess needs no knowledge of which one this process picked, so that choice stays private to it. `runfiles_current_repository` now consistently returns 0 when it has to fall back to parsing the caller's execroot path; it used to do so only if the runfiles directory was used.
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
2 participants
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_rlocation_checkedlooked up a path in the runfiles directory before consulting the manifest. Whether the runfiles directory is populated is a property of how the action or test is executed and is not known at analysis time, so the directory may exist while holding the stale contents of a previous execution. Consult the manifest first and only fall back to the runfiles directory if no manifest exists.This also fixes
runfiles_current_repositoryon Windows with--enable_runfiles. That mode is the only one in which bothRUNFILES_DIRandRUNFILES_MANIFEST_FILEare set: Windows does not sandbox, so the runfiles tree used at runtime contains theMANIFESTfile thatrunfiles_export_envvarspromotes toRUNFILES_MANIFEST_FILE.rlocationthen returned a path inside the runfiles tree, but the manifest maps rlocation paths to the locations of the original files, so the caller could never be the target of a manifest entry andrunfiles_current_repositoryreported every caller as belonging to the main repository. With the manifest taking precedence, callers are manifest targets again and the lookup succeeds.runfiles_current_repositoryhad no test coverage at all, which is why this went unnoticed. Cover all three combinations of the two envvars: directory only, manifest only, and both set at the same time. The last case also asserts that a stale copy in the runfiles directory is never preferred over the manifest.Future work (bazelbuild/bazel#30674) will allow the directory to be used again in both functions, assuming it's materialized.