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

Compile in universal2 mode for macOS Python >= 3.9.1. #114

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

bwoodsend
Copy link
Contributor

Fixes actions/runner-images#4133.

For Python versions that support it, build in universal2 mode so that package authors can build macOS M1 compatible wheels. As it is at the moment, I've made the switch mandatory because I can't think of any reason (minus the increase in size) that a universal2 Python would cause any harm.

@bwoodsend
Copy link
Contributor Author

@miketimofeev Prod prod...

@miketimofeev
Copy link
Contributor

@bwoodsend sorry, we didn't have a chance to take a look yet, but it's on our radar

@astrofrog
Copy link

Just a +1 for this - it would be great to have!

@miketimofeev
Copy link
Contributor

miketimofeev commented Oct 18, 2021

@bwoodsend we have announced macOS-10.14 image deprecation so in the scope of switching python packages generation to macOS-10.15 we will consider adding this change as well

@bwoodsend
Copy link
Contributor Author

Hmm, not entirely sure what that implies but it sounds like good news ... 🙂

@@ -6,7 +6,7 @@ stages:
- stage: Build_Python_MacOS
dependsOn: []
variables:
VmImage: 'macOS-10.14'
VmImage: 'macOS-10.15'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One side effect of bumping the build image version is that, because the MACOSX_DEPLOYMENT_TARGET isn't set anywhere, the Pythons and therefore any wheels produced will only be compatible with macOS >= 10.15. Is there a reason MACOSX_DEPLOYMENT_TARGET isn't used? The python.org builds use minimum macOS version from 10.6 to 10.9 (which admittedly is overkill).

FtZPetruska added a commit to FtZPetruska/video-dl that referenced this pull request Jan 2, 2022
Due to `setup-python@v2` limitations, only x64 binary are created for
macOS, universal binaries will be available once the following PR is
merged: actions/python-versions#114
### For Python versions which support it, compile a universal2 (arm64 + x86_64 hybrid) build. The arm64 slice
### will never be used itself by a Github Actions runner but using a universal2 Python is the only way to build
### universal2 C extensions and wheels.
if ($this.Version -ge "3.9.1") {

Choose a reason for hiding this comment

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

May I ask why the Python version need to be greater than 3.9.1? In official Python download page, you can have universal Python for version 3.8.10. See below:

https://www.python.org/downloads/macos/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right. I hadn't realised that they backported to Python 3.8.x as well. Note though that Python 3.9.0 does not support universal2 so the conditional would have to be:

if ($this.Version -ge "3.8.10" && $this.Version -ne "3.9.0") {

(assuming that that is the correct powershell syntax).

Copy link

@TonyXiang8787 TonyXiang8787 Jan 28, 2022

Choose a reason for hiding this comment

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

I think 3.8.10 is the only 3.8 version with universal2. So a ==3.8.10 or >=3.9.1 will work I guess.

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 can compile universal2 for 3.8.11 - it's just by that point, python.org stops providing precompiled installers of any kind.

Using `universal2` Python environments is currently the only way to build Python
wheels which support macOS M1.
@bwoodsend
Copy link
Contributor Author

That last change was just a rebase plus adding Python 3.8.10+ to the Python versions to use universal2.

@misl6
Copy link

misl6 commented May 31, 2022

As a bonus point:

  • Github is now providing actions/runner binaries for Apple Silicon (self-hosted)
  • A universal2 compatible Python, provided by actions/python-versions will also run on Apple Silicon.
  • With some (minimal?) changes (e.g: versions-manifest.json) a runner on Apple Silicon will likely be able to download and install the proper Python version, without any additional M1-specific configuration.

@bwoodsend
Copy link
Contributor Author

bwoodsend commented Jun 1, 2022

Github is now providing actions/runner binaries for Apple Silicon (self-hosted)

That is the best piece of news I've heard for ages. I've been facing using Jenkins indefinitely at the day job since it's the closest we can get to CI/CD on M1.

@bwoodsend
Copy link
Contributor Author

Github is now providing actions/runner binaries for Apple Silicon (self-hosted)

I take it that there's still no VMware in there so no job isolation and therefore no chance of this being available without self-hosting?

@ssbarnea
Copy link

ssbarnea commented Jun 6, 2022

Thanks! I read the code node and that comment worries me a little "The arm64 slice will never be used itself by a Github Actions runner".

I wonder that comment was before the news went out, like https://www.protocol.com/bulletins/microsoft-azure-arm-chips-ampere --- With Microsoft building their own arm based servers for azure, I believe that in 1-2 years we will likely see runners on arm, as being cheaper.

Another case of never say never? ;)

@bwoodsend
Copy link
Contributor Author

Despite it having the same name and instruction set, Apple M1 isn't really arm64. It's a propriety chip requiring custom drivers. You can't just grab any arm64 device and run macOS on it, nor can you run Linux or Windows on an M1 machine (well Linux has just about managed it but not without a lot of work).

@bwoodsend bwoodsend deleted the universal2 branch June 6, 2022 18:39
charles-cooper added a commit to kelvinfan001/vyper that referenced this pull request Jun 17, 2022
the fix in actions/python-versions#114 only
applies to >= 3.9.1
@alex
Copy link

alex commented Jun 18, 2022

Is it expected that this might break anything in users' builds? We (pyca/cryptography) are seeing builds failing with 3.10.5 (the first version to be built with this patch as far as I can tell). here's what the failures look like: https://github.com/pyca/cryptography/runs/6948897569?check_suite_focus=true

And here's a build demonstrating that 3.10.4 is clean: pyca/cryptography#7344

The big difference one see is in the CLI args that are passed to the compiler when building C extensions. Here's what a diff of those looks like between passing and a failing build:

--- old	2022-06-18 11:24:34.000000000 -0400
+++ new	2022-06-18 11:24:43.000000000 -0400
@@ -7,6 +7,12 @@
 -fwrapv
 -O3
 -Wall
+-arch
+arm64
+-arch
+x86_64
+-isysroot
+/Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk
 -I/usr/local/opt/sqlite/include
 -I/usr/local/opt/sqlite/include
 -I/Users/runner/openssl-macos-x86-64/include
@@ -19,12 +25,11 @@
 -march=core2
 -DUSE_OSRANDOM_RNG_FOR_TESTING
 -I/Users/runner/work/cryptography/cryptography/.tox/py310/include
--I/Users/runner/hostedtoolcache/Python/3.10.4/x64/include/python3.10
+-I/Users/runner/hostedtoolcache/Python/3.10.5/x64/include/python3.10
 -c
-build/temp.macosx-10.15-x86_64-cpython-310/_openssl.c
+build/temp.macosx-10.15-universal2-cpython-310/_openssl.c
 -o
-build/temp.macosx-10.15-x86_64-cpython-310/build/temp.macosx-10.15-x86_64-cpython-310/_openssl.o
+build/temp.macosx-10.15-universal2-cpython-310/build/temp.macosx-10.15-universal2-cpython-310/_openssl.o
 -Wconversion
 -Wno-error=sign-conversion
 

@bwoodsend
Copy link
Contributor Author

I think that the only expected failures are if you're trying to link against single arch binaries (possibly installed via brew) or if you land with an XCode < 12.2. Neither of those look like the problem you're facing.

@alex
Copy link

alex commented Jun 19, 2022

We are actually trying to link against a single arch binary -- the OpenSSL we're linking there is x86_64 only. However, even if we switch to linking against a universal2 OpenSSL (see pyca/cryptography#7346) the issue with the sysroot exists.

Is it possible these builds aren't compatible with the macos-12 images we're using?

@bwoodsend
Copy link
Contributor Author

I think that it's more likely just to be a bug either in XCode or in one of the layers of build tools if it's pointing to non-existant XCode SDKs. I'll fire up my not so beloved macOS and see if I can reproduce locally.

@alex
Copy link

alex commented Jun 19, 2022

I've put together a minimal reproducer that demonstrates that universal2 + macos-12 images aren't working for some usecases: https://github.com/alex/temp-gha-macos-python/actions/runs/2523945774

@alex
Copy link

alex commented Jun 20, 2022

I think 3.10.4 must have gotten rebuilt as universal2 in the last day or so, as it's now failing the same way 3.10.5 does.

@achals
Copy link

achals commented Jun 24, 2022

Not sure if I misunderstood, but won't this break building wheels for python projects that also compile and link to native code?

To provide context, I'm a maintainer for feast-dev/feast. We have some latency sensitive code that's been ported to go and we call into it using gopy. Up until the last release we attempted, we'd correctly produce wheels with the go libraries embedded for x86_64, and other platforms needed to install feast from source. This change ended up breaking our wheel building process and required us to do some digging to figure out what had changed.

I'd love to know if there's a better way for us to be building wheels or setting up our toolchain.

@bwoodsend
Copy link
Contributor Author

Yeah, I'm having second thoughts about not making this optional. I believe that the status quo in PyPA land is very much universal2 or nothing - compiling all dependencies from source if that's what it takes.

You can opt-out of universal2 mode using the hidden set of environment variables that cibuildwheel uses. Namely, set ARCHFLAGS="-arch x86_64" _PYTHON_HOST_PLATFORM=macosx-10.9-x86_64. (In fact, now that I see that code I realise that there was a hidden way to build universal2 wheels with x86_64 Python after all. 🙄 ) Note that that 10.9 is your minimum supported macOS version. You can adjust it freely if you want to.

@alex
Copy link

alex commented Jun 24, 2022

Explicitly providing ARCHFLAGS and _PYPTHON_HOST_PLATFORM does not resolve the issue that on macOS-12 the sysroot points at a non-existant path and thus extension modules cannot be built.

@bwoodsend
Copy link
Contributor Author

Can you share a build URL for that?

@alex
Copy link

alex commented Jun 24, 2022

pyca/cryptography#7346 (it's going to take a minute for the macOS 3.10 job to fail, I previously reverted the commit so the CI needs to re-run)

Edit: Specific job https://github.com/pyca/cryptography/runs/7046642554?check_suite_focus=true

@hynek
Copy link

hynek commented Jun 29, 2022

Hi, this change has also broken the build process of a C extension of mine, because now CONFIGURE_CFLAGS hardcodes cross-compiling x64 and arm. That fails because I need to use different C files on each platform. Here's more detailsso far I haven't found a useful way to circumvent this forced cross-compilation. I'm not even building wheels (my cibuildwheel job seems to be working FWIW), it's just a pip install -e . (but python -m build . failed too).

In my case setting setting ARCHFLAGS helped, but it send me on an hours-long goose hunt.

@bwoodsend
Copy link
Contributor Author

@miketimofeev Would you be OK with this change becoming optional and opt-in. i.e. Anyone wanting universal2 would have to use?:

- uses: setup-python@v3
  with:
    macos_arch: universal2

It feels like the safest option to me.

@ssbarnea
Copy link

I know that this might be inconvenient for some projects that failed to adopt the universal2, but I am against not making the default universal2 as that is what Apple recommends.

The opt-in should be about not avoiding universal2.

@alex
Copy link

alex commented Jun 29, 2022

If universal2 is going to be the default, the fact that it does not work on GHA's macOS 12 needs to be resolved. Until that happens, I encourage reverting the default.

@miketimofeev
Copy link
Contributor

@marko-zivic-93 ^

@vsafonkin
Copy link

vsafonkin commented Jun 30, 2022

@bwoodsend looks like it will be better to disable universal2 builds at the moment and revert these changes.

@cielavenir
Copy link

oh reverting that change will break https://github.com/cielavenir/python-slz/actions/workflows/wheel.yaml

@misl6
Copy link

misl6 commented Jul 23, 2022

I know that this might be inconvenient for some projects that failed to adopt the universal2, but I am against not making the default universal2 as that is what Apple recommends.

The opt-in should be about not avoiding universal2.

Absolutely agree.

PS: Having multiple builds (x86_64, arm64 and universal2) at the first stage, and then later switching to universal2 as a default would also work well.

@albertosottile
Copy link

I understand why you wanted to disable the default universal2 build for all the GHA projects, but the need described in actions/runner-images#4133 is still valid. Is there a plan to reintroduce a universal2 Python build in GHA, at least optionally?

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