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

bpo-45413: Define "posix_venv", "nt_venv" and "venv" sysconfig installation schemes #31034

Merged
merged 20 commits into from
Mar 18, 2022

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Jan 31, 2022

Define posix_venv and nt_venv sysconfig installation schemes
to be used for bootstrapping new virtual environments.
Add venv sysconfig installation scheme to get the appropriate one of the above.
The schemes are identical to the pre-existing
posix_prefix and nt install schemes.
The venv module now uses the venv scheme to create new virtual environments
instead of hardcoding the paths depending only on the platform. Downstream
Python distributors customizing the posix_prefix or nt install
scheme in a way that is not compatible with the install scheme used in
virtual environments are encouraged not to customize the venv schemes.
When Python itself runs in a virtual environment,
sysconfig.get_default_scheme and
sysconfig.get_preferred_scheme with key="prefix" returns
venv.

https://bugs.python.org/issue45413

The scheme is used for bootstrapping new virtual environments.
It is a copy of the "posix_prefix" or "nt" install scheme depending on
the platform.
The venv module now uses that scheme to create new virtual environments
instead of hardcoding the paths depending only on the platform.
Downstream Python distributors customizing the "posix_prefix" or
"nt" install scheme in a way that is not compatible with the install scheme
used in virtual environments are encouraged to supply a modified working
"venv" scheme.
Lib/sysconfig.py Outdated Show resolved Hide resolved
@hroncok hroncok marked this pull request as ready for review February 1, 2022 19:32
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.

Thank you for looking into this! Though, I do have some concerns about if the approach taken was the correct one.

It is a copy of the "posix_prefix" or "nt" install scheme depending on the platform.

I think this is a bit messy, and not very nice for long term maintainability. There is also the existing issue of distributors patching the posix_prefix scheme, which would mess this up, and while that is not officially supported, it would be best to try to preemptively mitigate any issues there, as long as it is a reasonable option for CPython.

When I opened the issue, I envisioned adding a completely new scheme, and making the interpreter select those paths when we are in a virtual environment. We already have code to detect this (hint: pyvenv.cfg), so it should just be a matter of hard-coding the paths there.

This would also make the scheme be the same across operating systems, which is fairly neat IMO. prefix would be the virtual environment path, and everything builds from there.

That said, I would like both @jaraco and @zooba to take a look, if they are available. If not, I think it would be best to delay merging this for a bit, but I am not a core dev, so my opinion doesn't matter much there, if anyone is confident enough with it, go ahead and merge I guess 😅

Lib/venv/__init__.py Outdated Show resolved Hide resolved
@hroncok
Copy link
Contributor Author

hroncok commented Feb 1, 2022

We can certainly make get_prefered_scheme return "venv" in venvs. I don't know if it needs to be done as part of this change.

@FFY00
Copy link
Member

FFY00 commented Feb 1, 2022

We can certainly make get_prefered_scheme return "venv" in venvs. I don't know if it needs to be done as part of this change.

Oh yeah, that is something I overlook in this preliminary review. I think we should definitely do that.

@hroncok
Copy link
Contributor Author

hroncok commented Feb 1, 2022

We can certainly make get_prefered_scheme return "venv" in venvs. I don't know if it needs to be done as part of this change.

Oh yeah, that is something I overlook in this preliminary review. I think we should definitely do that.

OK, this is now done. But I am not sure if this is what I wanted :D

Lib/sysconfig.py Outdated Show resolved Hide resolved
@FFY00
Copy link
Member

FFY00 commented Feb 1, 2022

Thank you!

@encukou
Copy link
Member

encukou commented Mar 16, 2022

Who else do should comment here before you can send the PR?

@merwok
Copy link
Member

merwok commented Mar 16, 2022

This needs a fix, so I'll merge the PR as it is.

This is confusing: why run to merge something before it’s complete?

@encukou
Copy link
Member

encukou commented Mar 16, 2022

This PR is complete now. There is discussion about how it can be made better, and that discussion is not really moving forward. Everyone is waiting for something while a fix is ready.
And the solution to this issue should get as much practical testing as it can before the API is locked in by 3.11.0 beta. At the pace it's been going, it won't get in at all.

@merwok
Copy link
Member

merwok commented Mar 16, 2022

Ah, I thought fix was referring to fixing an issue in this PR, but I see you mean fix for the ticket, i.e. the whole PR.
Didn’t understand that at first because I see this as a new feature!

@encukou
Copy link
Member

encukou commented Mar 16, 2022

Right, my wording was ambiguous.
It's a new feature for CPython, but it's an issue in software affected by the changes surrounding distutils deprecation/removal.

@FFY00
Copy link
Member

FFY00 commented Mar 16, 2022

I think it is safe to say that the current state of the PR isn't clear to be the best approach. I don't understand the eagerness to merge this right now, I think that can do more harm than good.

The issue this PR solves already exists in the current released version, 3.10. We will not be able to fix it in that version, that ship has sailed. To work around this, we have already designed an approach to deal with this issue, which was adopted by the impacted parties (see pypa/virtualenv#2208).

This PR, which would land in 3.11, is incompatible with the adopted workaround, requiring more changes to the downstreams, and introducing more complexity in version compatibility code. If we merge it as-is and it gets released, we put more weight on the downstreams, and even more if it turns out that we need to change the approach later.

So, my proposal would be to drop get_venv_scheme and use a venv scheme directly instead in this PR, preserving compatibility with the downstream workaround.
We can then investigate adopting a new approach, and other needs that might arise. If we decide another approach is needed, we can then adopt it (honestly, I don't think it is), but please lets do not do that with an undereviewed one.

Most of my concerns with the current implementation, other than it being incompatible with the 3.10 workaround, are due to long term maintainability and distributors messing things up with patching, and can be solved just by changing the internal implementation, without breaking compatibility. We can handle them later.

@FFY00
Copy link
Member

FFY00 commented Mar 16, 2022

It's a new feature for CPython, but it's an issue in software affected by the changes surrounding distutils deprecation/removal.

Not really. It is an issue in Python 3.10+ software, period. It was just introduced by changes to support the distutils deprecation, but this affect all Python 3.10 software that is on the business of manually creating virtual environments.

@hroncok
Copy link
Contributor Author

hroncok commented Mar 16, 2022

So, the "venv" scheme would do what the function currently does, and the platform specific schemes would stay? I can do that as well.

@FFY00
Copy link
Member

FFY00 commented Mar 16, 2022

Yes, exactly.

@encukou
Copy link
Member

encukou commented Mar 17, 2022

@zooba, what do you think about that?

@hroncok
Copy link
Contributor Author

hroncok commented Mar 17, 2022

I've committed Drop get_venv_scheme() in favor of venv scheme. It can be reverted if @zooba does not like it.
There was some outdated documentation, so I've changed that in Apply my own suggestions from docs review.

I would squash those together, but I've been told not to push force.

Doc/library/sysconfig.rst Outdated Show resolved Hide resolved
Doc/library/sysconfig.rst Outdated Show resolved Hide resolved
Doc/library/venv.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.11.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.11.rst Outdated Show resolved Hide resolved
Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

Looks good to me. (Apologies for unresponsiveness - GitHub notifications are a bad way to reach me, unfortunately. bpo messages are far more reliable due to no noise.)

Doc/whatsnew/3.11.rst Outdated Show resolved Hide resolved
Co-authored-by: Steve Dower <[email protected]>
@hroncok hroncok changed the title bpo-45413: Define "posix_venv" and "nt_venv" sysconfig installation schemes bpo-45413: Define "posix_venv", "nt_venv" and "venv" sysconfig installation schemes Mar 17, 2022
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, this is significantly better IMO.

We can improve the implementation later if needed.

@encukou encukou merged commit 48d9262 into python:main Mar 18, 2022
@hroncok hroncok deleted the venv_scheme branch March 18, 2022 10:01
@hroncok
Copy link
Contributor Author

hroncok commented Mar 18, 2022

dancing cat

@encukou
Copy link
Member

encukou commented Mar 18, 2022

Thanks for your work and patience, @hroncok and @FFY00!

Steve: OK, I'll keep it in mind. I was just drafting a direct email; hopefully that would work if notifications fail :)

@zooba
Copy link
Member

zooba commented Mar 18, 2022

@encukou I suspect direct emails are going to become important for me once all notifications move to GitHub. I can only see things getting even worse than they are now 😞

hauntsaninja pushed a commit to hauntsaninja/build that referenced this pull request Apr 16, 2022
As a result of python/cpython#31034,
venv now calls sysconfig.get_paths. This messes up the mock call
count.

The change to using Signature.bind isn't strictly necessary, but fixes
the initial symptom (since venv calls sysconfig.get_paths without
kwargs). This also means we can use sysconfig.get_paths however we want.
layday pushed a commit to pypa/build that referenced this pull request Apr 17, 2022
* Fix mock in test_executable_missing_post_creation for Python 3.11

As a result of python/cpython#31034,
venv now calls sysconfig.get_paths. This messes up the mock call
count.

The change to using Signature.bind isn't strictly necessary, but fixes
the initial symptom (since venv calls sysconfig.get_paths without
kwargs). This also means we can use sysconfig.get_paths however we want.

* add comment, unskip on pypy3

* mock venv.EnvBuilder.create

* remove unused imports

Authored-by: hauntsaninja <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants