fix: support runfiles.CurrentRepository with non-hermetic sys.path
#1634
Conversation
|
@rickeylev Not sure what the CI failures are about. |
python/runfiles/runfiles.py
Outdated
| # TODO: This doesn't cover the case of a script being run from an | ||
| # external repository, which could be heuristically detected | ||
| # by parsing the script's path. | ||
| if sys.path[0] != self._python_runfiles_root: |
There was a problem hiding this comment.
What about checking that we are running on python 3.10 and older and applying this behaviour only then? That may be easier to understand later that we can remove it once the lowest supported Python version is 3.11 (which will probably be a while), but it would also give validation errors on Python 3.11 and above.
There was a problem hiding this comment.
I added a minor version check. Why would we see a validation error on Python 3.11?
There was a problem hiding this comment.
I think with python 3.11, the hermeticity is improved and sys.path entries within the script are better isolated from the the system python. The comment in the code mentions that this affects only 3.10 and below, so I thought that this should not change anything for 3.11 and above, or should it?
There was a problem hiding this comment.
It shouldn't, I was just referring to your comment, which said "but it would also give validation errors on Python 3.11 and above.". I wasn't sure what that was about.
There was a problem hiding this comment.
Just wanted to note that the sys.path isolation is technically an opt-in behavior, not intrinsic to Python 3.11. rules_python's bootstrap sets PYTHONSAFEPATH, so it opts in.
|
The CI should be happy again if you rebase. We had some issues when this PR was created, if I remember correctly. |
fmeum
force-pushed
the
1631-fix-runfiles-lookup
branch
from
4bf451b
to
175687c
Compare
|
re: just assume the file is in the main repo if its loaded from outside the runfiles Yeah, that's probably fine. This is to support older Python versions that were already relying on non-hermetic behavior. I think this problem would only happen for files that are in the same directory (or a subdirectory of) as the main .py file? I don't see how an import would be able to find such files otherwise. So I suppose the |
When using Python 3.10 or earlier, the first `sys.path` entry is the directory containing the script. This can result in modules being loaded from the source root rather than the runfiles root, which the runfiles library previously didn't support.
fmeum
force-pushed
the
1631-fix-runfiles-lookup
branch
from
175687c
to
7adc315
Compare
There was a problem hiding this comment.
I think this got lost, @rickeylev, any extra comments you have here?
|
Any updates on this PR? I think I'm hitting the same issue |
|
I merged |
|
The problem indeed does only surface in edge-cases where:
But still I think this looks like the best solution for now, so it would be great to get this integrated if possible. Gentle ping @rickeylev |
…o 1631-fix-runfiles-lookup
When using Python 3.10 or earlier, the first
sys.pathentry is the directory containing the script. This can result in modules being loaded from the source root rather than the runfiles root, which the runfiles library previously didn't support.Fixes #1631