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

Merged
merged 3 commits into from
Oct 23, 2024
Merged

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.

@larshg
Copy link
Contributor

larshg commented Oct 19, 2024

Could we adopt the way they do it from lighthouse - ie https://github.com/microsoft/LightGBM/blob/v4.5.0/CMakeLists.txt#L161-L183.

@larshg
Copy link
Contributor

larshg commented Oct 20, 2024

Looks good and OpenMp is found again on CIs 👍

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Thank you!

@mvieth mvieth merged commit 56a3172 into PointCloudLibrary:master Oct 23, 2024
13 checks passed
@cybaol cybaol deleted the fix-openmp-on-macos branch October 24, 2024 01:46
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.

4 participants