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-92897: Ensure venv --copies respects source build property of the creating interpreter (GH-92899) #92899

Merged
merged 2 commits into from
Jul 5, 2022

Conversation

jkloth
Copy link
Contributor

@jkloth jkloth commented May 17, 2022

There is a discrepancy between a symlink venv and a copied venv when determining whether the interpreter is considered to be from a source build. A symlink venv indicates True for sysconfig.is_python_build() whereas a copied venv returns False.

Since symlink venvs are the default for POSIX (where the majority of source builds would be used/tested), I have opted to use that behavior as correct. Windows users are far more likely to use the binary installer so odds of venvs being created from source builds are quite low. Likewise with venv --copies on POSIX.

These changes will allow for building extensions out of venvs created from source builds without any pre-configuration steps (see this section from pyperformance documentation).

Adding @zooba for Windows and @vsajip for venv

Verified

This commit was signed with the committer’s verified signature.
kadiwa4 Kalle Wachsmuth
… of the

creating interpreter.
@zooba
Copy link
Member

zooba commented May 17, 2022

Don't touch distutils - it's going to be deleted in this release anyway, and there are no maintenance releases where it is undeprecated.

If we're using it within the stdlib, update the references elsewhere. If setuptools is impacted, they need the patch.

I haven't looked any deeper than that - it's late. I'll try and take a look later this week

@jkloth
Copy link
Contributor Author

jkloth commented May 17, 2022

The changes to distutils are only to keep it from raising exceptions. Since it is being removed, I saw no need to keep private functions around just for use within distutils which would prevent a more straight forward implementation. Hence the modifications to distutils.

The changes to stdlib sysconfig have no impact on setuptools distutils.sysconfig so nothing to do there.

@vstinner
Copy link
Member

The changes to distutils are only to keep it from raising exceptions.

In practice, setuptools now injects its own distutils flavor, so I am not sure that it's worth it to modify it here. At least, you should get your change merged first in https://github.com/pypa/distutils/

@jkloth
Copy link
Contributor Author

jkloth commented May 18, 2022

distutils.sysconfig and distutils.test.test_sysconfig in the stdlib have diveraged from those in setuptools (PR #23142). Therefore the changes included here have ZERO correspondance to those in setuptools. distutils.sysconfig in setuptools does not rely on any private functions from sysconfig so there is no change to merge.

@jkloth
Copy link
Contributor Author

jkloth commented May 18, 2022

To make it absolutely clear, the changes here to distutils modules are solely to prevent an ImportError from occurring wherever distutils modules are still used in the stdlib. The changes are intended to keep it "working" until it can be fully removed at a later date.

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.

LGTM, and appears to handle the case on Windows where a venv is created from a source build. I can only assume the behaviour is correct on other platforms?

@jkloth
Copy link
Contributor Author

jkloth commented May 19, 2022

As far as other platforms go, it is tested on Ubuntu (WSL).

@vsajip vsajip changed the title gh-92897: Ensure venv --copies respects source build property of the creating interpreter gh-92897: Ensure venv --copies respects source build property of the creating interpreter (GH-92899) Jul 5, 2022
@vsajip vsajip merged commit 0675975 into python:main Jul 5, 2022
@vsajip vsajip added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jul 5, 2022
@miss-islington
Copy link
Contributor

Thanks @jkloth for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @jkloth for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Sorry, @jkloth and @vsajip, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 067597522a9002f3b8aff7f46033f10acb2381e4 3.10

@miss-islington
Copy link
Contributor

Sorry @jkloth and @vsajip, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 067597522a9002f3b8aff7f46033f10acb2381e4 3.11

vsajip pushed a commit to vsajip/cpython that referenced this pull request Jul 5, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…roperty of the creating interpreter (pythonGH-92899)

(cherry picked from commit 0675975)

Co-authored-by: Jeremy Kloth <jeremy.kloth@gmail.com>
@bedevere-bot
Copy link

GH-94567 is a backport of this pull request to the 3.11 branch.

vsajip pushed a commit to vsajip/cpython that referenced this pull request Jul 5, 2022
… of the creating interpreter (pythonGH-92899)

(cherry picked from commit 0675975)
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 5, 2022
@bedevere-bot
Copy link

GH-94572 is a backport of this pull request to the 3.10 branch.

vsajip added a commit that referenced this pull request Jul 5, 2022
…y of the creating interpreter (GH-92899) (GH-94567)

(cherry picked from commit 0675975)

Co-authored-by: Jeremy Kloth <jeremy.kloth@gmail.com>
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.

None yet

6 participants