Skip to content

[cmake] fixes static build for macos with OpenMP enabled (fixes #6601) #6600

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 4 commits into from
Aug 15, 2024

Conversation

Mottl
Copy link
Contributor

@Mottl Mottl commented Aug 10, 2024

Fixes #6601 with -DBUILD_STATIC_LIB=ON on macos with USE_OPENMP enabled

@Mottl
Copy link
Contributor Author

Mottl commented Aug 10, 2024

@microsoft-github-policy-service agree

@Mottl Mottl changed the title Fixes CMakeLists.txt for macos [cmake] fixes static build for macos with OpenMP enabled Aug 10, 2024
@StrikerRUS StrikerRUS added the fix label Aug 10, 2024
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks very much for this, and sorry for breaking builds of the static library on macOS. We probably need to add some CI coverage of that.

Could you please instead skip this entire block, by adding a NOT BUILD_STATIC_LIB into this condition?

if(APPLE AND USE_OPENMP)

All of this logic, like modifying RPATH entries, should be unnecessary for the static library.

@Mottl
Copy link
Contributor Author

Mottl commented Aug 10, 2024

Thanks James. Done.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

I pulled your branch onto my M2 mac and tested like this:

git remote add mottl [email protected]:Mottl/LightGBM.git
git fetch mottl macos_openmp_cmake_fix

cmake \
    -B build \
    -S . \
    -DBUILD_STATIC_LIB=ON \
    -DUSE_OPENMP=ON

cmake --build build --target all -j4

Saw everything build successfully, with 0 warnings or errors.

[ 89%] Built target lightgbm_objs
[ 97%] Linking CXX static library /Users/jlamb/repos/LightGBM/lib_lightgbm.a
[ 97%] Building CXX object CMakeFiles/lightgbm.dir/src/application/application.cpp.o
[ 97%] Building CXX object CMakeFiles/lightgbm.dir/src/main.cpp.o
[ 97%] Built target _lightgbm
[100%] Linking CXX executable /Users/jlamb/repos/LightGBM/lightgbm
[100%] Built target lightgbm

Also inspected the library a bit and everything looked correct.

ar -t ./lib_lightgbm.a
nm ./lib_lightgbm.a

@jameslamb jameslamb changed the title [cmake] fixes static build for macos with OpenMP enabled [cmake] fixes static build for macos with OpenMP enabled (fixes #6601) Aug 15, 2024
@jameslamb jameslamb merged commit 047c4fd into microsoft:master Aug 15, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build fails on macos with openmp and static lib enabled
3 participants