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

Make pipenv work with the venv install scheme if it is detected #5096

Merged
merged 2 commits into from
May 26, 2022

Conversation

torsava
Copy link
Contributor

@torsava torsava commented May 6, 2022

The issue

Python 3.10 introduced sysconfig._get_preferred_schemes[1], a mechanism to allow downstream distributors to overwrite the default install scheme.
This is done to support downstream modifications where distributors change the installation layout (eg. different site-packages directory). So, distributors will change the default scheme to one that correctly represents their layout.

This presents an issue for projects/people that need to bootstrap virtual environments, like virtualenv (see [2]) and pipenv too. As distributors might now be customizing the default install scheme, there is no guarantee that the information returned by sysconfig.get_default_scheme/get_paths is correct for the virtual environment, the only guarantee we have is that it is correct for the current environment. When bootstrapping a virtual environment, we need to know its layout, so that we can place the files in the correct locations.

[1] https://docs.python.org/3/library/sysconfig.html#sysconfig._get_preferred_schemes
[2] pypa/virtualenv#2208

The fix

To solve this issue, CPython upstream added in issue45413 [3] a new "venv" install scheme, for virtual environments. And this new install scheme has already been implemented in virtualenv [4] and the Python build frontend [5]. This PR is similar, it first checks if “venv” is among the available install schemes and uses it if it is. Otherwise it fallbacks to the standard nt or posix_prefix install schemes as before. This should produce no change for environments where default schemes were not changed.

[3] https://bugs.python.org/issue45413
[4] pypa/virtualenv#2209
[5] pypa/build#434

I’d like to deploy this fix in Fedora [6] and test it there, because Fedora changes the default install schemes and therefore pipenv fails there. But first I wanted to ask for feedback, if you see any obvious issues with the change. If not we’ll test it downstream, and then I’d like to ask you to merge it here as well.

[6] https://src.fedoraproject.org/rpms/pipenv/pull-request/53

The checklist

@oz123 oz123 requested a review from matteius May 12, 2022 18:54
@matteius
Copy link
Member

@torsava this looks good -- can you add a news fragment please?

@torsava
Copy link
Contributor Author

torsava commented May 26, 2022

@torsava this looks good -- can you add a news fragment please?

Done, please take a look.

@matteius
Copy link
Member

@torsava this looks good -- can you add a news fragment please?

Done, please take a look.

@torsava Looks pretty good but fails the lint check due to file name of news fragment. If you rename it 5096.bugfix.rst then I can merge more easily.

NEWS fragment files must be named *.(feature|behavior|bugfix|vendor|doc|trivial|removal|process).rst
``

@torsava
Copy link
Contributor Author

torsava commented May 26, 2022

@torsava this looks good -- can you add a news fragment please?

Done, please take a look.

@torsava Looks pretty good but fails the lint check due to file name of news fragment. If you rename it 5096.bugfix.rst then I can merge more easily.

NEWS fragment files must be named *.(feature|behavior|bugfix|vendor|doc|trivial|removal|process).rst
``

Already fixed. :)

EDIT: I accidentally fixed it wrong. Now it should work.

@torsava
Copy link
Contributor Author

torsava commented May 26, 2022

(Btw, the .rst bit is missing from the PR template, that's why I didn't add it at first.)

@matteius
Copy link
Member

I am not sure what's going on here because it doesn't show an option to kick off the next job run and when I re-run the prior job it doesn't pick up the new change and re-fails the lint check.

@matteius
Copy link
Member

@torsava Can you also delete this line so that the CI will run again? https://github.com/pypa/pipenv/blob/main/.github/workflows/ci.yaml#L11

@torsava
Copy link
Contributor Author

torsava commented May 26, 2022

@torsava Can you also delete this line so that the CI will run again? https://github.com/pypa/pipenv/blob/main/.github/workflows/ci.yaml#L11

Done, let's see.

@matteius
Copy link
Member

@torsava Thank you for your responsiveness, this is how it goes sometimes with pipenv. Your news fragment any backquotes need to be double because that is rst standard, I learned this a few months back when I added linting. Example: not `venv` but ``venv``

@torsava
Copy link
Contributor Author

torsava commented May 26, 2022

@torsava Thank you for your responsiveness, this is how it goes sometimes with pipenv. Your news fragment any backquotes need to be double because that is rst standard, I learned this a few months back when I added linting. Example: not venv but venv

Ah, of course, no problem. Fixed.

@matteius matteius merged commit c97ffc1 into pypa:main May 26, 2022
@torsava
Copy link
Contributor Author

torsava commented May 27, 2022

Thanks, @matteius !

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.

3 participants