Prefer the runfiles manifest over the runfiles directory (#68)
`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.
`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.
Future work (https://github.com/bazelbuild/bazel/pull/30674) will allow
the directory to be used again in both functions, assuming it's
materialized.
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 @@
# 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 @@
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 @@
[[ "$(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=
