Skip to content
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

gh-128779: Fix site venv() for system site-packages #129184

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 22, 2025

Add sys.base_exec_prefix to prefixes if pyvenv.cfg enables system site-packages.

Add sys.base_exec_prefix to prefixes if pyvenv.cfg enables system
site-packages.
@vstinner
Copy link
Member Author

cc: @FFY00

@befeleme
Copy link
Contributor

I can confirm this seems to fix the reported issue.

Lib/site.py Outdated
Comment on lines 638 to 644
if system_site == "true":
PREFIXES.insert(0, sys.prefix)
if sys.base_exec_prefix != sys.exec_prefix:
PREFIXES.append(sys.base_exec_prefix)
else:
PREFIXES = [sys.prefix]
ENABLE_USER_SITE = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since sys.prefix is now set to point to the virtual environment before this code runs, adding sys.prefix here is now incorrect, we want to add sys.base_prefix and sys.base_exec_prefix, and because the existing directories in PREFIXES are now already the venv, we want to append the base prefixes instead.

Additionally, resetting PREFIXES when system_site is false is now unnecessary, because it already has the correct value.

Suggested change
if system_site == "true":
PREFIXES.insert(0, sys.prefix)
if sys.base_exec_prefix != sys.exec_prefix:
PREFIXES.append(sys.base_exec_prefix)
else:
PREFIXES = [sys.prefix]
ENABLE_USER_SITE = False
if system_site == "true":
PREFIXES += [sys.base_prefix, sys.base_exec_prefix]
else:
ENABLE_USER_SITE = False

The comment above can also be removed, or moved to getsitepackages, as there's no possibility of us adding a duplicate prefix now. PREFIXES will already have a duplicated prefix from its initialization if we are running on a virtual environment.


Background into this:

GH-126987 moved the venv initialization logic to getpath, the code that initializes the prefixes and sys.path. Previously, that was done here in site, which was somewhat problematic, but you can read about that in the isssue.

So, before that change, when site, initialized PREFIXES on import, it would effectively be set to sys.base_prefix and sys.base_exec_prefix, which at that point would always have the same value as sys.prefix and sys.exec_prefix. 1

PREFIXES = [sys.prefix, sys.exec_prefix]

Then, here in venv(), if we detect a virtual environment, we would set sys.prefix and sys.exec_prefix to site_prefix in the snippet above that now issues warnings if we detect a mismatch (link to the change in GH-126987).

- 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)

After that, we either reset PREFIXES to [sys.prefix], if system_site is false, or append sys.prefix, which had now been updated to site_prefix. We only do this for sys.prefix, and ignore sys.exec_prefix, because in virtual environments there's no distinction, they always have the same value.

Considering the getpath changes, with PREFIXES getting set to [sys.prefix, sys.exec_prefix] on import, it already includes the virtual environment prefix, so all we need to do here now is to append sys.base_prefix and sys.base_exec_prefix when system_site is true.

Footnotes

  1. Why it was not explicitly initialized to the base prefixes, which should be the intent, I am not sure, but I guess that's an implementation detail that did not really matter at the time.

@bedevere-app
Copy link

bedevere-app bot commented Jan 24, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@FFY00
Copy link
Member

FFY00 commented Jan 24, 2025

I'm writing this here as a reminder for me later.

  • It would be nice to add a test for sys.path, including its order, in test_site
  • We should update the site docstring to reflect the new changes

@vstinner
Copy link
Member Author

@FFY00: I applied your suggestion, please review the updated PR.

@vstinner
Copy link
Member Author

It would be nice to add a test for sys.path, including its order, in test_site

I tried but failed to write such test so far :-(

Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Victor!

I'll have a look to see if I can write a test for this.

@FFY00 FFY00 merged commit a549f43 into python:main Jan 30, 2025
40 of 41 checks passed
@FFY00
Copy link
Member

FFY00 commented Jan 30, 2025

I added a test in GH-129462.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants