The Wayback Machine - https://web.archive.org/web/20230117140126/https://github.com/python/cpython/pull/101039
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

Jump to bottom

gh-95299: Stop installing setuptools as a part of ensurepip and venv #101039

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

gh-95299: Stop installing setuptools as a part of ensurepip and venv #101039

wants to merge 4 commits into from
+43 −56

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Jan 14, 2023 •
edited

This PR removes the bundled setuptools wheel from ensurepip, and stops installing setuptools in environments created by venv.

I based this PR off of my understanding of venv and ensurepip, and using the following search to locate all the relevant bits of code + ensuring that tests are happy locally.

Screenshot 2023-01-14 at 17 20 01

In this PR, I've intentionally not touched the example in https://docs.python.org/3.12/library/venv.html#an-example-of-extending-envbuilder. That example is largely outdated and is likely better served being updated in a dedicated PR for it.

@pradyunsg pradyunsg added stdlib Python modules in the Lib dir 3.12 labels Jan 14, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jan 14, 2023
Open
@pradyunsg
Copy link
Member Author

pradyunsg commented Jan 14, 2023 •
edited

Hmm... @vstinner @encukou any opinions on what I should do with test_cppext (added in #91321) here?

That test has an implicit assumption that setuptools will be present in the virtual environment created by venv (added in #92639, as part of removing reliance on distutils).

Notably, some leading questions that affect what direction I could take for updating that test:

  • Does this test need to use packaging tooling? (otherwise, I guess we can hand-craft building the extension and rely on that)
  • If it should use setuptools, would it be OK for this test to require reaching out over the network to fetch setuptools via pip?
  • Would it be OK to build the extension in pip-installable package, rather than using the deprecated approach of directly invoking setup.py?

@pradyunsg
Use environment variables to pass data into test setup script
6aef7f9 
This avoids meddling with `sys.argv` and removes reliance on careful
control over the arguments passed to the script.
@pradyunsg pradyunsg force-pushed the remove-setuptools-ensurepip branch from 17a121e to 32b3ccc Compare Jan 14, 2023
@pradyunsg
Copy link
Member Author

pradyunsg commented Jan 14, 2023 •
edited

Since it's cheap to revert commits/remove them from a PR, and nice to have a code change to discuss details over, I've gone ahead and pushed a change that assumes that the answer to the questions I raised above is yes, yes and yes. :)

Mentioning a GitHub UI thing that I find very useful (and non-initutive): you can click "force-pushed" on the event above to see what changed in the force pushes between the hashes that GitHub lists in the from ... to ... at the end of the event.

pradyunsg added 2 commits Jan 14, 2023
@pradyunsg
Update test_cppext to stop invoking setuptools-based script as a CLI
82bf244 
This allows the "package" to be built using regular Python packaging
workflows instead of relying on `setuptools` being present in the
virtual environment.
@pradyunsg
Stop installing setuptools as a part of ensurepip and venv
6939d48 
This is no longer necessary since newer versions of pip are able to
operate in environments that do not contain setuptools.
@pradyunsg pradyunsg force-pushed the remove-setuptools-ensurepip branch from 32b3ccc to 6939d48 Compare Jan 14, 2023
vsajip
vsajip approved these changes Jan 15, 2023
View changes
Copy link
Member

@vsajip vsajip left a comment

Choose a reason for hiding this comment

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

The changes seem reasonable to me, though I'm only the code owner for the venv and test_venv bits.

@encukou
Copy link
Member

encukou commented Jan 16, 2023

I guess we can hand-craft building the extension and rely on that

IMO, that would be best. Relatedly, I've been thinking how to handle distutils removal in the extension tutorial -- it should definitely mention setuptools, but a simple but working example command line, because setuptools is not always available.
Having that in the tests -- even in a way that only satisfies our CI on select platforms -- would help :)
CPython tests depending on setuptools isn't great in general, and neither is downloading code as part of the test run.

Anyway, if that sounds like more than you signed up for, go ahead with Setuptools.
I don't have time for a full PR review now, sorry! Whoever gets around to it, please test-with-buildbots before merging.

@pradyunsg
Copy link
Member Author

Anyway, if that sounds like more than you signed up for, go ahead with Setuptools.

I spent some time today looking into this after my work day, and I've not figured out the details in a portable manner. I'd prefer if this could be tackled in a follow up instead. :)

@pradyunsg
Copy link
Member Author

/cc @dstufft @ncoghlan as other experts on ensurepip.

I'm technically listed as an expert on ensurepip already and I'm ~sure that the changes I've made to it are correct; but it certainly won't hurt if y'all can review this. :)

@pradyunsg pradyunsg mentioned this pull request Jan 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk

@vsajip vsajip

@ezio-melotti ezio-melotti

Assignees
No one assigned
Labels
3.12 awaiting merge stdlib Python modules in the Lib dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pradyunsg @encukou @vsajip @hugovk @bedevere-bot