fix: add runfiles root for system_python bootstrap (#3423)

When the system_bootstrap code was changed to using the site init for
adding to sys.path,
importing `bazel_tools.tools.python.runfiles` stopped working. This is
because the
runfiles root was no longer being added to sys.path.

This is somewhat WAI because:
1. Always adding the runfiles root to sys.path is a deprecated legacy
behavior because
it can interfere with Python imports (a repo name can mask a legitimate
import).
2. Under bzlmod, repo directory names aren't importable Python names, so
having the
   runfiles root on sys.path doesn't do much.

An exception to (2) is bazel_tools: this is special cased to use the
directory
name `bazel_tools` in runfiles. This is where the legacy runfiles
library for
Python is.

In any case, forgetting the runfiles root on sys.path for the
system_python bootstrap
was an oversight, as the intention was to be a more no-op refactoring.

Fixes #3422

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Author: rickeylev

CHANGELOG.md

@@ -91,7 +91,8 @@ END_UNRELEASED_TEMPLATE
 * (performance) 90% reduction in py_binary/py_test analysis phase cost.
   ([#3381](https://github.com/bazel-contrib/rules_python/pull/3381)).
 * (gazelle) Fix `gazelle_python_manifest.test` so that it accesses manifest files via `runfile` path handling rather than directly ([#3397](https://github.com/bazel-contrib/rules_python/issues/3397)).
-
+* (core rules) For the system_python bootstrap, the runfiles root is added to
+  sys.path.
 
 {#v0-0-0-added}
 ### Added

python/private/py_executable.bzl

@@ -345,6 +345,9 @@ def _create_executable(
             output_prefix = base_executable_name,
             imports = imports,
             runtime_details = runtime_details,
+            add_runfiles_root_to_sys_path = (
+                "1" if BootstrapImplFlag.get_value(ctx) == BootstrapImplFlag.SYSTEM_PYTHON else "0"
+            ),
         )
 
         stage2_bootstrap = _create_stage2_bootstrap(
@@ -508,7 +511,7 @@ def _create_zip_main(ctx, *, stage2_bootstrap, runtime_details, venv):
 # * https://snarky.ca/how-virtual-environments-work/
 # * https://github.com/python/cpython/blob/main/Modules/getpath.py
 # * https://github.com/python/cpython/blob/main/Lib/site.py
-def _create_venv(ctx, output_prefix, imports, runtime_details):
+def _create_venv(ctx, output_prefix, imports, runtime_details, add_runfiles_root_to_sys_path):
     create_full_venv = BootstrapImplFlag.get_value(ctx) == BootstrapImplFlag.SCRIPT
     venv = "_{}.venv".format(output_prefix.lstrip("_"))
 
@@ -596,6 +599,7 @@ def _create_venv(ctx, output_prefix, imports, runtime_details):
         template = runtime.site_init_template,
         output = site_init,
         substitutions = {
+            "%add_runfiles_root_to_sys_path%": add_runfiles_root_to_sys_path,
             "%coverage_tool%": _get_coverage_tool_runfiles_path(ctx, runtime),
             "%import_all%": "True" if read_possibly_native_flag(ctx, "python_import_all_repositories") else "False",
             "%site_init_runfiles_path%": "{}/{}".format(ctx.workspace_name, site_init.short_path),

python/private/site_init_template.py

@@ -26,6 +26,8 @@
 _SELF_RUNFILES_RELATIVE_PATH = "%site_init_runfiles_path%"
 # Runfiles-relative path to the coverage tool entry point, if any.
 _COVERAGE_TOOL = "%coverage_tool%"
+# True if the runfiles root should be added to sys.path
+_ADD_RUNFILES_ROOT_TO_SYS_PATH = "%add_runfiles_root_to_sys_path%" == "1"
 
 
 def _is_verbose():
@@ -127,11 +129,6 @@ def _search_path(name):
 def _setup_sys_path():
     """Perform Bazel/binary specific sys.path setup.
 
-    NOTE: We do not add _RUNFILES_ROOT to sys.path for two reasons:
-    1. Under workspace, it makes every external repository importable. If a Bazel
-       repository matches a Python import name, they conflict.
-    2. Under bzlmod, the repo names in the runfiles directory aren't importable
-       Python names, so there's no point in adding the runfiles root to sys.path.
     """
     seen = set(sys.path)
     python_path_entries = []
@@ -147,6 +144,17 @@ def _maybe_add_path(path):
         sys.path.append(path)
         seen.add(path)
 
+    # Adding the runfiles root to sys.path is a legacy behavior that will be
+    # removed. We don't want to add it to sys.path for two reasons:
+    # 1. Under workspace, it makes every external repository importable. If a Bazel
+    #    repository matches a Python import name, they conflict.
+    # 2. Under bzlmod, the repo names in the runfiles directory aren't importable
+    #    Python names, so there's no point in adding the runfiles root to sys.path.
+    # For temporary compatibility with the original system_python bootstrap
+    # behavior, it is conditionally added for that boostrap mode.
+    if _ADD_RUNFILES_ROOT_TO_SYS_PATH:
+        _maybe_add_path(_RUNFILES_ROOT)
+
     for rel_path in _IMPORTS_STR.split(":"):
         abs_path = os.path.join(_RUNFILES_ROOT, rel_path)
         _maybe_add_path(abs_path)

tests/bootstrap_impls/BUILD.bazel

@@ -104,6 +104,18 @@ sh_py_run_test(
     target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT,
 )
 
+py_reconfig_test(
+    name = "bazel_tools_importable_system_python_test",
+    srcs = ["bazel_tools_importable_test.py"],
+    bootstrap_impl = "system_python",
+    # Necessary because bazel_tools doesn't have __init__.py files.
+    legacy_create_init = True,
+    main = "bazel_tools_importable_test.py",
+    deps = [
+        "@bazel_tools//tools/python/runfiles",
+    ],
+)
+
 py_reconfig_test(
     name = "sys_path_order_bootstrap_script_test",
     srcs = ["sys_path_order_test.py"],
@@ -121,6 +133,9 @@ py_reconfig_test(
     env = {"BOOTSTRAP": "system_python"},
     imports = ["./site-packages"],
     main = "sys_path_order_test.py",
+    deps = [
+        "@bazel_tools//tools/python/runfiles",
+    ],
 )
 
 py_reconfig_test(

tests/bootstrap_impls/bazel_tools_importable_test.py

@@ -0,0 +1,20 @@
+import sys
+import unittest
+
+
+class BazelToolsImportableTest(unittest.TestCase):
+    def test_bazel_tools_importable(self):
+        try:
+            import bazel_tools
+            import bazel_tools.tools.python
+            import bazel_tools.tools.python.runfiles
+        except ImportError as exc:
+            raise AssertionError(
+                "Failed to import bazel_tools.python.runfiles\n"
+                + "sys.path:\n"
+                + "\n".join(f"{i}: {v}" for i, v in enumerate(sys.path))
+            ) from exc
+
+
+if __name__ == "__main__":
+    unittest.main()