From 5adb6de7d99b31c73e48075380d31c3c7bbe5e1d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 13 Aug 2026 12:47:24 +0200 Subject: [PATCH] Look up runfiles in the runfiles directory if it is materialized `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. #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. --- shell/runfiles/runfiles.bash | 119 ++++++++++++++++-------------- tests/runfiles/runfiles_test.bash | 118 +++++++++++++++++++++++++++-- 2 files changed, 175 insertions(+), 62 deletions(-) diff --git a/shell/runfiles/runfiles.bash b/shell/runfiles/runfiles.bash index b6bbdfe..efb9276 100644 --- a/shell/runfiles/runfiles.bash +++ b/shell/runfiles/runfiles.bash @@ -87,16 +87,36 @@ # You can skip steps 1 and 2 when setting "use_bash_launcher" attribute in sh_binary or sh_test. # +# The runfiles path of this library, which is also where the initialization snippet sources it from. +# Exported since it is read by the exported functions below. +export _RLOCATION_RUNFILES_LIB_PATH=bazel_tools/tools/bash/runfiles/runfiles.bash + if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then if [[ -f "$0.runfiles_manifest" ]]; then export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" elif [[ -f "$0.runfiles/MANIFEST" ]]; then export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" - elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + elif [[ -f "$0.runfiles/$_RLOCATION_RUNFILES_LIB_PATH" ]]; then export RUNFILES_DIR="$0.runfiles" fi fi +# _RLOCATION_USE_RUNFILES_DIR is set to 1 if the runfiles directory can be used to look up runfiles +# and to the empty string otherwise. It is exported for subprocesses that use the functions below +# without sourcing this library, but never read back from the environment: it is always determined +# here, so that it cannot contradict the RUNFILES_DIR it belongs to. +# +# Whether a runfiles directory is populated is a property of how the action or test is executed +# rather than of the build: with --noenable_runfiles, Bazel 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. The presence of this library in the directory +# distinguishes the two cases as it is just an ordinary runfile. +if [[ -f "${RUNFILES_DIR:-/dev/null}/$_RLOCATION_RUNFILES_LIB_PATH" ]]; then + export _RLOCATION_USE_RUNFILES_DIR=1 +else + export _RLOCATION_USE_RUNFILES_DIR= +fi + case "$(uname -s | tr '[:upper:]' '[:lower:]')" in msys*|mingw*|cygwin*) # matches an absolute Windows path @@ -229,14 +249,18 @@ function runfiles_export_envvars() { export RUNFILES_MANIFEST_FILE= fi elif [[ ! -d "${RUNFILES_DIR:-/dev/null}" ]]; then + # Only pass on a runfiles directory that has been fully materialized so that + # subprocesses that aren't aware of this issue don't blindly use it. if [[ "$RUNFILES_MANIFEST_FILE" == */MANIFEST \ - && -d "${RUNFILES_MANIFEST_FILE%/MANIFEST}" ]]; then + && -f "${RUNFILES_MANIFEST_FILE%/MANIFEST}/$_RLOCATION_RUNFILES_LIB_PATH" ]]; then export RUNFILES_DIR="${RUNFILES_MANIFEST_FILE%/MANIFEST}" export JAVA_RUNFILES="$RUNFILES_DIR" + export _RLOCATION_USE_RUNFILES_DIR=1 elif [[ "$RUNFILES_MANIFEST_FILE" == *_manifest \ - && -d "${RUNFILES_MANIFEST_FILE%_manifest}" ]]; then + && -f "${RUNFILES_MANIFEST_FILE%_manifest}/$_RLOCATION_RUNFILES_LIB_PATH" ]]; then export RUNFILES_DIR="${RUNFILES_MANIFEST_FILE%_manifest}" export JAVA_RUNFILES="$RUNFILES_DIR" + export _RLOCATION_USE_RUNFILES_DIR=1 else export RUNFILES_DIR= fi @@ -267,43 +291,29 @@ function runfiles_current_repository() { fi local rlocation_path= + # Replace \ with / since the manifest uses / as the path separator even on Windows. + local normalized_caller_path + normalized_caller_path="$(echo "$caller_path" | sed 's|\\\\*|/|g')" - # If the runfiles manifest exists, search for an entry with target the caller's path. - if [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then - # Escape $caller_path for use in the grep regex below. Also replace \ with / since the manifest - # uses / as the path separator even on Windows. - local -r normalized_caller_path="$(echo "$caller_path" | sed 's|\\\\*|/|g')" + # The caller's path was returned by rlocation, so only the source of truth that rlocation uses can + # contain it: a path looked up in the runfiles directory lies in that directory and is never the + # target of a manifest entry, which is the location of the original file. + if [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" && -z "$_RLOCATION_USE_RUNFILES_DIR" ]]; then + # Search for an entry whose target is the caller's path. local -r escaped_caller_path="$(__runfiles_escape_grep "$normalized_caller_path")" rlocation_path=$(__runfiles_maybe_grep -m1 "^[^ ]* ${escaped_caller_path}$" "${RUNFILES_MANIFEST_FILE}" | cut -d ' ' -f 1) if [[ -z "$rlocation_path" ]]; then if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then - echo >&2 "ERROR[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) is not the target of an entry in the runfiles manifest ($RUNFILES_MANIFEST_FILE)" - fi - # The binary may also be run directly from bazel-bin or bazel-out. - local -r repository=$(echo "$normalized_caller_path" | __runfiles_maybe_grep -E -o '(^|/)(bazel-out/[^/]+/bin|bazel-bin)/external/[^/]+/' | tail -1 | awk -F/ '{print $(NF-1)}') - if [[ -n "$repository" ]]; then - if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then - echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) lies in repository ($repository) (parsed exec path)" - fi - echo "$repository" - else - if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then - echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) lies in the main repository (parsed exec path)" - fi - echo "" + echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) is not the target of an entry in the runfiles manifest ($RUNFILES_MANIFEST_FILE)" fi - return 1 else if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) is the target of ($rlocation_path) in the runfiles manifest" fi fi - fi - - # If the runfiles directory exists, check if the caller's path is of the form - # $RUNFILES_DIR/rlocation_path and if so, set $rlocation_path. - if [[ -z "$rlocation_path" && -d "${RUNFILES_DIR:-/dev/null}" ]]; then - normalized_caller_path="$(echo "$caller_path" | sed 's|\\\\*|/|g')" + elif [[ -d "${RUNFILES_DIR:-/dev/null}" ]]; then + # Check whether the caller's path is of the form $RUNFILES_DIR/rlocation_path. + local normalized_dir normalized_dir="$(echo "${RUNFILES_DIR%[\/]}" | sed 's|\\\\*|/|g')" if [[ -n "${_RLOCATION_GREP_CASE_INSENSITIVE_ARGS}" ]]; then # When comparing file paths insensitively, also normalize the case of the prefixes. @@ -318,37 +328,38 @@ function runfiles_current_repository() { if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) does not lie under the runfiles directory ($normalized_dir)" fi - # The only shell script that is not executed from the runfiles directory (if it is populated) - # is the sh_binary entrypoint. Parse its path under the execroot, using the last match to - # allow for nested execroots (e.g. in Bazel integration tests). The binary may also be run - # directly from bazel-bin. - local -r repository=$(echo "$normalized_caller_path" | __runfiles_maybe_grep -E -o '(^|/)(bazel-out/[^/]+/bin|bazel-bin)/external/[^/]+/' | tail -1 | awk -F/ '{print $(NF-1)}') - if [[ -n "$repository" ]]; then - if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then - echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) lies in repository ($repository) (parsed exec path)" - fi - echo "$repository" - else - if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then - echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) lies in the main repository (parsed exec path)" - fi - echo "" - fi - return 0 else if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($caller_path) has path ($rlocation_path) relative to the runfiles directory ($RUNFILES_DIR)" fi fi - fi - - if [[ -z "$rlocation_path" ]]; then + else if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "ERROR[runfiles.bash]: runfiles_current_repository($idx): cannot determine repository for ($caller_path) since neither the runfiles directory (${RUNFILES_DIR:-}) nor the runfiles manifest (${RUNFILES_MANIFEST_FILE:-}) exist" fi return 1 fi + if [[ -z "$rlocation_path" ]]; then + # The only shell script that is not among the runfiles is the entry point of a binary, which is + # executed from the execroot. Parse its path under the execroot, using the last match to allow + # for nested execroots (e.g. in Bazel integration tests). The binary may also be run directly + # from bazel-bin. + local -r repository=$(echo "$normalized_caller_path" | __runfiles_maybe_grep -E -o '(^|/)(bazel-out/[^/]+/bin|bazel-bin)/external/[^/]+/' | tail -1 | awk -F/ '{print $(NF-1)}') + if [[ -n "$repository" ]]; then + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) lies in repository ($repository) (parsed exec path)" + fi + echo "$repository" + else + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) lies in the main repository (parsed exec path)" + fi + echo "" + fi + return 0 + fi + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($caller_path) corresponds to rlocation path ($rlocation_path)" fi @@ -374,11 +385,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. - # 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 + # Exactly one of the runfiles directory and the manifest is consulted, so that the paths returned + # by this function are consistent with those that runfiles_current_repository resolves. The + # directory is preferred if it is usable since looking a path up in it does not require scanning + # the manifest, which can be very large. + if [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" && -z "$_RLOCATION_USE_RUNFILES_DIR" ]]; then if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: rlocation($1): looking in RUNFILES_MANIFEST_FILE ($RUNFILES_MANIFEST_FILE)" fi diff --git a/tests/runfiles/runfiles_test.bash b/tests/runfiles/runfiles_test.bash index fd60eee..43c1ae6 100755 --- a/tests/runfiles/runfiles_test.bash +++ b/tests/runfiles/runfiles_test.bash @@ -73,6 +73,13 @@ function find_runfiles_lib() { fi } +# Marks the given runfiles directory as fully materialized by placing the runfiles library in it at +# the path the initialization snippet looks it up at. +function materialize_runfiles_dir() { + mkdir -p "$1/bazel_tools/tools/bash/runfiles" + cp "$runfiles_lib_path" "$1/bazel_tools/tools/bash/runfiles/runfiles.bash" +} + function test_rlocation_call_requires_no_envvars() { export RUNFILES_DIR=mock/runfiles export RUNFILES_MANIFEST_FILE= @@ -216,6 +223,7 @@ function test_manifest_based_envvars() { export RUNFILES_DIR= export RUNFILES_MANIFEST_FILE=$tmpdir/foo.runfiles_manifest mkdir -p $tmpdir/foo.runfiles + materialize_runfiles_dir "$tmpdir/foo.runfiles" source "$runfiles_lib_path" runfiles_export_envvars @@ -574,20 +582,50 @@ EOF # 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() { +# runfiles_export_envvars promotes to RUNFILES_MANIFEST_FILE. rlocation and +# runfiles_current_repository then have to agree on which of the two they use: +# the manifest maps rlocation paths to the locations of the original files, so a +# path that rlocation resolved against the runfiles directory can never be the +# target of a manifest entry. +function test_current_repository_materialized_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" + mkdir -p "$RUNFILES_DIR" + 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 + # Copies that must never be sourced: the runfiles directory is materialized, so the manifest is + # not consulted at all. + write_current_repository_lib "$tmpdir/protobuf+3.19.2/foo/lib.sh" repo_of_other unused + write_current_repository_lib "$tmpdir/_main/bar/lib.sh" repo_of_main unused + 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 + materialize_runfiles_dir "$RUNFILES_DIR" + source "$runfiles_lib_path" + + [[ "$(rlocation "protobuf+3.19.2/foo/lib.sh" "" || echo failed)" == "$RUNFILES_DIR/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)" == "$RUNFILES_DIR/_main/bar/lib.sh" ]] || fail + source "$(rlocation "_main/bar/lib.sh" "")" || fail + [[ "$(repo_of_main || echo failed)" == "" ]] || fail +} + +# With --noenable_runfiles, Bazel leaves behind a runfiles directory that only contains the MANIFEST +# file and the subdirectory of the main repository, which both functions have to recognize as not +# usable. +function test_current_repository_unmaterialized_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" + mkdir -p "$RUNFILES_DIR/_main" 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 @@ -603,6 +641,70 @@ EOF [[ "$(repo_of_main || echo failed)" == "" ]] || fail } +# A materialized runfiles directory is preferred over the manifest, which requires scanning a +# potentially very large file. +function test_rlocation_prefers_materialized_directory() { + local tmpdir="$(mktemp -d $TEST_TMPDIR/tmp.XXXXXXXX)" + + export RUNFILES_DIR="${tmpdir}/foo.runfiles" + export RUNFILES_MANIFEST_FILE="$RUNFILES_DIR/MANIFEST" + mkdir -p "$RUNFILES_DIR/_main/pkg" "$tmpdir/original/pkg" + touch "$RUNFILES_DIR/_main/pkg/file" "$tmpdir/original/pkg/file" + cat > "$RUNFILES_MANIFEST_FILE" << EOF +_main/pkg/file $tmpdir/original/pkg/file +EOF + materialize_runfiles_dir "$RUNFILES_DIR" + source "$runfiles_lib_path" + + [[ "$(rlocation _main/pkg/file || echo failed)" == "$RUNFILES_DIR/_main/pkg/file" ]] || fail + + # A runfile that is missing from the materialized directory is not looked up in the manifest. + rm "$RUNFILES_DIR/_main/pkg/file" + [[ "$(rlocation _main/pkg/file || echo failed)" == failed ]] || fail +} + +# A subprocess may use the exported functions without sourcing the library itself, in which case it +# does not learn which of the two the parent picked. It doesn't have to: it inherits RUNFILES_DIR +# and RUNFILES_MANIFEST_FILE, either of which is usable on its own if set. +function test_exported_functions_in_subprocess() { + local tmpdir="$(mktemp -d $TEST_TMPDIR/tmp.XXXXXXXX)" + + export RUNFILES_DIR="$tmpdir/foo.runfiles" + export RUNFILES_MANIFEST_FILE= + mkdir -p "$RUNFILES_DIR/_main/pkg" "$tmpdir/original/pkg" + touch "$RUNFILES_DIR/_main/pkg/file" "$tmpdir/original/pkg/file" + materialize_runfiles_dir "$RUNFILES_DIR" + source "$runfiles_lib_path" + + [[ "$(bash -c 'set -euo pipefail; rlocation _main/pkg/file')" == "$RUNFILES_DIR/_main/pkg/file" ]] || fail + + # With both set, the subprocess is free to pick the manifest even though this process picked the + # directory, as long as what it resolves to exists. + export RUNFILES_MANIFEST_FILE="$RUNFILES_DIR/MANIFEST" + cat > "$RUNFILES_MANIFEST_FILE" << EOF +_main/pkg/file $tmpdir/original/pkg/file +EOF + local -r resolved="$(bash -c 'set -euo pipefail; rlocation _main/pkg/file')" + [[ -f "$resolved" ]] || fail "subprocess resolved to $resolved" +} + +# A runfiles directory that has not been fully materialized should not be passed on to subprocesses, +# which may blindly trust the value they are given. +function test_manifest_based_envvars_unmaterialized_directory() { + local tmpdir="$(mktemp -d $TEST_TMPDIR/tmp.XXXXXXXX)" + echo "a b" > $tmpdir/foo.runfiles_manifest + + export RUNFILES_DIR= + export RUNFILES_MANIFEST_FILE=$tmpdir/foo.runfiles_manifest + # What --noenable_runfiles leaves behind. + mkdir -p $tmpdir/foo.runfiles/_main + source "$runfiles_lib_path" + + runfiles_export_envvars + [[ -z "${RUNFILES_DIR:-}" ]] || fail + [[ "${RUNFILES_MANIFEST_FILE:-}" == "$tmpdir/foo.runfiles_manifest" ]] || fail +} + function test_directory_based_envvars() { export RUNFILES_DIR=mock/runfiles export RUNFILES_MANIFEST_FILE=