Skip to content

Add support for python.org builds targeting osx 10.9 #224

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

Merged
merged 43 commits into from
Mar 23, 2019

Conversation

robbuckley
Copy link
Contributor

@robbuckley robbuckley commented Feb 4, 2019

as mentioned in MacPython/pandas-wheels#37, I've extended the osx parts of multibuild to allow building wheels using 10.9-based pythons from python.org, as well as the default 10.6. I've built wheels on travis for pandas using my fork, and they seem to install OK on my local system (10.14) with a range of pythons (2.7.10 / maxosx-10.14-intel, 3.7.2 / macosx-10.6-intel, 3.6.7 / maxosc-10.7-x86_64, 3.7.1 / macosx-10.9-x86_64)

My implementation adds a new environment variable MB_PYTHON_OSX_VER, to be defined in the end user's .travis.yml. It can determines the mac osx version of the python downloaded from python.org to build the wheel, for example:

python-2.7.15-macosx10.6.pkg
python-2.7.15-macosx10.9.pkg

If not present, it defaults to 10.6, as before. Wheels built using 10.9-based [64b] python are tagged with macosx_10_9_x86_64.macosx_10_10_x86_64 platforms, those built with 10.6-based [32/64b] pythons are tagged as before with macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64

I have come across a few issues which i cant explain. I don't know how important they are:

  1. tests/test_supported_wheels.sh doesn't seem to be able to fail - i discovered this when I broke the wheel format during testing.
  2. recent versions of pip I've tried (18.1, 19.0.1) seem to flag both the 10.6-based [tagged as 32/64b] and 10.9-based [tagged as 64b] wheels I've built as compatible with my [64b] system with both single arch [64b] and dual-arch [32/64b] pythons. That seems to contradict the info on https://github.com/MacPython/wiki/wiki/Spinning-wheels, which states that pip will not install single-arch wheels on dual-arch python versions. I have more info if needed
  3. using otool, i can see that libs in pandas wheels that I've built using the python.org 10.9-based pythons are fat 32/64b. I was expecting the libs to be 64b only. The wheel when first built is tagged as i expected with macosx_10_9_x86_64, it just that the binaries seem to be been compiled as dual-arch
  4. I have followed the existing code, adding 10.10 tags to the 10.9 wheels. Im curious why 10.10, and not also other later macosx versions (10.11...). I suppose this was to workaround some issue on older versions of pip?

@radarhere radarhere changed the title add support for python.org builds targetting osx 10.9 Add support for python.org builds targeting osx 10.9 Feb 5, 2019
@native-api
Copy link
Contributor

native-api commented Feb 11, 2019

I have come across a few issues which i cant explain. I dont know how important they are:

  1. tests/test_supported_wheels.sh doesn't seem to be able to fail - i discovered this when I broke the wheel format during testing.

Yes, the test is broken. It runs the script but doesn't check its result.

  1. recent versions of pip I've tried (18.1, 19.0.1) seem to flag both the 10.6-based [tagged as 32/64b] and 10.9-based [tagged as 64b] wheels I've built as compatible with my [64b] system with both single arch [64b] and dual-arch [32/64b] pythons. That seems to contradict the info on https://github.com/MacPython/wiki/wiki/Spinning-wheels, which states that pip will not install single-arch wheels on dual-arch python versions. I have more info if needed

If this is true, it looks like pip's problem. Or could be connected to the next item.

  1. using otool, i can see that libs in pandas wheels that I've built using the python.org 10.9-based pythons are fat 32/64b. I was expecting the libs to be 64b only. The wheel when first built is tagged as i expected with macosx_10_9_x86_64, it just that the binaries seem to be been compiled as dual-arch

See https://github.com/matthew-brett/multibuild/blob/devel/configure_build.sh#L19 .

@robbuckley
Copy link
Contributor Author

please note, i added another commit to build for 64-bit only for 10.9-based pythons, with a new function mac_arch_for_pyosx_version in osx_utils.sh. Once its passing the multibuild CI I will repeat the checks 2 and 3 in my first comment locally, and also build + test pandas wheels using a fork of https://github.com/MacPython/pandas-wheels

@robbuckley
Copy link
Contributor Author

robbuckley commented Feb 13, 2019

OK, i have built pandas 0.24.0 wheels for 2.7, 3.6 and 3.7, both 10.6 (dual arch) and 10.9 (single arch versions). They all passed the pandas release tests.

Also i did the following tests on my local system

  1. pip installed
  • 3.6 wheels on anaconda 3.6.7 python built for macosx-10.7-x86_64
  • 3.7 wheels on conda forge 3.7.1 python built for macosx-10.9-x86_64
  • 3.7 wheels on python.org installed on my base system (3.7.2 built for macosx-10.6-intel)
    Results: both 10.6(dual arch) and 10.9(single arch) wheels installed. This was expected on the first two pythons which are 64b-only. But not on the 3rd which is dual-arch. According to https://github.com/MacPython/wiki/wiki/Spinning-wheels that should have refused to install the 64b-only wheel
  1. checked the c++ standard library dependencies for a pandas C extension using delocate-listdeps
  • 10.6 builds link to libstdc++
  • 10.9 builds link to libc++
    (both as expected)
  1. checked the arches in a sample pandas C extension .so using lipo -info
  • 10.6 (dual arch) builds - i386 x86_x64
  • 10.9 (64b only) builds - x86_64
    (as expected)

Thats all the testing I was planning to do for this PR

@matthew-brett
Copy link
Collaborator

@radarhere - I'm slammed at the moment - can I leave you to merge this if you are happy?

@robbuckley
Copy link
Contributor Author

robbuckley commented Mar 11, 2019

i've addressed all comments (i think). There's another issue i've noticed, and have a fix for: the arch flags for pypy wheels will be set according to the MB_PYTHON_OSX_VER variable, which is only meant to be for cpython. Shall I add it to this PR, or wait till its merged and submit as a new PR? The effect of not adding this is fairly benign, in that pypy wheels may be built for dual arch when they should only be single arch. But the way its linked to the environment variable may be confusing. @radarhere @matthew-brett

@radarhere
Copy link
Collaborator

I would add it to this PR

@robbuckley
Copy link
Contributor Author

robbuckley commented Mar 13, 2019

i've built pandas 0.24 wheels again with the latest version of this PR, and checked them per https://github.com/matthew-brett/multibuild/pull/224#issuecomment-463392679. As a final check, I installed one of the 10.9 64b-only wheels in a conda environment and ran the pandas test suite.

@robbuckley
Copy link
Contributor Author

is there anything more you'd like me to do on this, @radarhere , @matthew-brett ? Im quite keen to get this merged and released as I'd like to use it in an MR for the next pandas release. If i can help pls let me know.

@matthew-brett
Copy link
Collaborator

@radarhere - I'm happy when you're happy.

@radarhere radarhere merged commit e373d06 into multi-build:devel Mar 23, 2019
@matthew-brett
Copy link
Collaborator

matthew-brett commented Mar 23, 2019 via email

@robbuckley robbuckley deleted the cpython-10.9 branch March 23, 2019 12:11
@robbuckley
Copy link
Contributor Author

thanks both for bearing with me on this PR! Does it need a release (i suppose that means a merge of devel>master) before I can use it for production?

@radarhere
Copy link
Collaborator

You don't need devel to be merged into master.

https://github.com/matthew-brett/multibuild/blob/devel/README.rst

We try to keep the master branch stable and do testing and development in the devel branch. From time to time we merge devel into master.

In practice, you can check out the newest commit from devel that works for you, then stay at it until you need newer features.

If you're talking about the use of multibuild in https://github.com/MacPython/pandas-wheels, then provided they don't mind going with a commit from the devel branch instead of master, you just need to go into the submodule, check out the relevant commit, go back into the base repository, and create a PR out of the change. To potentially save you some time though, here's a PR and/or commit that you can use if you want - https://github.com/MacPython/pandas-wheels/compare/master...radarhere:cpython-10.9?expand=1

@robbuckley
Copy link
Contributor Author

thanks

@robbuckley
Copy link
Contributor Author

Hmm - yes - it's complicated. Thinking about it, for macOS, we have:

  • The architecture of the installed Python. At the moment this gets set implicitly by the choice of 10.6 (intel) or 10.9 (x86_64)
  • The architecture to compile for. This is getting set by this code, so forced to the same as the architecture of installed Python.

Yes, youre right. Similar considerations apply to the targetted minimum macOS version set byMACOSX_DEPLOYMENT_TARGET. I took the path of least resistance, and just installed the python version with the right values for both.

I guess we can use PLAT as the second thing - architecture to compile for, and default to it being the same as the Python architecture. This will result in a lot of x86_64 compiles for intel - because people don't notice the distinction, and have PLAT set in their travis files. I doubt that will cause a problem. What do you think?

That would be consistent with PLAT usage for linux, and makes the "user API" more consistent. But its not back compatible - until now setting PLAT in the users .travis.yml had no effect for macOS. I cant judge which is better...

If you do change this, then really the wheel tags should be fixed to be consistent with the arches built. That could be done by reinstating some logic based on $PLAT in repair_wheelhouse

BTW i can see my changes broke things for some other users (eg https://github.com/matthew-brett/multibuild/pull/242). I'd like to help, e.g. by adding test coverage for breakages, even if they are now fixed. So if there's anything else you can point me towards, please let me know.

@matthew-brett
Copy link
Collaborator

It turned out your changes causes trouble only because of the PLAT issue - i.e. I thought PLAT was the PLAT set in .travis.yml, but in fact it was the one forced from configure_build.sh.

The most common use-case for PLAT differing from the implied platform, from the Python installation, is building intel wheels which do in fact only contain x86_64. I think that's fine - it allows the wheels to install on intel builds, and only breaks if you try to use it in 32-bit mode, which, as you say, is unlikely to happen.

Any interest in dropping some documentation of PLAT etc into the README, though?

@robbuckley
Copy link
Contributor Author

robbuckley commented Jul 25, 2019

Yes, Im happy to add to the docs.

So do you want to add the ability for the user to override PLAT for macOS builds or are you OK with it the way it is?

pls confirm and I'll open an issue to remind me to update the docs.

The problem with conversing on a closed PR is that the only notification from github is by email when a new comment is added - it doesnt appear on my dashboard at all. So Im always afraid I'll miss something

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants