Skip to content

Commit 06eaaa2

Browse files
rickeylevrickeylev
authored
fix(bootstrap): handle when runfiles env vars don't point to current binary's runfiles (#3192)
The stage1 bootstrap script had a bug in the find_runfiles_root function where it would unconditionally use the RUNFILES_DIR et al environment variables if they were set. This failed in a particular nested context: an outer binary calling an inner binary when the inner binary isn't a data dependency of the outer binary (i.e. the outer doesn't contain the inner in runfiles). This would cause the inner binary to incorrectly resolve its runfiles, leading to failures. Such a case can occur if a genrule calls the outer binary, which has the inner binary passed as an arg. This change adds a check to validate that the script's entry point exists within the inherited RUNFILES_DIR before using it. If the entry point is not found, it proceeds with other runfiles discovery methods. This matches the system_python runfiles discovery logic. Fixes #3187
1 parent 24146a4 commit 06eaaa2

File tree

10 files changed

+196
-6
lines changed

10 files changed

+196
-6
lines changed

‎CHANGELOG.md

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,29 @@ BEGIN_UNRELEASED_TEMPLATE
4747
END_UNRELEASED_TEMPLATE
4848
-->
4949

50+
{#v0-0-0}
51+
## Unreleased
52+
53+
[0.0.0]: https://github.com/bazel-contrib/rules_python/releases/tag/0.0.0
54+
55+
{#v0-0-0-changed}
56+
### Changed
57+
* Nothing changed.
58+
59+
{#v0-0-0-fixed}
60+
### Fixed
61+
* (bootstrap) The stage1 bootstrap script now correctly handles nested `RUNFILES_DIR`
62+
environments, fixing issues where a `py_binary` calls another `py_binary`
63+
([#3187](https://github.com/bazel-contrib/rules_python/issues/3187)).
64+
65+
{#v0-0-0-added}
66+
### Added
67+
* Nothing added.
68+
69+
{#v0-0-0-removed}
70+
### Removed
71+
* Nothing removed.
72+
5073
{#1-6-0}
5174
## [1.6.0] - 2025-08-23
5275

@@ -102,7 +125,7 @@ END_UNRELEASED_TEMPLATE
102125
name.
103126
* (pypi) The selection of the whls has been changed and should no longer result
104127
in ambiguous select matches ({gh-issue}`2759`) and should be much more efficient
105-
when running `bazel query` due to fewer repositories being included
128+
when running `bazel query` due to fewer repositories being included
106129
({gh-issue}`2849`).
107130
* Multi-line python imports (e.g. with escaped newlines) are now correctly processed by Gazelle.
108131
* (toolchains) `local_runtime_repo` works with multiarch Debian with Python 3.8

‎python/private/python_bootstrap_template.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,9 @@ def Main():
516516
module_space = FindModuleSpace(main_rel_path)
517517
delete_module_space = False
518518

519+
if os.environ.get("RULES_PYTHON_TESTING_TELL_MODULE_SPACE"):
520+
new_env["RULES_PYTHON_TESTING_MODULE_SPACE"] = module_space
521+
519522
python_imports = '%imports%'
520523
python_path_entries = CreatePythonPathEntries(python_imports, module_space)
521524
python_path_entries += GetRepositoriesImports(module_space, %import_all%)

‎python/private/stage1_bootstrap_template.sh

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,14 +61,20 @@ if [[ "$IS_ZIPFILE" == "1" ]]; then
6161

6262
else
6363
function find_runfiles_root() {
64+
local maybe_root=""
6465
if [[ -n "${RUNFILES_DIR:-}" ]]; then
65-
echo "$RUNFILES_DIR"
66-
return 0
66+
maybe_root="$RUNFILES_DIR"
6767
elif [[ "${RUNFILES_MANIFEST_FILE:-}" = *".runfiles_manifest" ]]; then
68-
echo "${RUNFILES_MANIFEST_FILE%%.runfiles_manifest}.runfiles"
69-
return 0
68+
maybe_root="${RUNFILES_MANIFEST_FILE%%.runfiles_manifest}.runfiles"
7069
elif [[ "${RUNFILES_MANIFEST_FILE:-}" = *".runfiles/MANIFEST" ]]; then
71-
echo "${RUNFILES_MANIFEST_FILE%%.runfiles/MANIFEST}.runfiles"
70+
maybe_root="${RUNFILES_MANIFEST_FILE%%.runfiles/MANIFEST}.runfiles"
71+
fi
72+
73+
# The RUNFILES_DIR et al variables may misreport the runfiles directory
74+
# if an outer binary invokes this binary when it isn't a data dependency.
75+
# e.g. a genrule calls `bazel-bin/outer --inner=bazel-bin/inner`
76+
if [[ -n "$maybe_root" && -e "$maybe_root/$STAGE2_BOOTSTRAP" ]]; then
77+
echo "$maybe_root"
7278
return 0
7379
fi
7480

@@ -99,6 +105,9 @@ else
99105
RUNFILES_DIR=$(find_runfiles_root $0)
100106
fi
101107

108+
if [[ -n "$RULES_PYTHON_TESTING_TELL_MODULE_SPACE" ]]; then
109+
export RULES_PYTHON_TESTING_MODULE_SPACE="$RUNFILES_DIR"
110+
fi
102111

103112
function find_python_interpreter() {
104113
runfiles_root="$1"

‎tests/bootstrap_impls/bin_calls_bin/BUILD.bazel

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
load("@rules_shell//shell:sh_test.bzl", "sh_test")
2+
load("//tests/support:py_reconfig.bzl", "py_reconfig_binary")
3+
load("//tests/support:support.bzl", "NOT_WINDOWS", "SUPPORTS_BOOTSTRAP_SCRIPT")
4+
5+
# =====
6+
# bootstrap_impl=system_python testing
7+
# =====
8+
py_reconfig_binary(
9+
name = "outer_bootstrap_system_python",
10+
srcs = ["outer.py"],
11+
bootstrap_impl = "system_python",
12+
main = "outer.py",
13+
tags = ["manual"],
14+
)
15+
16+
py_reconfig_binary(
17+
name = "inner_bootstrap_system_python",
18+
srcs = ["inner.py"],
19+
bootstrap_impl = "system_python",
20+
main = "inner.py",
21+
tags = ["manual"],
22+
)
23+
24+
genrule(
25+
name = "outer_calls_inner_system_python",
26+
outs = ["outer_calls_inner_system_python.out"],
27+
cmd = "RULES_PYTHON_TESTING_TELL_MODULE_SPACE=1 $(location :outer_bootstrap_system_python) $(location :inner_bootstrap_system_python) > $@",
28+
tags = ["manual"],
29+
tools = [
30+
":inner_bootstrap_system_python",
31+
":outer_bootstrap_system_python",
32+
],
33+
)
34+
35+
sh_test(
36+
name = "bootstrap_system_python_test",
37+
srcs = ["verify_system_python.sh"],
38+
data = [
39+
"verify.sh",
40+
":outer_calls_inner_system_python",
41+
],
42+
# The way verify_system_python.sh loads verify.sh doesn't work
43+
# with Windows for some annoying reason. Just skip windows for now;
44+
# the logic being test isn't OS-specific, so this should be fine.
45+
target_compatible_with = NOT_WINDOWS,
46+
)
47+
48+
# =====
49+
# bootstrap_impl=script testing
50+
# =====
51+
py_reconfig_binary(
52+
name = "inner_bootstrap_script",
53+
srcs = ["inner.py"],
54+
bootstrap_impl = "script",
55+
main = "inner.py",
56+
tags = ["manual"],
57+
)
58+
59+
py_reconfig_binary(
60+
name = "outer_bootstrap_script",
61+
srcs = ["outer.py"],
62+
bootstrap_impl = "script",
63+
main = "outer.py",
64+
tags = ["manual"],
65+
)
66+
67+
genrule(
68+
name = "outer_calls_inner_script_python",
69+
outs = ["outer_calls_inner_script_python.out"],
70+
cmd = "RULES_PYTHON_TESTING_TELL_MODULE_SPACE=1 $(location :outer_bootstrap_script) $(location :inner_bootstrap_script) > $@",
71+
tags = ["manual"],
72+
tools = [
73+
":inner_bootstrap_script",
74+
":outer_bootstrap_script",
75+
],
76+
)
77+
78+
sh_test(
79+
name = "bootstrap_script_python_test",
80+
srcs = ["verify_script_python.sh"],
81+
data = [
82+
"verify.sh",
83+
":outer_calls_inner_script_python",
84+
],
85+
target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT,
86+
)

‎tests/bootstrap_impls/bin_calls_bin/inner.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import os
2+
3+
module_space = os.environ.get("RULES_PYTHON_TESTING_MODULE_SPACE")
4+
print(f"inner: RULES_PYTHON_TESTING_MODULE_SPACE='{module_space}'")

‎tests/bootstrap_impls/bin_calls_bin/outer.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import os
2+
import subprocess
3+
import sys
4+
5+
if __name__ == "__main__":
6+
module_space = os.environ.get("RULES_PYTHON_TESTING_MODULE_SPACE")
7+
print(f"outer: RULES_PYTHON_TESTING_MODULE_SPACE='{module_space}'")
8+
9+
inner_binary_path = sys.argv[1]
10+
result = subprocess.run(
11+
[inner_binary_path],
12+
capture_output=True,
13+
text=True,
14+
check=True,
15+
)
16+
print(result.stdout, end="")
17+
if result.stderr:
18+
print(result.stderr, end="", file=sys.stderr)

‎tests/bootstrap_impls/bin_calls_bin/verify.sh

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
#!/bin/bash
2+
set -euo pipefail
3+
4+
verify_output() {
5+
local OUTPUT_FILE=$1
6+
7+
# Extract the RULES_PYTHON_TESTING_MODULE_SPACE values
8+
local OUTER_MODULE_SPACE=$(grep "outer: RULES_PYTHON_TESTING_MODULE_SPACE" "$OUTPUT_FILE" | sed "s/outer: RULES_PYTHON_TESTING_MODULE_SPACE='\(.*\)'/\1/")
9+
local INNER_MODULE_SPACE=$(grep "inner: RULES_PYTHON_TESTING_MODULE_SPACE" "$OUTPUT_FILE" | sed "s/inner: RULES_PYTHON_TESTING_MODULE_SPACE='\(.*\)'/\1/")
10+
11+
echo "Outer module space: $OUTER_MODULE_SPACE"
12+
echo "Inner module space: $INNER_MODULE_SPACE"
13+
14+
# Check 1: The two values are different
15+
if [ "$OUTER_MODULE_SPACE" == "$INNER_MODULE_SPACE" ]; then
16+
echo "Error: Outer and Inner module spaces are the same."
17+
exit 1
18+
fi
19+
20+
# Check 2: Inner is not a subdirectory of Outer
21+
case "$INNER_MODULE_SPACE" in
22+
"$OUTER_MODULE_SPACE"/*)
23+
echo "Error: Inner module space is a subdirectory of Outer's."
24+
exit 1
25+
;;
26+
*)
27+
# This is the success case
28+
;;
29+
esac
30+
31+
echo "Verification successful."
32+
}

‎tests/bootstrap_impls/bin_calls_bin/verify_script_python.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
#!/bin/bash
2+
set -euo pipefail
3+
4+
source "$(dirname "$0")/verify.sh"
5+
verify_output "$(dirname "$0")/outer_calls_inner_script_python.out"

‎tests/bootstrap_impls/bin_calls_bin/verify_system_python.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
#!/bin/bash
2+
set -euo pipefail
3+
4+
source "$(dirname "$0")/verify.sh"
5+
verify_output "$(dirname "$0")/outer_calls_inner_system_python.out"

‎tests/support/support.bzl

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,3 +54,8 @@ SUPPORTS_BZLMOD_UNIXY = select({
5454
"@platforms//os:windows": ["@platforms//:incompatible"],
5555
"//conditions:default": [],
5656
}) if BZLMOD_ENABLED else ["@platforms//:incompatible"]
57+
58+
NOT_WINDOWS = select({
59+
"@platforms//os:windows": ["@platforms//:incompatible"],
60+
"//conditions:default": [],
61+
})

0 commit comments

Comments
 (0)