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

Add support for macOS 11.0 as a target #528

Merged
merged 22 commits into from
Oct 22, 2024

Conversation

freakboy3742
Copy link
Contributor

The osx_utils.sh script provides for 32-bit and 64-bit intel builds, but doesn't provide any default support for ARM64 builds.

This PR adds support for MB_PYTHON_OSX_VER=11.0 mapping to arm64 builds. macOS 11.0 was the first release to support ARM64, so this is the usual SDK level targeted when an ARM64 wheel is generated.

@mattip
Copy link
Collaborator

mattip commented Oct 21, 2024

Thanks. I guess you are adding this for a concrete use case? Most projects have migrated to cibuildwheel instead, I wonder who is still using multibuild.

It would be nice to test this. I see we added a matrix for testing under different OSes in #456, but never followed up to enable the commented-out OSes. Could you try enabling that to see if this passes tests?

@freakboy3742
Copy link
Contributor Author

The backstory here is that I'm the author of PEP 730 adding iOS support to CPython. Those changes are part of CPython 3.13, so now I'm working on the changes needed to support iOS cross-platform builds for the rest of the CPython ecosystem.

I've got a work-in-progress branch for cibuildwheel; that's enough to get a really simple package building for iOS; I'm currently working on compiling Pillow for iOS as a more complex example with downstream binary dependencies, and Pillow uses multibuild to provide macOS binaries.

There's a lot of similarities between the iOS and a macOS build processes; in the process of working out how Pillow's build system worked, I hit this issue, and the fix seemed obvious, so I figured I'd submit a PR. I don't have a pressing use for this specific change myself - the Pillow usage will always be via cibuildwheel, which (indirectly) sets PLAT. However, since I was in the area, I figured I'd submit the change.

As for enabling tests... I'd be happy to ... except that the test matrix doesn't seem to actually test anything, and it's not obvious to me how the test suite is meant to be run. The best I can find is the travis test script, which invokes tests/test_multibuild.sh... but that script doesn't pass for me locally on a clean checkout. Any pointers on where I should looking?

@radarhere
Copy link
Collaborator

Most projects have migrated to cibuildwheel instead, I wonder who is still using multibuild.

Just my two cents - cibuildwheel doesn't have the ability to build dependencies like library_builders.sh, so at the moment, Pillow is still using that.

@radarhere radarhere changed the title Add support for macOS 11.0 as a target. Add support for macOS 11.0 as a target Oct 21, 2024
@mattip
Copy link
Collaborator

mattip commented Oct 21, 2024

I guess the addtional CI and testing should be done in a separate PR. I am happy to put this in as-is now if it solves the macos/IOS problem, which it does, right?

@mattip
Copy link
Collaborator

mattip commented Oct 21, 2024

See #530. I think the macos matrix entry needs this PR to succeed, since it is running on arm64?

@mattip
Copy link
Collaborator

mattip commented Oct 21, 2024

Rebased on top of #530 to get CI testing

@mattip
Copy link
Collaborator

mattip commented Oct 21, 2024

Any idea why build_openssl fails to identify the proper platform macro? This is in openssl crypto/arm_arch.h.

@freakboy3742
Copy link
Contributor Author

Any idea why build_openssl fails to identify the proper platform macro?

The error is consistent with something is adding -arch x86_64 to CFLAGS prior to invoking the ./config script for OpenSSL.

Digging into the code - it looks like that's exactly what is happening:

If I compile locally (with no CFLAGS defined) the compilation lines read:

cc  -I. -Iinclude -fPIC -arch arm64 -O3 ...

but the log indicates compilations of:

cc  -I. -Iinclude -fPIC -arch arm64 -arch x86_64 -fPIC ...

That means cc is trying to build a universal binary, which won't work. Universal builds need to work on both architectures being complied; you can include architecture-specific code, but all that code needs to be gated with #defines. OpenSSL's code isn't - it's being configured on an ARM64 machine, so it's generating ARM64 sources, which then won't compile for x86_64. AFAIK, there's no "universal build" option in OpenSSL's build configuration.

As for the cause - configure_build.sh is running with an explicit PLAT=x86_64, which adds -arch x86_64 to CFLAGS. This appears to be the direct consequence of specifying MB_PYTHON_OSX_VER=10.9 in the environment. However, nothing about this "preferred x86_64 architecture" context is being passed to the OpenSSL configure script.

I've pushed an update to the CI script that expands the test a little to do a separate macOS-13 and macOS-latest build - macOS-13 is the last x86_64 runner on Github. This seems to appease the openSSL build, but then breaks the webp build. I'm looking into that now.

I guess the more thorough test would be to build an x86_64 binary on ARM64 (and vice versa) - but that will require a more substantial set of changes to how ./config is being invoked.

@freakboy3742 freakboy3742 force-pushed the macos-cleanups branch 3 times, most recently from ba662c9 to 1a4a4c2 Compare October 22, 2024 03:43
@freakboy3742
Copy link
Contributor Author

The issue with the WebP build is an interesting one - it appears to be code rot. The test suite was building WebP 0.5.0, which is almost 10 years old. The error is being raised as a compiler warning on Linux; but the ARM compiler is surfaces the warning as an error. Bumping WebP to a more recent version (1.4.0, released in April this year) avoids the issue, presumably because the bug has been resolved.

A similar problem existed with hdf5 builds; hdf5 1.10.5 is also almost 10 years old, and was also generating compilation errors on x86_64.

I've bumped those two package versions (including a tweak to the download URL for hdf5).

@freakboy3742
Copy link
Contributor Author

There's one more test failure on macOS, caused because the existing code would only work on single digit python minor versions :-)

Co-authored-by: Andrew Murray <[email protected]>
@freakboy3742
Copy link
Contributor Author

One more fix - the python installation tests contained an assertion that won't be assertable on any Python install since 3.8. Recent macOS Python installers are always universal2, not platform specific, so asserting that the macOS SDK version and architecture matches something reverse engineered from environment variables doesn't make much sense.

@mattip
Copy link
Collaborator

mattip commented Oct 22, 2024

Thanks for all these fixes. I do wonder if any of those utilities are used in the wild anymore but who knows. Unfortunately there is no straightforward way to deprecate code.

@mattip
Copy link
Collaborator

mattip commented Oct 22, 2024

By searching the log for RET=1 I see there are still 4 failing tests. two of them are a missing x in macosx:

+++(tests/test_osx_utils.sh:17): '[' python-3.9.3-macos11.pkg == python-3.9.3-macosx11.pkg ']'

The other two have to do with macpython_sdk_for_version, which doesn't make sense to support. Can we remove it (maybe in another PR for traceablility if we need to restore)? I think it would be prudent to, at least for this PR, to comment out the tests.

@freakboy3742
Copy link
Contributor Author

By searching the log for RET=1 I see there are still 4 failing tests. two of them are a missing x in macosx:

Yeah - I'm working through these at the moment. The testing process is going very slowly, because (a) the tests require the use of bash, which isn't the default shell on macOS any more, and (b) the full test suite requires sudo access, specific git configurations etc that are only reasonable in a CI environment. As a result, CI seems to be the only way I can run them all reliably.

The other two have to do with macpython_sdk_for_version, which doesn't make sense to support. Can we remove it (maybe in another PR for traceablility if we need to restore)? I think it would be prudent to, at least for this PR, to comment out the tests.

I think I'm almost at the point where I've got them working... If I can't wrap it up soonish, though, I'll push a "comment out the tests" PR for the ones I can't make work.

@mattip
Copy link
Collaborator

mattip commented Oct 22, 2024

OK. Cool, thanks for all this work. It probably exceeds your budgeted time by an order of magnitude or two, for that I am grateful.

I see macpython_sdk_for_version is used in travis_osx_steps.sh to set MB_PYTHON_OSX_VER. Maybe we could make setting MB_PYTHON_OSX_VER mandatory, and error if it is missing (again, in a separate PR).

It is also used in install_macpython via install_mac_cpython via pyinst_fname_for_version, but only as a fallback if MB_PYTHON_OSX_VER is not specified in install_macpython. I would imagine these days CI platforms have their own way of getting python versions, so install_macpython is anacronistic.

So my practicl question is what of all that does Pillow use in its CI?

@freakboy3742
Copy link
Contributor Author

OK. Cool, thanks for all this work. It probably exceeds your budgeted time by an order of magnitude or two, for that I am grateful.

No problems. Consider it a downpayment on the review that I'll be asking for in a bit to add iOS support :-)

I see macpython_sdk_for_version is used in travis_osx_steps.sh to set MB_PYTHON_OSX_VER. Maybe we could make setting MB_PYTHON_OSX_VER mandatory, and error if it is missing (again, in a separate PR).

Honestly, I've been pretty much ignoring the Travis config stuff, on the basis that travis is effectively dead as an OSS concern.

It is also used in install_macpython via install_mac_cpython via pyinst_fname_for_version, but only as a fallback if MB_PYTHON_OSX_VER is not specified in install_macpython. I would imagine these days CI platforms have their own way of getting python versions, so install_macpython is anacronistic.

I agree it's anachronistic; I'll leave the decision on whether to purge that code up to someone else. I think I've almost got it working (or, at least, passing the test suite) at this point.

So my practicl question is what of all that does Pillow use in its CI?

Pillow uses multibuild as a way to compile the binary libraries it depends on - libpng, libjpeg-turbo, and the like. Many of these have existing recipes (hence the giflib patch that I submitted); but the ones that don't fit into the same broad configure/cmake type build structure that the tools provided by multibuild are helpful.

@freakboy3742
Copy link
Contributor Author

Finally!

@mattip @radarhere CI is now passing on macOS x86_64, macOS ARM64, and Linux.

tests/test_osx_utils.sh Outdated Show resolved Hide resolved
Co-authored-by: Andrew Murray <[email protected]>
Copy link
Collaborator

@mattip mattip left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @freakboy3742 @radarhere

@mattip mattip merged commit d0023bb into multi-build:devel Oct 22, 2024
4 checks passed
@freakboy3742 freakboy3742 deleted the macos-cleanups branch October 22, 2024 21:31
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.

3 participants