fix(runfiles): correct Python runfiles path assumption (#3086)

The current _FindPythonRunfilesRoot() implementation assumes that
the Python module has been unpacked four levels below the runfiles
directory. This is not the case in multiple situations, for example when
rules_pycross is in use and has installed the module via pypi (in which
case it is five levels below runfiles).

Both strategies already know where the runfiles directory exists -
implement _GetRunfilesDir() on the _DirectoryBased strategy, then call
_GetRunfilesDir() in order to populate self._python_runfiles_dir.

Stop passing a bogus path to runfiles.Create() in
testCurrentRepository(),
such that the test actually uses the appropriate runfiles path.

Fixes #3085

---------

Co-authored-by: Ignas Anikevicius <[email protected]>

Author: jsing-canva

CHANGELOG.md

@@ -42,6 +42,10 @@ BEGIN_UNRELEASED_TEMPLATE
 ### Fixed
 * (gazelle) Remove {obj}`py_binary` targets with invalid `srcs`. This includes files
   that are not generated or regular files.
+* (runfiles) Fix incorrect Python runfiles path assumption - the existing
+  implementation assumes that it is always four levels below the runfiles
+  directory, leading to incorrect path checks
+  ([#3085](https://github.com/bazel-contrib/rules_python/issues/3085)).
 
 {#v0-0-0-added}
 ### Added

python/runfiles/runfiles.py

@@ -229,6 +229,9 @@ def RlocationChecked(self, path: str) -> str:
         # runfiles strategy on those platforms.
         return posixpath.join(self._runfiles_root, path)
 
+    def _GetRunfilesDir(self) -> str:
+        return self._runfiles_root
+
     def EnvVars(self) -> Dict[str, str]:
         return {
             "RUNFILES_DIR": self._runfiles_root,
@@ -246,7 +249,7 @@ class Runfiles:
 
     def __init__(self, strategy: Union[_ManifestBased, _DirectoryBased]) -> None:
         self._strategy = strategy
-        self._python_runfiles_root = _FindPythonRunfilesRoot()
+        self._python_runfiles_root = strategy._GetRunfilesDir()
         self._repo_mapping = _RepositoryMapping.create_from_file(
             strategy.RlocationChecked("_repo_mapping")
         )
@@ -469,18 +472,6 @@ def Create(env: Optional[Dict[str, str]] = None) -> Optional["Runfiles"]:
 _Runfiles = Runfiles
 
 
-def _FindPythonRunfilesRoot() -> str:
-    """Finds the root of the Python runfiles tree."""
-    root = __file__
-    # Walk up our own runfiles path to the root of the runfiles tree from which
-    # the current file is being run. This path coincides with what the Bazel
-    # Python stub sets up as sys.path[0]. Since that entry can be changed at
-    # runtime, we rederive it here.
-    for _ in range("rules_python/python/runfiles/runfiles.py".count("/") + 1):
-        root = os.path.dirname(root)
-    return root
-
-
 def CreateManifestBased(manifest_path: str) -> Runfiles:
     return Runfiles.CreateManifestBased(manifest_path)
 

tests/runfiles/BUILD.bazel

@@ -5,6 +5,9 @@ load("@rules_python//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED")  # bu
 py_test(
     name = "runfiles_test",
     srcs = ["runfiles_test.py"],
+    data = [
+        "//tests/support:current_build_settings",
+    ],
     env = {
         "BZLMOD_ENABLED": "1" if BZLMOD_ENABLED else "0",
     },

tests/runfiles/runfiles_test.py

@@ -12,7 +12,9 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import json
 import os
+import pathlib
 import tempfile
 import unittest
 from typing import Any, List, Optional
@@ -63,6 +65,16 @@ def testRlocationArgumentValidation(self) -> None:
             lambda: r.Rlocation("\\foo"),
         )
 
+    def testRlocationWithData(self) -> None:
+        r = runfiles.Create()
+        assert r is not None  # mypy doesn't understand the unittest api.
+        settings_path = r.Rlocation(
+            "rules_python/tests/support/current_build_settings.json"
+        )
+        assert settings_path is not None
+        settings = json.loads(pathlib.Path(settings_path).read_text())
+        self.assertIn("bootstrap_impl", settings)
+
     def testCreatesManifestBasedRunfiles(self) -> None:
         with _MockFile(contents=["a/b c/d"]) as mf:
             r = runfiles.Create(
@@ -692,7 +704,7 @@ def testCurrentRepository(self) -> None:
             expected = ""
         else:
             expected = "rules_python"
-        r = runfiles.Create({"RUNFILES_DIR": "whatever"})
+        r = runfiles.Create()
         assert r is not None  # mypy doesn't understand the unittest api.
         self.assertEqual(r.CurrentRepository(), expected)
 

tests/runtime_env_toolchain/toolchain_runs_test.py

@@ -10,10 +10,20 @@
 class RunTest(unittest.TestCase):
     def test_ran(self):
         rf = runfiles.Create()
-        settings_path = rf.Rlocation(
-            "rules_python/tests/support/current_build_settings.json"
-        )
+        try:
+            settings_path = rf.Rlocation(
+                "rules_python/tests/support/current_build_settings.json"
+            )
+        except ValueError as e:
+            # The current toolchain being used has a buggy zip file bootstrap, which
+            # leaves RUNFILES_DIR pointing at the first stage path and not the module
+            # path.
+            if platform.system() != "Windows" or "does not lie under the runfiles root" not in str(e):
+                raise e
+            settings_path = "./tests/support/current_build_settings.json"
+
         settings = json.loads(pathlib.Path(settings_path).read_text())
+
         if platform.system() == "Windows":
             self.assertEqual(
                 "/_magic_pyruntime_sentinel_do_not_use", settings["interpreter_path"]