Skip to content

fix apple silicon builds [ci build]#8

Merged
nmwsharp merged 1 commit into
nmwsharp:masterfrom
MarkForged:test-build
Dec 14, 2021
Merged

fix apple silicon builds [ci build]#8
nmwsharp merged 1 commit into
nmwsharp:masterfrom
MarkForged:test-build

Conversation

@brucedjones
Copy link
Copy Markdown
Contributor

The previous fix for apple silicon builds was not correctly configuring CMake to build for the correct target architecture. The approach implemented here patterns off of that used in parselmouth in which the matrix of build targets is manually configured so that the correct environment variables can be specified at run time. In particular CMAKE_OSX_ARCHITECTURES must be set so that CMake builds for the correct architecture.

This approach is a little awkward, I would expect CIBuildWheel to be updated so that it can do this for us. As noted by the robust-laplacian package maintainer, other build systems such as scikit-build already do this correctly.

The previous fix for apple silicon builds was not correctly configuring CMake to build for the correct target architecture. The approach implemented here patterns off of that used in [parselmouth](https://github.com/YannickJadoul/Parselmouth/blob/master/.github/workflows/wheels.yml) in which the matrix of build targets is manually configured so that the correct environment variables can be specified at run time. In particular `CMAKE_OSX_ARCHITECTURES` must be set so that CMake builds for the correct architecture.

This approach is a little awkward, I would expect CIBuildWheel to be updated so that it can do this for us. As noted by the robust-laplacian package maintainer, other build systems such as scikit-build already do this correctly.
@nmwsharp nmwsharp changed the title fix apple silicon builds fix apple silicon builds [ci build] Dec 14, 2021
@nmwsharp
Copy link
Copy Markdown
Owner

woah awesome, thank you!

@nmwsharp
Copy link
Copy Markdown
Owner

Hopefully this all works and then we can just accept this change :) I approved the build, I will also push a commit this branch to bump the version number which should make the actual builds run (if I have finally gotten the action configured right)

@nmwsharp
Copy link
Copy Markdown
Owner

I couldn't get the push-to-PR-branch thing to work on this PR for some reason, but I just duplicated this in another PR just get the CI to run. It's running in #10.

Hopefully all succeeds there and we can just accept this pull. If not, any subsequent pushes you make to this PR should trigger the builds, since it has [ci build] in the title now.

@brucedjones
Copy link
Copy Markdown
Contributor Author

Forgot to add: i manually inspected the binaries for 3 of the mac wheels and everything appears to check out

$ file robust_laplacian-0.2.3-cp310-cp310-macosx_10_9_universal2/robust_laplacian_bindings.cpython-310-darwin.so
robust_laplacian-0.2.3-cp310-cp310-macosx_10_9_universal2/robust_laplacian_bindings.cpython-310-darwin.so: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit bundle x86_64] [arm64:Mach-O 64-bit bundle arm64]
robust_laplacian-0.2.3-cp310-cp310-macosx_10_9_universal2/robust_laplacian_bindings.cpython-310-darwin.so (for architecture x86_64):	Mach-O 64-bit bundle x86_64
robust_laplacian-0.2.3-cp310-cp310-macosx_10_9_universal2/robust_laplacian_bindings.cpython-310-darwin.so (for architecture arm64):	Mach-O 64-bit bundle arm64

$ file robust_laplacian-0.2.3-cp310-cp310-macosx_11_0_arm64/robust_laplacian_bindings.cpython-310-darwin.so
robust_laplacian-0.2.3-cp310-cp310-macosx_11_0_arm64/robust_laplacian_bindings.cpython-310-darwin.so: Mach-O 64-bit bundle arm64

$ file robust_laplacian-0.2.3-cp38-cp38-macosx_10_9_x86_64/robust_laplacian_bindings.cpython-38-darwin.so
robust_laplacian-0.2.3-cp38-cp38-macosx_10_9_x86_64/robust_laplacian_bindings.cpython-38-darwin.so: Mach-O 64-bit bundle x86_64

@nmwsharp
Copy link
Copy Markdown
Owner

awesome, everything looks good to go. I will merge and update the public version. thanks again for sorting this out!

@nmwsharp nmwsharp merged commit 2231c85 into nmwsharp:master Dec 14, 2021
@nmwsharp
Copy link
Copy Markdown
Owner

When you get a chance, could you confirm that the new public version on pip works for you now?

@brucedjones
Copy link
Copy Markdown
Contributor Author

No problem, happy to help! Just tested locally via pip and everything looks good my end.

@nmwsharp
Copy link
Copy Markdown
Owner

wonderful, thanks! I'm going to mirror this fix in a couple other libraries now.

Btw do you a preferred twitter handle or anything? I was gonna tweet about this and give you a shout-out :) Otherwise I'd just use your github handle.

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