Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 35.3k
GH-126985: move pyvenv.cfg detection from site to getpath#126987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
13809729cc5c44c6b638bfa3adbac5b7d6377aeff4892876abd506183c1fb293c24f44f98ec673bfb0737e776b7caf5984File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -94,6 +94,12 @@ def _trace(message): | ||
| print(message, file=sys.stderr) | ||
| def _warn(*args, **kwargs): | ||
| import warnings | ||
| warnings.warn(*args, **kwargs) | ||
| def makepath(*paths): | ||
| dir = os.path.join(*paths) | ||
| try: | ||
| @@ -602,7 +608,10 @@ def venv(known_paths): | ||
| elif key == 'home': | ||
| sys._home = value | ||
| sys.prefix = sys.exec_prefix = site_prefix | ||
| if sys.prefix != site_prefix: | ||
| _warn(f'Unexpected value in sys.prefix, expected {site_prefix}, got {sys.prefix}', RuntimeWarning) | ||
| if sys.exec_prefix != site_prefix: | ||
| _warn(f'Unexpected value in sys.exec_prefix, expected {site_prefix}, got {sys.exec_prefix}', RuntimeWarning) | ||
Comment on lines
+611
to
+614
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These warnings are also worth watching out for, though hopefully they only ever indicate actual problems. If the values don't match, should we update them anyway like we used to? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! We could run this through a deprecation period, setting the value for now and stopping in two versions, though, I think triggering these warnings likely highlights a bug in user code and giving how niche use-cases like these are, I am not sure if that's needed, and it's extra overhead on our part. I'm inclined to keep the code as-is, and if we see any issues during the pre-release phase, change to the deprecation approach before a final release. @hugovk do you have any preference as the RM? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No preference as RM. You could have the warnings for one release, then based on feedback (if any!) decide if you want to deprecate/change behaviour. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's go with the warnings and re-evaluate based on feedback, then. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change makes I found this because I am digging into a different problem related to resolving executable symlinks when creating venvs (#106045). IMO, the logic for determining what the path should be belongs in one place ( Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess you are talking about the case where you have a venv based on copy, not on symlinks? Because for the latter, I think we have all we need. I'm glad that @FFY00 pointed me at #127895, as I find
I think this should be documented, as it may be something that people have designed around. For example, there is a comment in
This comment is now outdated (it isn't quiet 😂) Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, like @pelson says, pyvenv.cfg having home wasn't actually necessary when symlinks were used. The logic would follow the symlink and treat that as home (or more specifically, use it as a potential candidate). Dereferencing the symlink to find home seems pretty intentional? There's various calls to realpath() that inform later logic. Similarly the top-of-file comments describe this behavior (i.e. it was intentional prior to this change).
For rules_python, this becomes problematic because it creates a conundrum:
The net effect is there isn't any content we can write to pyvenv.cfg that allows Python to find its home and recognize the venv. | ||
| # Doing this here ensures venv takes precedence over user-site | ||
| addsitepackages(known_paths, [sys.prefix]) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to, we should be able to make them affected by the option again. This seems like the most likely change to impact people, so it's worth being aware of how we might cleanly roll it back without undoing all the work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this should be relatively easy, as
PyConfigalready has asite_importfield, which we can use to check to change the behavior.I think the specifics of a possible rollback would depend on the reported issues. I would like to still keep the new behavior long term, if there are any issues, I think we should consider rolling back with a deprecation period, but depending on how critical and in what way the new behavior is problematic, we may just want to roll back permanently.