From cf374f0e5d9a3f722e81cb42bb2e0d6f5927a093 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 11 Aug 2026 17:00:33 +0000 Subject: [PATCH] Prefer the runfiles manifest over the runfiles directory `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. --- shell/runfiles/runfiles.bash | 16 ++++-- tests/runfiles/runfiles_test.bash | 91 +++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 6 deletions(-) diff --git a/shell/runfiles/runfiles.bash b/shell/runfiles/runfiles.bash index 3dfe3e2..b6bbdfe 100644 --- a/shell/runfiles/runfiles.bash +++ b/shell/runfiles/runfiles.bash @@ -374,12 +374,11 @@ function runfiles_rlocation_checked() { # FIXME: If the runfiles lookup fails, the exit code of this function is 0 if # and only if the runfiles manifest exists. In particular, the exit code # behavior is not consistent across platforms. - if [[ -e "${RUNFILES_DIR:-/dev/null}/$1" ]]; then - if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then - echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR ($RUNFILES_DIR), return" - fi - echo "${RUNFILES_DIR}/$1" - elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + # The manifest takes precedence over the runfiles directory: whether the directory is populated + # is a property of the execution of the action or test, which is not known at analysis time, so + # the directory may exist but contain the stale contents of a previous execution. If the manifest + # exists, it is always authoritative. + if [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: rlocation($1): looking in RUNFILES_MANIFEST_FILE ($RUNFILES_MANIFEST_FILE)" fi @@ -483,6 +482,11 @@ function runfiles_rlocation_checked() { echo "" fi fi + elif [[ -e "${RUNFILES_DIR:-/dev/null}/$1" ]]; then + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR ($RUNFILES_DIR), return" + fi + echo "${RUNFILES_DIR}/$1" else if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "ERROR[runfiles.bash]: cannot look up runfile \"$1\" " \ diff --git a/tests/runfiles/runfiles_test.bash b/tests/runfiles/runfiles_test.bash index f738b3d..fd60eee 100755 --- a/tests/runfiles/runfiles_test.bash +++ b/tests/runfiles/runfiles_test.bash @@ -512,6 +512,97 @@ EOF [[ "$(rlocation "repo2+/foo/runfile" "my_module++ext+repo1" || echo failed)" == "$tmpdir/repo2+/runfile" ]] || fail } +# Writes a script that reports the repository it lies in via the given function. +# The function must not be defined by more than one script in a single test, as +# runfiles_current_repository resolves the path of the script that defines it. +function write_current_repository_lib() { + local -r path="$1" + local -r func="$2" + local -r result="${3:-}" + mkdir -p "$(dirname "$path")" + if [[ -n "$result" ]]; then + # A stale copy that must never be sourced. + cat > "$path" << EOF +function $func() { + echo "$result" +} +EOF + else + cat > "$path" << EOF +function $func() { + runfiles_current_repository 1 +} +EOF + fi +} + +function test_current_repository_directory_based() { + local tmpdir="$(mktemp -d $TEST_TMPDIR/tmp.XXXXXXXX)" + + export RUNFILES_DIR="${tmpdir}/mock/runfiles" + export RUNFILES_MANIFEST_FILE= + write_current_repository_lib "$RUNFILES_DIR/protobuf+3.19.2/foo/lib.sh" repo_of_other + write_current_repository_lib "$RUNFILES_DIR/_main/bar/lib.sh" repo_of_main + source "$runfiles_lib_path" + + source "$(rlocation "protobuf+3.19.2/foo/lib.sh" "")" || fail + [[ "$(repo_of_other || echo failed)" == "protobuf+3.19.2" ]] || fail + + source "$(rlocation "_main/bar/lib.sh" "")" || fail + [[ "$(repo_of_main || echo failed)" == "" ]] || fail +} + +function test_current_repository_manifest_based() { + local tmpdir="$(mktemp -d $TEST_TMPDIR/tmp.XXXXXXXX)" + + export RUNFILES_DIR= + export RUNFILES_MANIFEST_FILE="$tmpdir/foo.runfiles_manifest" + write_current_repository_lib "$tmpdir/protobuf+3.19.2/foo/lib.sh" repo_of_other + write_current_repository_lib "$tmpdir/_main/bar/lib.sh" repo_of_main + cat > "$RUNFILES_MANIFEST_FILE" << EOF +protobuf+3.19.2/foo/lib.sh $tmpdir/protobuf+3.19.2/foo/lib.sh +_main/bar/lib.sh $tmpdir/_main/bar/lib.sh +EOF + source "$runfiles_lib_path" + + source "$(rlocation "protobuf+3.19.2/foo/lib.sh" "")" || fail + [[ "$(repo_of_other || echo failed)" == "protobuf+3.19.2" ]] || fail + + source "$(rlocation "_main/bar/lib.sh" "")" || fail + [[ "$(repo_of_main || echo failed)" == "" ]] || fail +} + +# Both envvars are set at the same time e.g. on Windows with --enable_runfiles, +# where the runfiles directory contains the MANIFEST file that +# runfiles_export_envvars promotes to RUNFILES_MANIFEST_FILE. The manifest maps +# rlocation paths to the locations of the *original* files, so a caller that had +# been looked up in the runfiles directory could never be found in it. +function test_current_repository_directory_and_manifest_based() { + local tmpdir="$(mktemp -d $TEST_TMPDIR/tmp.XXXXXXXX)" + + export RUNFILES_DIR="${tmpdir}/mock/runfiles" + export RUNFILES_MANIFEST_FILE="$RUNFILES_DIR/MANIFEST" + write_current_repository_lib "$tmpdir/protobuf+3.19.2/foo/lib.sh" repo_of_other + write_current_repository_lib "$tmpdir/_main/bar/lib.sh" repo_of_main + # The runfiles directory may hold stale contents, so the manifest wins. + mkdir -p "$RUNFILES_DIR" + write_current_repository_lib "$RUNFILES_DIR/protobuf+3.19.2/foo/lib.sh" repo_of_other stale + write_current_repository_lib "$RUNFILES_DIR/_main/bar/lib.sh" repo_of_main stale + cat > "$RUNFILES_MANIFEST_FILE" << EOF +protobuf+3.19.2/foo/lib.sh $tmpdir/protobuf+3.19.2/foo/lib.sh +_main/bar/lib.sh $tmpdir/_main/bar/lib.sh +EOF + source "$runfiles_lib_path" + + [[ "$(rlocation "protobuf+3.19.2/foo/lib.sh" "" || echo failed)" == "$tmpdir/protobuf+3.19.2/foo/lib.sh" ]] || fail + source "$(rlocation "protobuf+3.19.2/foo/lib.sh" "")" || fail + [[ "$(repo_of_other || echo failed)" == "protobuf+3.19.2" ]] || fail + + [[ "$(rlocation "_main/bar/lib.sh" "" || echo failed)" == "$tmpdir/_main/bar/lib.sh" ]] || fail + source "$(rlocation "_main/bar/lib.sh" "")" || fail + [[ "$(repo_of_main || echo failed)" == "" ]] || fail +} + function test_directory_based_envvars() { export RUNFILES_DIR=mock/runfiles export RUNFILES_MANIFEST_FILE=