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

Fix OpenMP on macOS #6114

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cybaol
Copy link
Contributor

@cybaol cybaol commented Aug 24, 2024

No description provided.

@mvieth
Copy link
Member

mvieth commented Aug 25, 2024

Hi, could you explain this a bit, please? Do you know why OpenMP is currently not found on macOS? Shouldn't the FindOpenMP.cmake script handle this? Hardcoding the path /usr/local/opt/libomp/... seems like a bad idea.

By the way, PCL currently has its own FindOpenMP.cmake file, but we are planning to remove that so that CMake's file of the same name is used instead: #6100 I don't know if that changes anything regarding this pull request, I just wanted to mention it.

@valgur
Copy link

valgur commented Aug 25, 2024

LightGBM does something similar to support the use of OpenMP via Homebrew: https://github.com/microsoft/LightGBM/blob/v4.5.0/CMakeLists.txt#L161-L183

I would suggest following their approach as it only uses Homebrew as a fallback and does not hard-code any paths.

One other suggestion would be to avoid the use of set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}") in favor of link_libraries(OpenMP::OpenMP_CXX), if possible. The latter makes it easier to provide OpenMP for Clang and AppleClang via a package manager without patching or workarounds. Any non-global linker and include paths cannot be reliably provided via the compiler flags alone.
Specifically, I'm currently working towards getting a better LLVM OpenMP support to Conan and Vcpkg to fill the gap and it would be helpful in that context:

@cybaol
Copy link
Contributor Author

cybaol commented Sep 20, 2024

libomp is keg-only, which means it was not symlinked into /usr/local,
because it can override GCC headers and result in broken builds.
So must specify manually.

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

Successfully merging this pull request may close these issues.

3 participants