From fa3209a19e3dcc2bf5dd04d6b301363125eab21d Mon Sep 17 00:00:00 2001 From: Joel Sing Date: Mon, 14 Jul 2025 14:15:41 +1000 Subject: [PATCH 1/2] fix(runfiles): correct Python runfiles path assumption 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 --- CHANGELOG.md | 4 ++++ python/runfiles/runfiles.py | 17 ++++------------- tests/runfiles/runfiles_test.py | 2 +- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f02c8bbb4..2da1640ea3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,10 @@ END_UNRELEASED_TEMPLATE ({gh-issue}`3043`). * (pypi) The pipstar `defaults` configuration now supports any custom platform name. +* (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 diff --git a/python/runfiles/runfiles.py b/python/runfiles/runfiles.py index 3943be5646..e9ec2f1e87 100644 --- a/python/runfiles/runfiles.py +++ b/python/runfiles/runfiles.py @@ -112,6 +112,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, @@ -129,7 +132,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 = _ParseRepoMapping( strategy.RlocationChecked("_repo_mapping") ) @@ -347,18 +350,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 _ParseRepoMapping(repo_mapping_path: Optional[str]) -> Dict[Tuple[str, str], str]: """Parses the repository mapping manifest.""" # If the repository mapping file can't be found, that is not an error: We diff --git a/tests/runfiles/runfiles_test.py b/tests/runfiles/runfiles_test.py index a3837ac842..efe00940fc 100644 --- a/tests/runfiles/runfiles_test.py +++ b/tests/runfiles/runfiles_test.py @@ -532,7 +532,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) From 11f8af9482fc8ae218840c274ccb3489706d6678 Mon Sep 17 00:00:00 2001 From: Joel Sing Date: Wed, 16 Jul 2025 12:19:29 +1000 Subject: [PATCH 2/2] Prioritise zip file runfiles case See if this fixes Windows CI --- python/private/python_bootstrap_template.txt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/private/python_bootstrap_template.txt b/python/private/python_bootstrap_template.txt index a979fd4422..f5ff469610 100644 --- a/python/private/python_bootstrap_template.txt +++ b/python/private/python_bootstrap_template.txt @@ -256,6 +256,10 @@ def RunfilesEnvvar(module_space): 'RUNFILES_MANIFEST_FILE' and var_value is the path to that directory or file, or (None, None) if runfiles couldn't be found. """ + # If running from a zip, there's no manifest file. + if IsRunningFromZip(): + return ('RUNFILES_DIR', module_space) + # If this binary is the data-dependency of another one, the other sets # RUNFILES_MANIFEST_FILE or RUNFILES_DIR for our sake. runfiles = os.environ.get('RUNFILES_MANIFEST_FILE', None) @@ -266,10 +270,6 @@ def RunfilesEnvvar(module_space): if runfiles: return ('RUNFILES_DIR', runfiles) - # If running from a zip, there's no manifest file. - if IsRunningFromZip(): - return ('RUNFILES_DIR', module_space) - # Look for the runfiles "output" manifest, argv[0] + ".runfiles_manifest" runfiles = module_space + '_manifest' if os.path.exists(runfiles):