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 #3518

Author: adhoc-bobcat

CHANGELOG.md

@@ -67,6 +67,9 @@ END_UNRELEASED_TEMPLATE
 * (zipapp) Resolve issue passing through compression settings in
   `py_zippapp_binary` targets
   ([#3646](https://github.com/bazel-contrib/rules_python/issues/3646)).
+* (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

python/private/python_bootstrap_template.txt

@@ -203,6 +203,13 @@ def FindModuleSpace(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

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)

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"],

tests/bootstrap_impls/bin_calls_bin/inner.py

@@ -1,4 +1,14 @@
 import os
 
 module_space = os.environ.get("RULES_PYTHON_TESTING_MODULE_SPACE")
+runfiles_dir = os.environ.get("RUNFILES_DIR")
+runfiles_manifest_file = os.environ.get("RUNFILES_MANIFEST_FILE")
 print(f"inner: RULES_PYTHON_TESTING_MODULE_SPACE='{module_space}'")
+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}'")

tests/bootstrap_impls/bin_calls_bin/inner_lib.py

@@ -0,0 +1,3 @@
+# Rather than having a completely empty file...
+def confirm():
+    return "success"

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)

tests/bootstrap_impls/bin_calls_bin/verify.sh

@@ -11,6 +11,18 @@ verify_output() {
   echo "Outer module space: $OUTER_MODULE_SPACE"
   echo "Inner module space: $INNER_MODULE_SPACE"
 
+  # 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_MODULE_SPACE" == "$INNER_MODULE_SPACE" ]; then
     echo "Error: Outer and Inner module spaces 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."
 }