From d311b9efcf31f7af2ffb0bd7383edbda774b6745 Mon Sep 17 00:00:00 2001 From: Lloyd Pique Date: Wed, 4 Mar 2026 11:56:53 -0800 Subject: [PATCH 1/2] fix(bootstrap) handle when the runfiles env vars are not correct There was still an issue where the runfiles environment variables could be set for a parent Python binary, and if that spawned another Python binary (directly or indirectly), the child would try to use the parents runfiles. This was confirmable by adding a py_library dependency to the existing bin_calls_bin test, and having inner.py try to import it. A workaround sugggested in the bug for code outside of rules_python was to remove RUNFILES_DIR from the environment inherited by the child when launching the child process, though in some cases you might need to also remove RUNFILES_MANIFEST_FILE, but working around the issue is not very satisfactory. Diving into the bootstrapping process, it seemed like the proper fix was to conditionally unset them, if they were not correct. I found equivalent places in both bootstrap template files, and if the test to confirm "runfile_dir" was correct fails, the bootstrap code itself now removes those invalid values from the environment. Later bootstrap code assumes they are set correctly, if set. When they are not set, it goes through some fallback logic to locate them. By conditionally unsetting them, the fallback logic is not invoked when it isn't necessary, shortening the bootstrap process. With that change, the modified tests now pass. Fixes https://github.com/bazel-contrib/rules_python/issues/3518 --- CHANGELOG.md | 3 +++ python/private/python_bootstrap_template.txt | 7 +++++++ python/private/stage2_bootstrap_template.py | 7 +++++++ .../bootstrap_impls/bin_calls_bin/BUILD.bazel | 9 +++++++++ tests/bootstrap_impls/bin_calls_bin/inner.py | 10 ++++++++++ .../bootstrap_impls/bin_calls_bin/inner_lib.py | 3 +++ tests/bootstrap_impls/bin_calls_bin/outer.py | 2 +- tests/bootstrap_impls/bin_calls_bin/verify.sh | 18 ++++++++++++++++++ 8 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 tests/bootstrap_impls/bin_calls_bin/inner_lib.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e7c152acd..e83c3b8ed3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,9 @@ Other changes: * (bootstrap) Fixed incorrect runfiles path construction in bootstrap scripts when binary is defined in another bazel module ([#3563](https://github.com/bazel-contrib/rules_python/issues/3563)). +* (bootstrap) Resolve `RUNFILES_DIR` inheritance issues, which lead to a child + Python binary incorrectly using it's parent's Python binary environment + ([#3518](https://github.com/bazel-contrib/rules_python/issues/3518)). {#v0-0-0-added} ### Added diff --git a/python/private/python_bootstrap_template.txt b/python/private/python_bootstrap_template.txt index 4efd46690a..51c9013e9d 100644 --- a/python/private/python_bootstrap_template.txt +++ b/python/private/python_bootstrap_template.txt @@ -214,6 +214,13 @@ def find_runfiles_root(main_rel_path): if runfiles_dir and os.path.exists(os.path.join(runfiles_dir, main_rel_path)): return runfiles_dir + # Clear RUNFILES_DIR & RUNFILES_MANIFEST_FILE since the runfiles dir was + # not found. These can be correctly set for a parent Python process, but + # inherited by the child, and not correct for it. Later bootstrap code + # assumes they're are correct if set. + os.environ.pop('RUNFILES_DIR', None) + os.environ.pop('RUNFILES_MANIFEST_FILE', None) + stub_filename = sys.argv[0] # On Windows, the path may contain both forward and backslashes. # Normalize to the OS separator because the regex used later assumes diff --git a/python/private/stage2_bootstrap_template.py b/python/private/stage2_bootstrap_template.py index c03a4a2e62..3c2d6807cc 100644 --- a/python/private/stage2_bootstrap_template.py +++ b/python/private/stage2_bootstrap_template.py @@ -185,6 +185,13 @@ def find_runfiles_root(main_rel_path): if runfiles_dir and os.path.exists(os.path.join(runfiles_dir, main_rel_path)): return runfiles_dir + # Clear RUNFILES_DIR & RUNFILES_MANIFEST_FILE since the runfiles dir was + # not found. These can be correctly set for a parent Python process, but + # inherited by the child, and not correct for it. Later bootstrap code + # assumes they're are correct if set. + os.environ.pop('RUNFILES_DIR', None) + os.environ.pop('RUNFILES_MANIFEST_FILE', None) + stub_filename = sys.argv[0] if not os.path.isabs(stub_filename): stub_filename = os.path.join(os.getcwd(), stub_filename) diff --git a/tests/bootstrap_impls/bin_calls_bin/BUILD.bazel b/tests/bootstrap_impls/bin_calls_bin/BUILD.bazel index 1822ccd08e..394b485acb 100644 --- a/tests/bootstrap_impls/bin_calls_bin/BUILD.bazel +++ b/tests/bootstrap_impls/bin_calls_bin/BUILD.bazel @@ -1,6 +1,13 @@ load("@rules_shell//shell:sh_test.bzl", "sh_test") load("//tests/support:py_reconfig.bzl", "py_reconfig_binary") load("//tests/support:support.bzl", "NOT_WINDOWS", "SUPPORTS_BOOTSTRAP_SCRIPT") +load("//python:py_library.bzl", "py_library") + +py_library( + name = "inner_lib", + srcs = ["inner_lib.py"], + tags = ["manual"], +) # ===== # bootstrap_impl=system_python testing @@ -16,6 +23,7 @@ py_reconfig_binary( py_reconfig_binary( name = "inner_bootstrap_system_python", srcs = ["inner.py"], + deps = [":inner_lib"], bootstrap_impl = "system_python", main = "inner.py", tags = ["manual"], @@ -51,6 +59,7 @@ sh_test( py_reconfig_binary( name = "inner_bootstrap_script", srcs = ["inner.py"], + deps = [":inner_lib"], bootstrap_impl = "script", main = "inner.py", tags = ["manual"], diff --git a/tests/bootstrap_impls/bin_calls_bin/inner.py b/tests/bootstrap_impls/bin_calls_bin/inner.py index 96409eb8f8..6fef455a84 100644 --- a/tests/bootstrap_impls/bin_calls_bin/inner.py +++ b/tests/bootstrap_impls/bin_calls_bin/inner.py @@ -1,4 +1,14 @@ import os runfiles_root = os.environ.get("RULES_PYTHON_TESTING_RUNFILES_ROOT") +runfiles_dir = os.environ.get("RUNFILES_DIR") +runfiles_manifest_file = os.environ.get("RUNFILES_MANIFEST_FILE") print(f"inner: RULES_PYTHON_TESTING_RUNFILES_ROOT='{runfiles_root}'") +print(f"inner: RUNFILES_DIR='{runfiles_dir}'") +print(f"inner: RUNFILES_MANIFEST_FILE='{runfiles_manifest_file}'") + +try: + import tests.bootstrap_impls.bin_calls_bin.inner_lib as inner_lib + print(f"inner: import_result='{inner_lib.confirm()}'") +except ImportError as e: + print(f"inner: import_result='{e}'") diff --git a/tests/bootstrap_impls/bin_calls_bin/inner_lib.py b/tests/bootstrap_impls/bin_calls_bin/inner_lib.py new file mode 100644 index 0000000000..97efbb1565 --- /dev/null +++ b/tests/bootstrap_impls/bin_calls_bin/inner_lib.py @@ -0,0 +1,3 @@ +# Rather than having a completely empty file... +def confirm(): + return "success" \ No newline at end of file diff --git a/tests/bootstrap_impls/bin_calls_bin/outer.py b/tests/bootstrap_impls/bin_calls_bin/outer.py index f995c24ff6..a4432dff59 100644 --- a/tests/bootstrap_impls/bin_calls_bin/outer.py +++ b/tests/bootstrap_impls/bin_calls_bin/outer.py @@ -11,8 +11,8 @@ [inner_binary_path], capture_output=True, text=True, - check=True, ) print(result.stdout, end="") if result.stderr: print(result.stderr, end="", file=sys.stderr) + sys.exit(result.returncode) diff --git a/tests/bootstrap_impls/bin_calls_bin/verify.sh b/tests/bootstrap_impls/bin_calls_bin/verify.sh index 9a9a17c6c6..bbe4252cc1 100755 --- a/tests/bootstrap_impls/bin_calls_bin/verify.sh +++ b/tests/bootstrap_impls/bin_calls_bin/verify.sh @@ -11,6 +11,18 @@ verify_output() { echo "Outer runfiles root: $OUTER_RUNFILES_ROOT" echo "Inner runfiles root: $INNER_RUNFILES_ROOT" + # Extract the inner runfiles values + local INNER_RUNFILES_DIR=$(grep "inner: RUNFILES_DIR" "$OUTPUT_FILE" | sed "s/inner: RUNFILES_DIR='\(.*\)'/\1/") + local INNER_RUNFILES_MANIFEST_FILE=$(grep "inner: RUNFILES_MANIFEST_FILE" "$OUTPUT_FILE" | sed "s/inner: RUNFILES_MANIFEST_FILE='\(.*\)'/\1/") + + echo "Inner runfiles dir: $INNER_RUNFILES_DIR" + echo "Inner runfiles manifest file: $INNER_RUNFILES_MANIFEST_FILE" + + # Extract the inner lib import result + local INNER_LIB_IMPORT=$(grep "inner: import_result" "$OUTPUT_FILE" | sed "s/inner: import_result='\(.*\)'/\1/") + echo "Inner lib import result: $INNER_LIB_IMPORT" + + # Check 1: The two values are different if [ "$OUTER_RUNFILES_ROOT" == "$INNER_RUNFILES_ROOT" ]; then echo "Error: Outer and Inner runfiles roots are the same." @@ -28,5 +40,11 @@ verify_output() { ;; esac + # Check 3: inner_lib was imported + if [ "$INNER_LIB_IMPORT" != "success" ]; then + echo "Error: Inner lib was not successfully imported." + exit 1 + fi + echo "Verification successful." } From ec7dd107a975f8b3097efa8f25c237b89aa0ae16 Mon Sep 17 00:00:00 2001 From: Ignas Anikevicius <240938+aignas@users.noreply.github.com> Date: Fri, 3 Apr 2026 14:28:05 +0900 Subject: [PATCH 2/2] chore: buildifier --- tests/bootstrap_impls/bin_calls_bin/BUILD.bazel | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/bootstrap_impls/bin_calls_bin/BUILD.bazel b/tests/bootstrap_impls/bin_calls_bin/BUILD.bazel index 394b485acb..b5ba68f987 100644 --- a/tests/bootstrap_impls/bin_calls_bin/BUILD.bazel +++ b/tests/bootstrap_impls/bin_calls_bin/BUILD.bazel @@ -1,7 +1,7 @@ load("@rules_shell//shell:sh_test.bzl", "sh_test") +load("//python:py_library.bzl", "py_library") load("//tests/support:py_reconfig.bzl", "py_reconfig_binary") load("//tests/support:support.bzl", "NOT_WINDOWS", "SUPPORTS_BOOTSTRAP_SCRIPT") -load("//python:py_library.bzl", "py_library") py_library( name = "inner_lib", @@ -23,10 +23,10 @@ py_reconfig_binary( py_reconfig_binary( name = "inner_bootstrap_system_python", srcs = ["inner.py"], - deps = [":inner_lib"], bootstrap_impl = "system_python", main = "inner.py", tags = ["manual"], + deps = [":inner_lib"], ) genrule( @@ -59,10 +59,10 @@ sh_test( py_reconfig_binary( name = "inner_bootstrap_script", srcs = ["inner.py"], - deps = [":inner_lib"], bootstrap_impl = "script", main = "inner.py", tags = ["manual"], + deps = [":inner_lib"], ) py_reconfig_binary(