Skip to content

Add support for MacOS with ARM CPUs (CMake + GitHub Action) #25

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

Merged
merged 38 commits into from
May 5, 2025

Conversation

siggmo
Copy link
Collaborator

@siggmo siggmo commented Oct 4, 2024

Closes #21

Add support for macOS.

Changes in CMakeLists.txt:

  • Apple-specific relative RPATH

New GitHub Actions workflow file for building and testing on macOS:

  • Using the macos-15 GitHub hosted runner (M1 CPU)
  • No container image, dependencies are installed directly in the workflow
  • Brew dependencies
  • gcc-14 C/C++ compiler (not CMake default on macOS)
  • Otherwise, more or less the same workflow file as for Ubuntu

@siggmo siggmo marked this pull request as draft October 4, 2024 15:20
@siggmo
Copy link
Collaborator Author

siggmo commented Oct 4, 2024

So far I got it to compile, so I guess the depencies are fine now. But running the tests raises some HDF5 related errors. I've never worked with HDF5 in C++, so maybe @sanathkeshav could you please have a look at the errors?

The tests fail with both hdf5 and hdf5-mpi brew packages. They can't be installed at the same time:

Error: Cannot install hdf5-mpi because conflicting formulae are installed.
  hdf5: because hdf5-mpi is a variant of hdf5, one can only use one or the other

@IshaanDesai IshaanDesai added the enhancement New feature or request label Oct 5, 2024
@@ -18,7 +18,6 @@ if (NOT HDF5_C_IS_PARALLEL)
message(FATAL_ERROR "Parallel HDF5 implementation (mpi) required but not found!")
endif()
find_dependency(Eigen3)
find_dependency(OpenMP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change seems to be unrelated to this pull request. Please revert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is part of the remove OpenMP dependency PR. I merged it into this PR to make it work temporarily.

@sanathkeshav
Copy link
Member

Since we could not come to a consensus on how to proceed with this issue, I would propose to close this PR at this point.
Please let me know what you think @siggmo @IshaanDesai

@siggmo
Copy link
Collaborator Author

siggmo commented Apr 14, 2025

Since we could not come to a consensus on how to proceed with this issue, I would propose to close this PR at this point. Please let me know what you think @siggmo @IshaanDesai

Fine for me. If you don't need the MacOS support at the moment, I don't think we need to invest more time here.

@IshaanDesai
Copy link
Collaborator

The macOS test fails with the errors:

/Users/runner/work/FANS/FANS/include/solver.h:364:68: error: use 'template' keyword to treat 'get' as a dependent template name
    const std::string &measure = reader.errorParameters["measure"].get<std::string>();
                                                                   ^
                                                                   template 
/Users/runner/work/FANS/FANS/include/solver.h:390:68: error: use 'template' keyword to treat 'get' as a dependent template name
    const std::string &error_type = reader.errorParameters["type"].get<std::string>();
                                                                   ^
                                                                   template 

which seems to have some solutions, for example as stated in this post. I am no expert in C++ templates, but could this error occur because we now enforce a C++ standard

set(CMAKE_CXX_STANDARD 14)

I would suggest trying to resolve the errors above, because having a macOS test is nice, and @siggmo already invested effort here, so it would be a pity to waste it now.

@sanathkeshav sanathkeshav added this to the 0.4.0 milestone May 2, 2025
@sanathkeshav
Copy link
Member

sanathkeshav commented May 2, 2025

Okay, now this works natively on my M1 Macbook, but it works because we are not using the clang compilers but gcc-14. This also involved compiling open-mpi itself with gcc-14 via source. This is painfully slow (takes ~11 mins in CI).

  • I think we should create a MacOS docker image and use that container for the CI like we do for jammy and noble.
  • We should also maybe go back to the conda recipe and enable MacOS support.

@IshaanDesai IshaanDesai marked this pull request as ready for review May 5, 2025 14:57
Copy link
Member

@sanathkeshav sanathkeshav left a comment

Choose a reason for hiding this comment

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

LGTM!
But we should put a pin on the Docker image for the macOS CI for later.

@IshaanDesai
Copy link
Collaborator

But we should put a pin on the Docker image for the macOS CI for later.

I will open an issue.

@IshaanDesai IshaanDesai merged commit 3b64e43 into develop May 5, 2025
8 checks passed
@IshaanDesai IshaanDesai deleted the feature/macOS_M1_support branch May 5, 2025 15:22
@sanathkeshav sanathkeshav mentioned this pull request May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for MacOS with ARM CPUs (CMake + GitHub Action)
4 participants