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

feat: support building universal/arm64 from x64_86 Python 3.8 #704

Merged
merged 1 commit into from
May 31, 2021

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented May 28, 2021

This adds support for creating wheels with CPython 3.8 for Apple Silicon. This is significant because 3.8 is the default Python on macOS's Xcode CLT.


Orignal message:

This currently produces an "x86_64" tagged wheel instead of the correct universal2 tag (but the wheel is fine if you rename it, other than having the wrong tag also listed in WHEEL). See scipy/oldest-supported-numpy#20 and grab example wheels at https://github.com/scikit-hep/boost-histogram/pull/583.

@@ -286,6 +281,11 @@ def setup_python(
# we can do universal2 builds on macos 10.15, but we need to
# set ARCHFLAGS otherwise CPython sets it to `-arch x86_64`
env.setdefault("ARCHFLAGS", "-arch arm64 -arch x86_64")
Copy link

Choose a reason for hiding this comment

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

You need to set _PYTHON_HOST_PLATFORM=macosx-11.0-universal2 and ARCHFLAGS here (regardless of the os version).

Copy link
Contributor Author

@henryiii henryiii May 28, 2021

Choose a reason for hiding this comment

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

Awesome, thanks! Though I think we could set _PYTHON_HOST_PLATFORM=macosx-10.9-universal2, since the Intel portion supports 10.9+ (it does require a slightly newer packaging/pip to be supported). Actually, should we get this number from value of MACOSX_DEPLOYMENT_TARGET? Or does that get computed correctly automatically?

Copy link

Choose a reason for hiding this comment

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

Actually, should we get this number from value of MACOSX_DEPLOYMENT_TARGET?

Sure. (That might break a few workflows because the wheel tag will have say 10.11 instead of the expected 10.9 which users might have renamed to correctly reflect MACOS_DEPLOYMENT_TARGET. Though this should be minor and using MACOSX_DEPLOYMENT_TARGET is the correct strategy.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting this as the minimum (10.9) seems to do the right thing:

3 wheels produced in 13 minutes:
  boost_histogram-1.0.3.dev15+g7314060-cp38-cp38-macosx_10_12_universal2.whl
  boost_histogram-1.0.3.dev15+g7314060-cp38-cp38-macosx_10_12_x86_64.whl
  boost_histogram-1.0.3.dev15+g7314060-cp38-cp38-macosx_11_0_arm64.whl

Copy link
Member

Choose a reason for hiding this comment

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

I went ahead & use MACOS_DEPLOYMENT_TARGET in the strategy. Feel free to drop the commit if not appropriate.

Copy link

Choose a reason for hiding this comment

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

Could you set CMAKE_OSX_ARCHITECTURES as well? https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_ARCHITECTURES.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scikit-build (the official CMake build system for for Python) now supports ARCHFLAGS, and https://github.com/pybind/cmake_example does too. If you are wrapping CMake yourself, I think you should respect ARCHFLAGS as well. I don't think we can or should set every build tool's special flags for this, but the tooling should support the standard Python settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/pybind/cmake_example/blob/f6539807e764707f2ee2b9d7ba0c8d017d685853/setup.py#L93-L97 , specifically, if you are not using Scikit-build for some reason.

Copy link

Choose a reason for hiding this comment

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

Good to know this. I will try Scikit-build. Would mind telling me who defined the "ARCHFLAGS" standard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's from setuptools, and therefore probably from distutils in the Python standard library.

@henryiii henryiii marked this pull request as ready for review May 28, 2021 20:28
@henryiii henryiii changed the title feat: support building univeral/arm from x64_86 Python 3.8 feat: support building universal/arm64 from x64_86 Python 3.8 May 28, 2021
@Czaki
Copy link
Contributor

Czaki commented May 29, 2021

It is possible to also add to documentation information on how cross-compile external libraries or some links to such documents? I one project (https://github.com/czaki/imagecodecs_build) time of external libraries build is ~30 min, so I prefer to build it once per architecture.

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.

Ah, that's a nice solution to the Python 3.8 README footnote issue :D actually make it work!

This is a bit of a head-scratcher, but now that I've read it a few times, it does make sense. I don't have a strong opinion on whether MACOSX_DEPLOYMENT_TARGET should match _PYTHON_HOST_PLATFORM, but I agree that it does seem like the most 'correct' way to do it. So I guess I'm +0.1 on how it is currently.

LGTM, comments are only suggestions.

# macOS 11 is the first OS with arm64 support, so the wheels
# have that as a minimum.
env.setdefault("_PYTHON_HOST_PLATFORM", "macosx-11.0-arm64")
deployment_target = max(deployment_target, (11, 0))
host_arch = "arm64"
Copy link
Contributor

Choose a reason for hiding this comment

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

host_arch seems like the wrong name, I think... perhaps target_arch? I know that we're setting the _PYTHON_HOST_PLATFORM env var, but I think the only reason it's called that is because it overrides a function called get_host_platform in distutils.

Comment on lines 272 to 279
deployment_target = tuple(map(int, env.get("MACOSX_DEPLOYMENT_TARGET", "10.9").split(".")[:2]))
if deployment_target[0] >= 11:
# starting with macOS 11, the minor version in fact represents patch version
deployment_target = (deployment_target[0], 0)
if len(deployment_target) < 2:
# even though it didn't make much sense before macOS 11, it was valid to set
# MACOSX_DEPLOYMENT_TARGET=10 which is 10.0
deployment_target = (deployment_target[0], 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I tripped up on this a little, perhaps something like the following would be easier to read?

Suggested change
deployment_target = tuple(map(int, env.get("MACOSX_DEPLOYMENT_TARGET", "10.9").split(".")[:2]))
if deployment_target[0] >= 11:
# starting with macOS 11, the minor version in fact represents patch version
deployment_target = (deployment_target[0], 0)
if len(deployment_target) < 2:
# even though it didn't make much sense before macOS 11, it was valid to set
# MACOSX_DEPLOYMENT_TARGET=10 which is 10.0
deployment_target = (deployment_target[0], 0)
parts = env.get("MACOSX_DEPLOYMENT_TARGET", "10.9").split(".")
if len(parts) < 2:
deployment_target = int(parts[0]), 0
else:
deployment_target = int(parts[0]), int(parts[1])
if deployment_target[0] >= 11:
# starting with macOS 11, we retain compatibility across minor versions
deployment_target = (deployment_target[0], 0)

@henryiii
Copy link
Contributor Author

I'm in favor of the original form, for the following reasons:

  1. It reads better, in that you can see what's actually needed. When compiling to ARM, the minimum is 11.0. When using the Universal2 binary, it secretly supports 10.9 when targeting Intel, even though it's got an 11 in the name (for 3.9+, anyway). You can see this in the code, while in the current version, _PYTHON_HOST_PLATFORM loses its original meaning (what the Python host supports) and just becomes another way to write MACOSX_DEPLOYMENT_TARGET. If we want to catalog "what needs to be done to build highly compatible wheels", I would much rather the original version.
  2. It could eventually go away. When we drop support for Intel and Python < 3.9, this "hack" goes away and we can leave _PYTHON_HOST_PLATFORM untouched. While the current hack will never obviously go away.
  3. I like doing as little hacking as possible; the more we meddle with, the more we tend to break. I have already shown that there was a bug in the new version (setting MACOSX_DEPLOYMENT_TARGET to 11 broke the parsing that didn't have to be there). This changes _PYTHON_HOST_PLATFORM for many projects, the original version only changed it when absolutely necessary; only cross compiling on Python 3.8 was affected.
  4. Setting MACOSX_DEPLOYMENT_TARGET less than _PYTHON_HOST_PLATFORM is clearly a mistake. However, you see that if you always override one with the other.

If there was a clear reason to do this, then that would be different. But the wheel tags produced are already correct; they respect MACOSX_DEPLOYMENT_TARGET. This seems just like an arbitrary change and is not related to the point of this PR. At the very least, I'd like to get this PR without that change, then discuss making this change in a dedicated PR.

@mayeut
Copy link
Member

mayeut commented May 30, 2021

At the very least, I'd like to get this PR without that change, then discuss making this change in a dedicated PR.

As mentioned in my last comment, I mostly agree with @henryiii and think that this PR should drop my commits to proceed in its original form.

I've backed up the branch on my fork if we want to resume the conversation on this one later: https://github.com/mayeut/cibuildwheel/tree/backup-mdt

@joerick
Copy link
Contributor

joerick commented May 30, 2021

Sounds good to me

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.

None yet

6 participants