Skip to content

Commit fa3209a

Browse files
committed
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
1 parent 5c68ff9 commit fa3209a

File tree

3 files changed

+9
-14
lines changed

3 files changed

+9
-14
lines changed

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,10 @@ END_UNRELEASED_TEMPLATE
8686
({gh-issue}`3043`).
8787
* (pypi) The pipstar `defaults` configuration now supports any custom platform
8888
name.
89+
* (runfiles) Fix incorrect Python runfiles path assumption - the existing
90+
implementation assumes that it is always four levels below the runfiles
91+
directory, leading to incorrect path checks
92+
([#3085](https://github.com/bazel-contrib/rules_python/issues/3085)).
8993

9094
{#v0-0-0-added}
9195
### Added

python/runfiles/runfiles.py

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,9 @@ def RlocationChecked(self, path: str) -> str:
112112
# runfiles strategy on those platforms.
113113
return posixpath.join(self._runfiles_root, path)
114114

115+
def _GetRunfilesDir(self) -> str:
116+
return self._runfiles_root
117+
115118
def EnvVars(self) -> Dict[str, str]:
116119
return {
117120
"RUNFILES_DIR": self._runfiles_root,
@@ -129,7 +132,7 @@ class Runfiles:
129132

130133
def __init__(self, strategy: Union[_ManifestBased, _DirectoryBased]) -> None:
131134
self._strategy = strategy
132-
self._python_runfiles_root = _FindPythonRunfilesRoot()
135+
self._python_runfiles_root = strategy._GetRunfilesDir()
133136
self._repo_mapping = _ParseRepoMapping(
134137
strategy.RlocationChecked("_repo_mapping")
135138
)
@@ -347,18 +350,6 @@ def Create(env: Optional[Dict[str, str]] = None) -> Optional["Runfiles"]:
347350
_Runfiles = Runfiles
348351

349352

350-
def _FindPythonRunfilesRoot() -> str:
351-
"""Finds the root of the Python runfiles tree."""
352-
root = __file__
353-
# Walk up our own runfiles path to the root of the runfiles tree from which
354-
# the current file is being run. This path coincides with what the Bazel
355-
# Python stub sets up as sys.path[0]. Since that entry can be changed at
356-
# runtime, we rederive it here.
357-
for _ in range("rules_python/python/runfiles/runfiles.py".count("/") + 1):
358-
root = os.path.dirname(root)
359-
return root
360-
361-
362353
def _ParseRepoMapping(repo_mapping_path: Optional[str]) -> Dict[Tuple[str, str], str]:
363354
"""Parses the repository mapping manifest."""
364355
# If the repository mapping file can't be found, that is not an error: We

tests/runfiles/runfiles_test.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,7 @@ def testCurrentRepository(self) -> None:
532532
expected = ""
533533
else:
534534
expected = "rules_python"
535-
r = runfiles.Create({"RUNFILES_DIR": "whatever"})
535+
r = runfiles.Create()
536536
assert r is not None # mypy doesn't understand the unittest api.
537537
self.assertEqual(r.CurrentRepository(), expected)
538538

0 commit comments

Comments
 (0)