arrow: Fix cross-compiling errors on macos #2798
Conversation
This fixes the issue with missing LZ4 symbols in the arm64 wheel first introduced in v3.1.1. The crux of the issue is that Arrow was building lz4 (and zstd) in a separate cmake build, and it does not specially pass on the `CMAKE_OSX_ARCHITECTURES` flag that was set when perspective-server's build script invokes cmake. Arrow's build does forward on the value of CMAKE_TOOLCHAIN_FILE to its external dependencies, if it's set, so this commit creates two minimal toolchain files and uses them in the Mac build to set `CMAKE_OSX_ARCHITECTURES` (and a couple other variables) correctly when cross-compiling. a good follow-up would be to see if we can enable Python tests on arm64 using a macos-14 runner, which is available for public repos: <https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories> Signed-off-by: Tom Jakubowski <tom@prospective.dev>
| // CMAKE_OSX_ARCHITECTURES and but it does forward on a | ||
| // CMAKE_TOOLCHAIN_FILE. In Conda builds, the environment sets | ||
| // `CMAKE_ARGS` up with various toolchain arguments. This block may need | ||
| // to be patched out or adjusted for Conda. |
There was a problem hiding this comment.
by this I mean this block may eventually need some if !conda_build style guards
There was a problem hiding this comment.
funny enough, the conda 3.1.1 macos cross-compiling builds were apparently unaffected -- their import tests succeeded. https://github.com/conda-forge/perspective-feedstock/runs/31853180964
I'm not exactly sure why they were unaffected, but have some leads. conda-build passes its toolchain arguments to us (in CMAKE_ARGS) with compilers/tools with arm64 in the name, -DCMAKE_C_COMPILER=arm64-…-cc -DCMAKE_LIBTOOL=arm64-…-cc, which I presume is a compiler toolchain which targets arm64 without needing any other flags, and doesn't set CMAKE_OSX_ARCHITECTURE itself. And Arrow does forward those specific flags on to its external dependencies
| # (Arrow does not as of Oct 2024 pass on CMAKE_OSX_ARCHITECTURES to its | ||
| # dependencies if it is set, but does pass on a toolchain file) | ||
|
|
||
| set(CMAKE_OSX_ARCHITECTURES "arm64" CACHE STRING "Target architecture for Mac builds") |
There was a problem hiding this comment.
don't think this needs the CACHE etc. If it's confusing I'm happy to try removing it
|
If it turns out we can't easily get arm64 tests running on macos-14, then a cheap though maybe overly specific regression test would be to run something like |
| Ok("x86_64") => "./cmake/toolchains/darwin-x86_64.cmake", | ||
| Ok("aarch64") => "./cmake/toolchains/darwin-arm64.cmake", | ||
| arch @ Ok(_) | arch @ Err(_) => { | ||
| panic!("Unknown PSP_ARCH value: {:?}", arch) |
There was a problem hiding this comment.
this might be the wrong behavior if PSP_ARCH isn't set (which I think is the case for local dev). I guess it should just not set a toolchain file when PSP_ARCH is unset? (but perhaps still error + bail out when PSP_ARCH is an unknown value)
|
Shoot, CI on this PR doesn't build Mac wheels so it can't be tested. I'll open another PR which enables the Mac builds so we can validate the arm64 wheel artifact. I did create a one-off proof of concept test repo using one of the free arm64 macos-14 runners. These are apparently very new, like released in the last couple weeks. Here's a run using the arm64 wheel from the When the arm64 wheel is built on my other PR, can test it on that same repo and verify that perspective can be imported. I don't feel up to battling the (build) matrix tonight so will follow up with another PR later to land a macos-14 tester in this repository. |
|
OK, grabbed an artifact from #2799. Did a test run with the wheel on an arm64 macos-14 runner here, it passes the pytest suite https://github.com/tomjakubowski/perspective-arm64-macos-wheel-test/actions/runs/11453537555/job/31866177154 |
This fixes the issue with missing LZ4 symbols in the arm64 wheel first introduced in v3.1.1.
The crux of the issue is that Arrow was building lz4 (and zstd) in a separate cmake build, and it does not specially pass on the
CMAKE_OSX_ARCHITECTURESflag that was set when perspective-server's build script invokes cmake.Arrow's build does forward on the value of CMAKE_TOOLCHAIN_FILE to its external dependencies, if it's set, so this commit creates two minimal toolchain files and uses them in the Mac build to set
CMAKE_OSX_ARCHITECTURES(and a couple other variables) correctly when cross-compiling.a good follow-up would be to see if we can enable Python tests on arm64 using a macos-14 runner, which is available for public repos: https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories
Pull Request Checklist
Discussions, this PR applies to.