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

Move final wheel builder copy operation to wheel command #7517

Merged
merged 5 commits into from Dec 29, 2019

Conversation

@sbidoul
Copy link
Member

sbidoul commented Dec 26, 2019

Removes one more non-build concern from WheelBuilder.build().

sbidoul added 3 commits Dec 26, 2019
Also, pluralize variable names for readability and consistency with
similar variables in callers.
Copy link
Member

chrahunt left a comment

Technically this changes the behavior so that wheels will appear after all builds instead of as they are built, however:

  1. I think that this will be necessary going forward anyway with the new resolver work, since we won't know until the end which wheels are relevant, and will have to build some wheels in the middle of resolving
  2. the wheels that we do succeed in building will be available in the cache, so if an issue does happen the next invocation of the command should be fast since it will take files directly from the cache instead of rebuilding them

So LGTM

@sbidoul sbidoul marked this pull request as ready for review Dec 26, 2019
@pradyunsg pradyunsg removed the trivial label Dec 28, 2019
Copy link
Member

pradyunsg left a comment

LGTM, though this should be called out in the release notes.

sbidoul added 2 commits Dec 27, 2019
@sbidoul sbidoul force-pushed the sbidoul:wheel-builder-disentangle-6-sbi branch from 1ef342d to 1f39950 Dec 28, 2019
@sbidoul

This comment has been minimized.

Copy link
Member Author

sbidoul commented Dec 28, 2019

I added a .feature news explaining the new behavior. I suppose it's not a significant change that needs to be called out as breaking.

@chrahunt chrahunt merged commit 711cf4d into pypa:master Dec 29, 2019
26 checks passed
26 checks passed
🤖 (ubuntu-18.04, docs)
Details
🤖 (ubuntu-18.04, lint)
Details
Linux Build #20191228.4 succeeded
Details
Linux (Package) Package succeeded
Details
Linux (Test Primary Python27) Test Primary Python27 succeeded
Details
Linux (Test Primary Python36) Test Primary Python36 succeeded
Details
Linux (Test Secondary Python35) Test Secondary Python35 succeeded
Details
Linux (Test Secondary Python37) Test Secondary Python37 succeeded
Details
Windows Build #20191228.4 succeeded
Details
Windows (Package) Package succeeded
Details
Windows (Test Primary Python27-x86) Test Primary Python27-x86 succeeded
Details
Windows (Test Primary Python35-x64) Test Primary Python35-x64 succeeded
Details
Windows (Test Primary Python37-x64) Test Primary Python37-x64 succeeded
Details
Windows (Test Secondary Python35-x86) Test Secondary Python35-x86 succeeded
Details
Windows (Test Secondary Python36-x86) Test Secondary Python36-x86 succeeded
Details
Windows (Test Secondary Python37-x86) Test Secondary Python37-x86 succeeded
Details
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
macOS Build #20191228.4 succeeded
Details
macOS (Package) Package succeeded
Details
macOS (Test Primary Python27) Test Primary Python27 succeeded
Details
macOS (Test Primary Python36) Test Primary Python36 succeeded
Details
macOS (Test Secondary Python35) Test Secondary Python35 succeeded
Details
macOS (Test Secondary Python37) Test Secondary Python37 succeeded
Details
news-file/pr News files updated and/or change is trivial.
Details
@sbidoul sbidoul deleted the sbidoul:wheel-builder-disentangle-6-sbi branch Dec 29, 2019
@sbidoul

This comment has been minimized.

Copy link
Member Author

sbidoul commented Dec 29, 2019

@chrahunt I can continue with moving the should_unpack code path to the install command. Does that sound good?

FYI I tried removing the unpacking, but it's still necessary for WheelDistribution.get_pkg_resources_distribution, which in turn gets called by check_install_conflicts via _simulate_installation_of.

@chrahunt

This comment has been minimized.

Copy link
Member

chrahunt commented Dec 29, 2019

Yup, sounds good to me. Regarding get_pkg_resources_distribution, I noticed the same and am working on getting metadata directly from the wheel to accommodate it.

@lock lock bot added the S: auto-locked label Jan 28, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.