Skip to content

Add CIBW_REPAIR_WHEEL_COMMAND env variable #211

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.

Sign up for GitHub

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
Merged
merged 4 commits into from
Nov 20, 2019
Merged

Add CIBW_REPAIR_WHEEL_COMMAND env variable #211

merged 4 commits into from
Nov 20, 2019

Conversation

nsoranzo
Copy link
Contributor

to allow different options to auditwheel/delocate, alternative commands and a future Windows-equivalent.

Fix #191 .

Also fix some PEP-008 issues reported by flake8.

@joerick
Copy link
Contributor

joerick commented Nov 13, 2019 •
edited
Loading

Hi @nsoranzo, thanks for submitting this! I preferred the naming CIBW_REPAIR_WHEEL, as @mayeut suggested - would you mind changing the name to that?

Otherwise, I get that people have different tastes regarding style and pep8 might be a good place to be, but if we wanted to adopt that properly (and put tooling in place) that should be a separate PR. It's just tricky as a reviewer to review both style and functionality changes at the same time, and can cause lots of conflicts with other active PRs.

Would you mind removing those style changes from this PR? Feel free to send them in as a separate PR if you like.

@nsoranzo
Copy link
Contributor Author

@joerick Thanks for the fast feedback! I've moved the PEP-008 changes to a new PR #212.

Regarding the environment variable name, what about CIBW_REPAIR_COMMAND ? To me CIBW_REPAIR_WHEEL suggest a yes/no value. Also _COMMAND is already used for CIBW_TEST_COMMAND.

YannickJadoul
YannickJadoul reviewed Nov 13, 2019
View reviewed changes
cibuildwheel/windows.py
Comment on lines +138 to +143
if os.path.exists(built_wheel_dir):
shutil.rmtree(built_wheel_dir)
os.makedirs(built_wheel_dir)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to have it close to the code that actually use the directory.

@YannickJadoul
Copy link
Member

Regarding the environment variable name, what about CIBW_REPAIR_COMMAND ? To me CIBW_REPAIR_WHEEL suggest a yes/no value.

Agreed. But then to be nitpicky, make it CIBW_REPAIR_WHEEL_COMMAND (or CIBW_WHEEL_REPAIR_COMMAND?). "repair" could be many things, out of context, I'd say. And the use of this option should be rare enough to not care about a slightly longer name.

@YannickJadoul
Copy link
Member

Regarding the environment variable name, what about CIBW_REPAIR_COMMAND ? To me CIBW_REPAIR_WHEEL suggest a yes/no value.

Agreed. But then to be nitpicky, make it CIBW_REPAIR_WHEEL_COMMAND (or CIBW_WHEEL_REPAIR_COMMAND?). "repair" could be many things, out of context, I'd say. And the use of this option should be rare enough to not care about a slightly longer name.

Just noticed: it's also CIBW_BEFORE_BUILD and not CIBW_BEFORE_BUILD_COMMAND, though.

@nsoranzo
Add CIBW_REPAIR_COMMAND env variable
0e04faf 
to allow different options for auditwheel/delocate, alternative
commands and a future Windows equivalent.

Fix pypa#191 .
@nsoranzo
Copy link
Contributor Author

Rebased to fix a conflict.

@joerick joerick changed the title Add CIBW_DELOCATE_COMMAND env variable Add CIBW_REPAIR_COMMAND env variable Nov 14, 2019
joerick
joerick reviewed Nov 14, 2019
View reviewed changes
Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Nice work @nsoranzo ! I've made a few comments inline.

Finally, on the naming, I'd agree with @YannickJadoul and prefer CIBW_REPAIR_WHEEL_COMMAND - would you mind changing it to that? It'll be an infrequently-used option so specific and long is a good compromise. Thank you!

@nsoranzo nsoranzo changed the title Add CIBW_REPAIR_COMMAND env variable Add CIBW_REPAIR_WHEEL_COMMAND env variable Nov 15, 2019
@YannickJadoul
Copy link
Member

Quick thing, still: I'd just made sure that all three platforms follow the same structure/do +- the same steps in the same order (cfr. #209). It's kind of nice to keep that, I think. So just for certainty, since I see some things moving around but I lack the overview: could you have a quick look whether that consistency between Windows & macOS (& Linux, to a certain degree) is still there?

@nsoranzo
Copy link
Contributor Author

@YannickJadoul Actually this PR slightly improves the consistency across architectures, I hope.
See e.g. https://github.com/joerick/cibuildwheel/pull/211/files#diff-b356ce3147eb040e0ec45382568d9b40R28-R33 , which replicates what was already done in windows.py .

@YannickJadoul
Copy link
Member

@nsoranzo OK, great, then! :-)

@joerick
Copy link
Contributor

joerick commented Nov 20, 2019

Looking good! Thanks @nsoranzo !

@joerick joerick merged commit 2b62eef into pypa:master Nov 20, 2019
@nsoranzo nsoranzo deleted the delocate_command branch November 21, 2019 23:37
nsoranzo added a commit to nsoranzo/pysam that referenced this pull request Nov 25, 2019
@nsoranzo
Use cibuildwheel from GitHub master branch
ac96167 
to allow specifying the `-L .` option for the auditwheel command.
Introduced in pypa/cibuildwheel#211 .

Fix pysam-developers#831 .
@nsoranzo nsoranzo mentioned this pull request Nov 26, 2019
Merged
nsoranzo added a commit to nsoranzo/pysam that referenced this pull request Dec 17, 2019
@nsoranzo
Use cibuildwheel>=1.1.0
5bf72be 
to allow specifying the `-L .` option for the auditwheel command.
Introduced in pypa/cibuildwheel#211 .

Fix pysam-developers#831 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joerick joerick joerick left review comments

@YannickJadoul YannickJadoul YannickJadoul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hook to replace auditwheel/delocate/[windows equivalent that doesn't exist yet]
3 participants
@nsoranzo @joerick @YannickJadoul