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.

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

macOS update #57

Merged
merged 3 commits into from
Apr 2, 2018
Merged

macOS update #57

merged 3 commits into from
Apr 2, 2018

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Mar 31, 2018

This PR does the following things:

@mayeut mayeut mentioned this pull request Mar 31, 2018
Closed
@YannickJadoul
Copy link
Member

YannickJadoul commented Apr 1, 2018 •
edited
Loading

Thanks for this pull request! Looks pretty good to me, but I've got two minor questions:

  • Can you explain why you are downloading and calling the get-pip.py script? Don't newly installed python versions already have a pip? So we could just pip install -U pip? (As suggested by pip itself in your failed build: https://travis-ci.org/mayeut/cibuildwheel/jobs/360577719#L148). Or is there a problem with that that I'm missing?
  • Closely linked to what you did in this PR: would you mind having a look at the minimal example in the README.md file. Brew on OS X is also being used there (I believe?), so we should make sure it still works. (Yeah, we should probably turn that into an actual test, but ... faster/easier said than done.)

Apart from that, I'm happy to merge this if @joerick and @tgarc don't have any remarks? I guess given the severity of the problem, it might be nice to have a patch/release for this reasonably soon?

@mayeut
Copy link
Member Author

mayeut commented Apr 2, 2018

@YannickJadoul,

Can you explain why you are downloading and calling the get-pip.py script? Don't newly installed python versions already have a pip? So we could just pip install -U pip?

pip will fail to upgrade itself for the same reason. It lacks TLS 1.2 support. Using the get-pip.py script is the only way to install a working pip.

Closely linked to what you did in this PR: would you mind having a look at the minimal example in the README.md file. Brew on OS X is also being used there (I believe?), so we should make sure it still works.

Though the minimal example does not exactly install cibuildwheel in the same way as in the .travis.yml file, it's using python2 from Brew which is built against a Brew OpenSSL library that supports TLS 1.2. This differs from the official python packages used by cibuildwheel which are linked against the outdated macOS OpenSSL library.

@YannickJadoul
Copy link
Member

@mayeut Aaargh, that's quite a silly situation. Either way; then this seems to be the best solution indeed. Thanks for looking into it!

@joerick, should we then merge this soon?

joerick
joerick reviewed Apr 2, 2018
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.

Hey mayeut, thanks a lot for this! I've made a few comments in the diff, otherwise keen to get this merged soon too.

cibuildwheel/macos.py
@@ -63,7 +68,7 @@ def call(args, env=None, cwd=None, shell=False):
call([python, '--version'], env=env)

# install pip & wheel
call([python, '-m', 'ensurepip', '--upgrade'], env=env)
call([python, get_pip_script, '--no-setuptools', '--no-wheel'], env=env)
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason for --no-setuptools and --no-wheel?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather get the script only install pip and let the remaining as it was before.
wheel is installed shortly after.
This is also what travis-ci/dpl is doing for deployment so I guess I just did the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

.travis.yml
@@ -32,7 +32,7 @@ matrix:
- "PYTHON=python3"
before_install:
- brew update
- brew install python3
- brew outdated python || brew upgrade python
Copy link
Contributor

Choose a reason for hiding this comment

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

brew outdated returns with success if python isn't installed. Perhaps this line should be brew install python || brew upgrade python?

Copy link
Member Author

Choose a reason for hiding this comment

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

python is installed on macOS travis-ci images.
Otherwise, I can change this by

brew install python || brew outdated python || brew upgrade python

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry should be

brew install python && ( brew outdated python || brew upgrade python )

as install returns success even if already installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

well, I just did a test and it seems the behavior of brew install is not really consistent.
brew install python exits with 0 on my laptop (python is already installed). It exits with error code on travis-ci.
https://discourse.brew.sh/t/what-exit-code-is-brew-install-supposed-to-return-when-formula-is-already-installed/1607

Copy link
Member Author

Choose a reason for hiding this comment

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

I can switch to using a brew file & brew bundle. This works:
https://travis-ci.org/mayeut/cibuildwheel/builds/361196481
Tell me if you want me to use this instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's cool, I didn't know about brew bundle. But seems a little heavy-handed for this. Let's stick with your original for the moment.

@joerick joerick merged commit 7f77aa8 into pypa:master Apr 2, 2018
@joerick
Copy link
Contributor

joerick commented Apr 2, 2018

Released as 0.7.1. thanks all!

@mayeut mayeut deleted the macos-update branch April 2, 2018 15:59
@TheBB TheBB mentioned this pull request Apr 24, 2018
Closed
@YannickJadoul YannickJadoul mentioned this pull request May 6, 2018
Merged
@mayeut mayeut mentioned this pull request May 6, 2018
Merged
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

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

Successfully merging this pull request may close these issues.
3 participants
@mayeut @YannickJadoul @joerick