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

Refactor wheel.move_wheel_files to use updated distlib #6763

Merged
merged 4 commits into from Sep 8, 2019

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Jul 22, 2019

With the recent update to distlib, we can rid ourselves of some workarounds in wheel.py.

Now we rely on the script template in distlib here.

The script template is essentially the same as the one that was in wheel.py before, with the exception of the regular expression here. For reference, our original regular expression was (-script\.pyw?|\.exe)?$ and the new one is (-script\.pyw?|\.exe)?$. In our case since distlib itself is generating the wrapper and only generates wrappers with names as-is (for Unix) and with .exe (for Windows), the -script.pyw will never be applicable, ? not withstanding. This is further elaborated on here around the time of the original issue being raised.

In a followup PR I should be able to extract and unit test the generation of the script spec generation.

Notes:

  1. The removed comment "fix the fact that the default script swallows every single stack trace" is related to the discussion here. Essentially: The previous default template in distlib was catching exceptions and printing a simple error message
  2. The removed comment "Simplify the script" is related to the discussion here (thanks @takluyver for following up!)

@chrahunt chrahunt added skip news type: maintenance labels Jul 22, 2019
@chrahunt chrahunt force-pushed the maint/use-new-distlib-in-wheel branch from ceaf426 to 8b254c2 Compare Jul 22, 2019
@takluyver
Copy link
Member

@takluyver takluyver commented Jul 22, 2019

Thanks are probably more due to Vinay - I'm just the annoying git who nagged him about it. 😉

@chrahunt chrahunt force-pushed the maint/use-new-distlib-in-wheel branch from 8b254c2 to 6339545 Compare Jul 24, 2019
Copy link
Member

@xavfernandez xavfernandez left a comment

👍

@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Aug 5, 2019

@xavfernandez If you're confident about these changes, feel free to merge them. :)

@BrownTruck
Copy link
Contributor

@BrownTruck BrownTruck commented Aug 18, 2019

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge label Aug 18, 2019
@chrahunt chrahunt force-pushed the maint/use-new-distlib-in-wheel branch from 6339545 to b0a7d2b Compare Sep 2, 2019
@pypa-bot pypa-bot removed the needs rebase or merge label Sep 2, 2019
@chrahunt
Copy link
Member Author

@chrahunt chrahunt commented Sep 5, 2019

Gentle nudge. :)

@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Sep 7, 2019

@chrahunt A git-workflow-related comment: It's much easier to review PRs when changes-with-changing-variable-names vs other changes happen in separate commits.

This PR would've been easier to review if there were 2 separate commits:

  • Changing the ScriptMaker
  • Improving the flow of code, w.r.t. removing use of maker.make at each call site.

src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
tests/unit/test_wheel.py Outdated Show resolved Hide resolved
@chrahunt chrahunt force-pushed the maint/use-new-distlib-in-wheel branch from 752203b to f92961d Compare Sep 7, 2019
entry = e.args[0]
raise InstallationError(
"Invalid script entry point: {} for req: {} - A callable "
"suffix is required. Cf https://packaging.python.org/en/"
Copy link
Member

@pradyunsg pradyunsg Sep 7, 2019

Choose a reason for hiding this comment

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

Noting that we may want to re-wrap this text, so that the URL isn't wrapped.

@pradyunsg pradyunsg removed the skip news label Sep 7, 2019
@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Sep 7, 2019

This should probably get a NEWS file, since it's worth calling out as a .vendor (or .bugfix?) -- "Switch to using distlib's wheel script template as is. This should be functionally equivalent for end users." (or something along these lines).

@pradyunsg pradyunsg changed the title Refactor wheel.move_wheel_files to use updated distlib. Refactor wheel.move_wheel_files to use updated distlib Sep 8, 2019
@pradyunsg pradyunsg merged commit e023973 into pypa:master Sep 8, 2019
22 checks passed
@chrahunt chrahunt deleted the maint/use-new-distlib-in-wheel branch Sep 8, 2019
@lock lock bot added the auto-locked label Oct 8, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked type: maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants