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

Merged
merged 21 commits into from Apr 18, 2023
Merged

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

merged 21 commits into from Apr 18, 2023

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 new features, bugs and security fixes 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
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.

vsajip
vsajip approved these changes Jan 15, 2023
View reviewed 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
@jherland jherland mentioned this pull request Jan 24, 2023
Closed
@pradyunsg pradyunsg added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 31, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pradyunsg for commit 7457fd3 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 31, 2023
pfmoore
pfmoore reviewed Jan 31, 2023
View reviewed changes
Lib/test/test_cppext.py Outdated Show resolved Hide resolved
@pfmoore
Copy link
Member

pfmoore commented Jan 31, 2023

I added a review comment that should fix the buildbot errors.

merwok
merwok reviewed Jan 31, 2023
View reviewed changes
Lib/test/test_cppext.py Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Feb 2, 2023

Should this also go in What's New, under Removed?

@pfmoore
Copy link
Member

pfmoore commented Feb 2, 2023

Should this also go in What's New, under Removed?

The inclusion of setuptools was always explicitly documented as an internal implementation detail, and as such, I don't think we have any obligation to report its removal. It could be mentioned in the same way that we might document the removal of an internal API - I did a quick check and the 3.11 What's New included "Removed the undocumented private float.__set_format__() method". So I guess there's a precedent.

@encukou
Copy link
Member

encukou commented Feb 3, 2023

Please add it as a convenience to users, even if there's technically no obligation. It's been very easy to end up depending on setuptools accidentally.

IMO, for removals/breaks, it's best to write What's New entries for the target audience of a poor soul who isn't too well-versed in Python but inherited some complex system with a “temporary” hack. Point them to a solution, don't apologize, but don't berate them either.
With that in mind, please mention pkg_resources, as that shows up in ImportErrors but the connection to setuptools may not be clear.

(cc @python/proofreaders -- hope the above is good general advice)

merwok
merwok reviewed Apr 4, 2023
View reviewed changes
Doc/using/venv-create.inc Outdated Show resolved Hide resolved
@pradyunsg
Use the style suggested in review
ea1bcea
@pradyunsg
Use dirname on the absolute path
5275f93
@pradyunsg
Locate cppextdata with plain filesystem lookups
5c3ce99 
Doing anything more elaborate was flaky on certain buildbots, and
hopefully this is good enough.
@pradyunsg pradyunsg added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section topic-ensurepip labels Apr 4, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pradyunsg for commit 5c3ce99 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 4, 2023
CAM-Gerlach
CAM-Gerlach reviewed Apr 4, 2023
View reviewed changes
Lib/test/test_cppext.py Outdated Show resolved Hide resolved
vstinner
vstinner reviewed Apr 6, 2023
View reviewed changes
Lib/test/test_cppext.py Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

vstinner commented Apr 6, 2023

Since this change is tricky, would you mind to consider extracting the test_cppext changes into a separated PR, to start with that?

@pradyunsg pradyunsg mentioned this pull request Apr 6, 2023
Merged
@pradyunsg
Copy link
Member Author

I've filed #103316, pulling out the test_cppext changes into that PR.

@pradyunsg
Copy link
Member Author

OK, and now I've pulled in changes in the changes from that PR and addressed all outstanding concerns. Assuming the CI and buildbots are happy, this should be good to go.

@pradyunsg pradyunsg mentioned this pull request Apr 17, 2023
Open
1 task
@pradyunsg pradyunsg added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 17, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pradyunsg for commit b83cf9f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 17, 2023
@pradyunsg
Copy link
Member Author

All the buildbot failures seem like false negatives, with test_asyncio, test_eintr, test_toolsaand test_typing failing for unrelated reasons.

@pradyunsg
Copy link
Member Author

Given that this has multiple reviews, that the CI and buildbots are happy (all the non-flaky ones), that all outstanding comments are addressed and the multiple approvals at various points, I'm going to go ahead and merge this. If there's more needed here, we can handle that in a follow up!

@pradyunsg pradyunsg merged commit ece20db into python:main Apr 18, 2023
94 of 99 checks passed
@pradyunsg pradyunsg deleted the remove-setuptools-ensurepip branch April 18, 2023 04:43
@pradyunsg
Copy link
Member Author

Thanks all! Holler on the issue, if there's any concerns! :)

carljm added a commit to carljm/cpython that referenced this pull request Apr 20, 2023
@carljm
Merge branch 'main' into superopt
5c0a21c 
* main: (24 commits)
  pythongh-98040: Move the Single-Phase Init Tests Out of test_imp (pythongh-102561)
  pythongh-83861: Fix datetime.astimezone() method (pythonGH-101545)
  pythongh-102856: Clean some of the PEP 701 tokenizer implementation (python#103634)
  pythongh-102856: Skip test_mismatched_parens in WASI builds (python#103633)
  pythongh-102856: Initial implementation of PEP 701 (python#102855)
  pythongh-103583: Add ref. dependency between multibytecodec modules (python#103589)
  pythongh-83004: Harden msvcrt further (python#103420)
  pythonGH-88342: clarify that `asyncio.as_completed` accepts generators yielding tasks (python#103626)
  pythongh-102778: IDLE - make sys.last_exc available in Shell after traceback (python#103314)
  pythongh-103582: Remove last references to `argparse.REMAINDER` from docs (python#103586)
  pythongh-103583: Always pass multibyte codec structs as const (python#103588)
  pythongh-103617: Fix compiler warning in _iomodule.c (python#103618)
  pythongh-103596: [Enum] do not shadow mixed-in methods/attributes (pythonGH-103600)
  pythonGH-100530: Change the error message for non-class class patterns (pythonGH-103576)
  pythongh-95299: Remove lingering setuptools reference in installer scripts (pythonGH-103613)
  [Doc] Fix a typo in optparse.rst (python#103504)
  pythongh-101100: Fix broken reference `__format__` in `string.rst` (python#103531)
  pythongh-95299: Stop installing setuptools as a part of ensurepip and venv (python#101039)
  pythonGH-103484: Docs: add linkcheck allowed redirects entries for most cases (python#103569)
  pythongh-67230: update whatsnew note for csv changes (python#103598)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@merwok merwok merwok left review comments

@pfmoore pfmoore pfmoore approved these changes

@hugovk hugovk hugovk left review comments

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

@vsajip vsajip vsajip approved these changes

@dstufft dstufft dstufft approved these changes

@ezio-melotti ezio-melotti

@python/macos-team macos-team

Assignees
No one assigned
Labels
3.12 new features, bugs and security fixes stdlib Python modules in the Lib dir topic-ensurepip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet