GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-92897: Ensure `venv --copies` respects source build property of the creating interpreter (GH-92899) by jkloth · Pull Request #92899 · python/cpython

Conversation

@jkloth

@jkloth jkloth commented May 17, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

  • Issue: Ensure venv works with source builds when using --copies #92897

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jkloth
pythongh-92897: Ensure venv --copies respects source build property…
4b60bf8 
… of the

creating interpreter.
@jkloth
jkloth requested a review from vsajip as a code owner May 17, 2022 20:39
@bedevere-bot bedevere-bot added the awaiting review label May 17, 2022
@jkloth
Updated srcdir filename checks
f608ba9
@zooba

zooba commented May 17, 2022

Copy link
Copy Markdown
Member

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jkloth

jkloth commented May 17, 2022

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request May 18, 2022
Ensure venv works with source builds when using --copies #92897
Closed
@vstinner

vstinner commented May 18, 2022

Copy link
Copy Markdown
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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jkloth

jkloth commented May 18, 2022

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jkloth

jkloth commented May 18, 2022

Copy link
Copy Markdown
Contributor Author

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.
zooba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba
zooba approved these changes May 18, 2022
View reviewed changes

@zooba zooba left a comment

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels May 18, 2022
@jkloth

jkloth commented May 19, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vsajip vsajip added the skip news label May 21, 2022
@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
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 5, 2022
@vsajip vsajip added needs backport to 3.10 needs backport to 3.11 only security fixes labels Jul 5, 2022
@miss-islington

miss-islington commented Jul 5, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Jul 5, 2022

Copy link
Copy Markdown
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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Jul 5, 2022

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned vsajip Jul 5, 2022
@miss-islington

miss-islington commented Jul 5, 2022

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vsajip pushed a commit to vsajip/cpython that referenced this pull request Jul 5, 2022
@jkloth @vsajip
[3.11] pythongh-92897: Ensure venv --copies respects source build p…
124acce 
…roperty of the creating interpreter (pythonGH-92899)

(cherry picked from commit 0675975)

Co-authored-by: Jeremy Kloth <[email protected]>
@vsajip vsajip mentioned this pull request Jul 5, 2022
[3.11] gh-92897: Ensure venv --copies respects source build propert… #94567
Merged
@bedevere-bot

bedevere-bot commented Jul 5, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 5, 2022
@vsajip vsajip mentioned this pull request Jul 5, 2022
Unexpected run state encountered python/cherry-picker#65
Open
vsajip pushed a commit to vsajip/cpython that referenced this pull request Jul 5, 2022
@jkloth @vsajip
pythongh-92897: Ensure venv --copies respects source build property…
5e0626c 
… of the creating interpreter (pythonGH-92899)

(cherry picked from commit 0675975)
@vsajip vsajip mentioned this pull request Jul 5, 2022
[3.10] gh-92897: Ensure venv --copies respects source build property of th… #94572
Closed
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Jul 5, 2022
@bedevere-bot

bedevere-bot commented Jul 5, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vsajip added a commit that referenced this pull request Jul 5, 2022
@vsajip @jkloth
[3.11] gh-92897: Ensure venv --copies respects source build propert…
49aeda9 
…y of the creating interpreter (GH-92899) (GH-94567)

(cherry picked from commit 0675975)

Co-authored-by: Jeremy Kloth <[email protected]>
@FFY00 FFY00 mentioned this pull request Nov 21, 2022
TST: adding marker and stricter warning checks mesonbuild/meson-python#214
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zooba zooba zooba approved these changes
@vsajip vsajip Awaiting requested review from vsajip vsajip is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@vsajip vsajip

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jkloth @zooba @vstinner @miss-islington @bedevere-bot @vsajip
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.