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-80789: Bundle ``ensurepip`` wheels at build time by AA-Turner · Pull Request #109130 · python/cpython

Conversation

@AA-Turner

@AA-Turner AA-Turner commented Sep 8, 2023 •
edited by github-actions Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Based on the discussion in #12791, this is a sketch of a different approach (building very heavily on @webknjaz's work).

We add a fairly simple bundler script at Tools/build/bundle_ensurepip_wheels.py, with two changes to ensurepip itself to adapt. This PR doesn't attempt to switch to the one-project model, as I found the diff was too large to reasonably review.

One question: should we make this part of the default build process (i.e. integration into make all / PCBuild)?

A

  • Issue: Replace bundled pip and setuptools with a downloader in the ensurepip module #80789

📚 Documentation preview 📚: https://cpython-previews--109130.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

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

webknjaz reacted with eyes emoji
All reactions
  • 👀 1 reaction
webknjaz and others added 4 commits September 7, 2023 00:32
@webknjaz @AA-Turner
Remove bundled pip wheel
0d8c94f
@AA-Turner @webknjaz
Add bundle_ensurepip_wheels
c2c9a55
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
@AA-Turner @webknjaz
Remove verify_ensurepip_wheels
76de0b8
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
@AA-Turner
Add Makefile target
252ee8e
@AA-Turner
AA-Turner requested review from pfmoore, pradyunsg and zware September 8, 2023 08:47
@AA-Turner
AA-Turner requested review from ezio-melotti and hugovk as code owners September 8, 2023 08:47
@bedevere-bot bedevere-bot added the awaiting review label Sep 8, 2023
@bedevere-bot bedevere-bot mentioned this pull request Sep 8, 2023
Replace bundled pip and setuptools with a downloader in the ensurepip module #80789
Closed
pfmoore
pfmoore approved these changes Sep 8, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Sep 8, 2023
AA-Turner added 3 commits September 8, 2023 11:05
@AA-Turner
Merge branch 'main' into ensurepip/bundler-tools
800c487
@AA-Turner
Attempt to fix Cirrus CI
4f0b3a2
@AA-Turner
Integrate into build
0aaf135
@AA-Turner
AA-Turner requested a review from a team as a code owner September 8, 2023 10:56
@AA-Turner
Revert "Integrate into build"
a163396
This reverts commit 0aaf135.
@AA-Turner

AA-Turner commented Sep 8, 2023

Copy link
Copy Markdown
Member Author

0aaf135 was an attempt to integrate this into the Makefile/PCBuild more 'properly', but I'm not confident in making wider-scale changes here (said commit didn't work, so I've reverted it).

All reactions

Sorry, something went wrong.

Uh oh!

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

zware
zware reviewed Sep 8, 2023
View reviewed changes

@zware zware 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.

This is going to need something for buildbots to avoid breaking them all, but I like the direction here.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/whatsnew/3.13.rst Resolved

Uh oh!

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

pradyunsg
pradyunsg reviewed Sep 8, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py Resolved

Uh oh!

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

@ned-deily

ned-deily commented Sep 8, 2023

Copy link
Copy Markdown
Member

Adding @ambv and @Yhg1s for review of impact on release process and supply chain security.

pradyunsg and AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner
Changes from first review
ccaad2d
@AA-Turner AA-Turner mentioned this pull request Sep 9, 2023
Only comment about new commits if the PR is still open. python/bedevere#583
Open
@AA-Turner
Merge branch 'main' into ensurepip/bundler-tools
db6f0db
# Conflicts:
#	.cirrus.yml
pradyunsg
pradyunsg approved these changes Sep 9, 2023
View reviewed changes
hugovk
hugovk reviewed Sep 10, 2023
View reviewed changes
Comment thread .cirrus.yml
pythoninfo_script:
- cd build
- make pythoninfo
bundle_ensurepip_script:

@hugovk hugovk Sep 10, 2023

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.

Suggested change
bundle_ensurepip_script:
# Download wheels for the venv step
bundle_ensurepip_script:

Is this the reason for adding it here?

Sorry, something went wrong.

Uh oh!

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

All reactions

@webknjaz webknjaz Sep 10, 2023

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.

Anything using venv or ensurepip during testing needs this provisioning step. If there's no uses, it'd be unnecessary.

Sorry, something went wrong.

Uh oh!

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

All reactions

@AA-Turner AA-Turner Sep 10, 2023

Copy link
Copy Markdown
Member Author

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.

The actual reason to include it is that Cirrus CI failed without this and passed with it. I don't know why Free BSD requires the venv step though, as all other tests pass normally -- only documentation and hypothesis (which use venvs) seem to have an explicit venv step.

A

Sorry, something went wrong.

Uh oh!

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

All reactions

@webknjaz webknjaz Sep 10, 2023

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.

@AA-Turner It's not because of venv but ensurepip itself. The call chain looks as follows:

  1. test.test_tools.test_freeze.TestFreeze.test_freeze_simple_script calls a CPython build preparation helper @ https://github.com/python/cpython/blob/cbb3a6f/Lib/test/test_tools/test_freeze.py#L28
  2. That, in turn, calls make install here https://github.com/python/cpython/blob/cbb3a6f/Tools/freeze/test/freeze.py#L184.
  3. make install calls ensurepip at https://github.com/python/cpython/blob/cbb3a6f/Makefile.pre.in#L1932-L1933 to provision it into the new Python install directory.
  4. ensurepip uses the bundled pip wheel to run bootstrapping so it attempts to unpack it and freaks out when there's no dist for it: https://github.com/python/cpython/blob/cbb3a6f/Lib/ensurepip/__init__.py#L176.

Sorry, something went wrong.

Uh oh!

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

All reactions

@webknjaz webknjaz Sep 10, 2023

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.

why Free BSD requires

It's not that FreeBSD or Cirrus requires the test explicitly, it's rather that this test is skipped on most platforms via https://github.com/python/cpython/blob/cbb3a6f/Lib/test/test_tools/test_freeze.py#L11-L20.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/test/test_tools/test_bundle_ensurepip_wheels.py Resolved

Uh oh!

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

Comment thread Lib/test/test_tools/test_bundle_ensurepip_wheels.py Resolved

Uh oh!

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

@AA-Turner
Add an explanatory comment to Cirrus CI
8fae12a
Reviewed-by: Hugo van Kemenade <[email protected]>
@webknjaz

webknjaz commented Sep 10, 2023

Copy link
Copy Markdown
Member

This is going to need something for buildbots to avoid breaking them all, but I like the direction here.

I identified the place needing to be changed in #12791 (comment).

@AA-Turner FYI

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner

AA-Turner commented Sep 10, 2023

Copy link
Copy Markdown
Member Author

I identified the place needing to be changed

I tried to explore a different approach of fully integrating this into make all in 0aaf135, which I think may be a better approach as it doesn't require downsteam distributors to change anything in their processes. I defer to those with much more experience with the build systems, though.

A

All reactions

Sorry, something went wrong.

Uh oh!

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

@webknjaz

webknjaz commented Sep 10, 2023

Copy link
Copy Markdown
Member

I tried to explore a different approach of fully integrating this into make all

I tried doing that in my PR and was getting weird behavior, I wasn't sure why. Maybe I was misusing something in the build machinery or was just confused...

If that works, it's probably fine. OTOH, you may want to take into account that many distributions prefer their build envs to be disconnected from the internet so it might still be wise to provide them with means to pre-provision the wheel separately.

Tagging @encukou @hroncok @mgorny for downstream opinions.

All reactions

Sorry, something went wrong.

Uh oh!

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

@webknjaz

webknjaz commented Sep 10, 2023

Copy link
Copy Markdown
Member

This PR doesn't attempt to switch to the one-project model, as I found the diff was too large to reasonably review.

Would #109245 help?

All reactions

Sorry, something went wrong.

Uh oh!

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

@mgorny

mgorny commented Sep 11, 2023

Copy link
Copy Markdown
Contributor

If that works, it's probably fine. OTOH, you may want to take into account that many distributions prefer their build envs to be disconnected from the internet so it might still be wise to provide them with means to pre-provision the wheel separately.

In Gentoo we aren't actually using the wheels originally bundled with CPython itself but providing the newest versions of the them separately, so as long as that continues working, I suppose that's fine with us. However, it is is paramount that:

  1. The build process and test suite fully respect --with-wheel-pkg-dir and do not attempt to download anything.
  2. This is entirely restricted to CPython build-time, and in particularly creating venv doesn't attempt any fetching.

However, I can imagine it could be mildly annoying to users who aren't used to the new workflow that having a complete git clone would no longer suffice to actually build CPython offline.

hroncok and encukou reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@hroncok

hroncok commented Sep 11, 2023

Copy link
Copy Markdown
Contributor

Same for Fedora. With an addition that we do sometimes keep the bundled wheels for too new or too old Pythons, so having a way to pre-populate the wheels instead of downloaidng them would still be needed.

pradyunsg 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.

@pradyunsg

pradyunsg commented Oct 4, 2023

Copy link
Copy Markdown
Member

@AA-Turner could you resolve the conflicts with this PR?

I think it would be good to have this be in Python 3.13's cycle as early as possible, to allow any potential redistributors who care about the details of ensurepip to have time to adjust things as well as provide us feedback on this as early as feasible.

AA-Turner 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.

@AA-Turner
Merge branch 'main' into ensurepip/bundler-tools
161f304
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py
spec = importlib.util.spec_from_file_location("ensurepip", ENSURE_PIP_INIT)
ensurepip = importlib.util.module_from_spec(spec)
spec.loader.exec_module(ensurepip)
return ensurepip._PROJECTS

@webknjaz webknjaz Oct 11, 2023

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.

@AA-Turner I believe this would need to be updated after #109245, and it'll probably let you work with simpler structures.

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py
whl = response.read()
except URLError as exc:
print_error(f"Failed to download {wheel_url!r}: {exc}")
errors = 1

@webknjaz webknjaz Oct 11, 2023

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.

Was this meant to be a counter?

Suggested change
errors = 1
errors += 1

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py
continue
else:
print_error(f"An invalid '{name}' wheel exists.")
os.remove(wheel_path)

@webknjaz webknjaz Oct 11, 2023

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.

Any reason not to use pathlib's method? Seeing that it's already used everywhere..

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py
from urllib.error import URLError
from urllib.request import urlopen

HOST = 'https://files.pythonhosted.org'

@webknjaz webknjaz Oct 11, 2023

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.

Can we name this something like a PYPI_CDN_URL for maintainability?

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py

def print_notice(message: str) -> None:
if GITHUB_ACTIONS:
print(f"::notice::{message}", end="\n\n")

@webknjaz webknjaz Oct 11, 2023

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.

Nit: this also works if output to stderr FYI.

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py

try:
projects = _get_projects()
except (AttributeError, TypeError):

@webknjaz webknjaz Oct 11, 2023

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.

I think there's an abstraction leak here: it's hard to guess from looking at the _get_projects() function, what in it might trigger these exceptions. I'd recommend processing them inside that function and making obvious such places where the exceptions may occur. Instead, I'd convert both of the exceptions into something like an ImportError and handle just that. This would contribute to transparency of how that function works.

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py
return 1

errors = 0
for name, version, checksum in projects:

@webknjaz webknjaz Oct 11, 2023

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.

FWIW with #109245, looping will stop making sense here. So maybe you could start working with just projects[0] already?

Sorry, something went wrong.

Uh oh!

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

All reactions

@pradyunsg pradyunsg Oct 12, 2023

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.

No, let's keep them separate.

Sorry, something went wrong.

Uh oh!

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

All reactions

@webknjaz webknjaz Oct 12, 2023

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.

I was kinda assuming that the simplification PR would get merged first, in which case, this one would have to adapt..

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Doc/whatsnew/3.13.rst

* Wheels for :mod:`ensurepip` are no longer bundled in the CPython source
tree. Distributors should bundle these as part of the build process by
running :file:`Tools/build/bundle_ensurepip_wheels.py`.

@webknjaz webknjaz Oct 11, 2023

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.

This would probably be clearer:

Suggested change
running :file:`Tools/build/bundle_ensurepip_wheels.py`.
running :file:`Tools/build/bundle_ensurepip_wheels.py` with no arguments.

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Lib/test/test_ensurepip.py
# ensure the pip wheel exists
pip_filename = os.path.join(test.support.STDLIB_DIR, 'ensurepip', '_bundled',
f'pip-{ensurepip._PIP_VERSION}-py3-none-any.whl')
if not os.path.isfile(pip_filename):

@webknjaz webknjaz Oct 11, 2023

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.

I feel like if it didn't exist, there should be some code to clean it up after testing so that the dummy file doesn't get forgotten on disk. Tests should avoid side effects...

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 11, 2023
View reviewed changes
Comment thread Lib/test/test_tools/test_bundle_ensurepip_wheels.py
import bundle_ensurepip_wheels as bew


# Disable fancy GitHub actions output during the tests

@webknjaz webknjaz Oct 11, 2023

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.

There should be a test for enabled GHA, just for those helpers, then. It'd be sad not to get coverage there.

Sorry, something went wrong.

Uh oh!

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

All reactions
webknjaz
webknjaz reviewed Oct 12, 2023
View reviewed changes
Comment thread Tools/build/bundle_ensurepip_wheels.py


def _is_valid_wheel(content: bytes, *, checksum: str) -> bool:
return checksum == sha256(content, usedforsecurity=False).hexdigest()

@webknjaz webknjaz Oct 12, 2023

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.

@AA-Turner I already know that this effort is going to be rejected for now, but out of curiosity — what's the motivation for setting usedforsecurity=False? The other verification script doesn't have that: https://github.com/python/cpython/blob/3d18034/Tools/build/verify_ensurepip_wheels.py#L81.

Sorry, something went wrong.

Uh oh!

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

All reactions
@pradyunsg

pradyunsg commented Oct 12, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Closing this per #80789 (comment). Thanks @AA-Turner for filing this, even though we're not going ahead with this (despite me implying that earlier in the discussion).

All reactions

Sorry, something went wrong.

Uh oh!

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

@pradyunsg pradyunsg closed this Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@webknjaz webknjaz webknjaz left review comments
@hugovk hugovk hugovk left review comments
@pradyunsg pradyunsg pradyunsg approved these changes
@zware zware zware left review comments
@pfmoore pfmoore pfmoore approved these changes
@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge

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.

10 participants

@AA-Turner @ned-deily @webknjaz @mgorny @hroncok @pradyunsg @pfmoore @hugovk @zware @bedevere-bot
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.