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

[cmake] [R-package] include R-for-macOS vendored libs dir in OpenMP search path (fixes #6628) #6629

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Aug 29, 2024

Fixes #6628

The background for this is described in much greater detail in #6628 (comment).

In short:

  • {data.table} recently put up a new release (v1.16.0)
  • CRAN's macOS binaries for that release are linked to libomp.dylib
  • the way that linking was done causes {data.table} and a CMake + clang version of {lightgbm} to load 2 different copies of libomp.dylib, causing segfaults
  • as a result, R + CMake + clang macOS builds of {lightgbm} are broken

This fixes it by ensuring that the place where R vendors third-party libraries is searched for libomp.dylib first when library(lightgbm) is called (so that the same libraries used by CRAN will be found).

Notes for Reviewers

Other relevant discussions:

@jameslamb jameslamb changed the title WIP: [cmake] [R-package] include R-for-macOS vendored libs dir in OpenMP search path (fixes #6628) [cmake] [R-package] include R-for-macOS vendored libs dir in OpenMP search path (fixes #6628) Aug 29, 2024
@jameslamb jameslamb marked this pull request as ready for review August 29, 2024 06:06
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the detailed description! Based on it everything looks good to me!
I left only one stylish suggestion, feel free to merge without applying it,

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -186,9 +187,16 @@ execute_process(
OUTPUT_VARIABLE LIBR_INCLUDE_DIRS
)

# ask R for the lib dir
execute_process(
COMMAND ${LIBR_EXECUTABLE} "--slave" "--vanilla" "-e" "cat(normalizePath(R.home('lib'), winslash='/'))"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh wow! Ok good, that makes me feel more confident in this 😁

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@jameslamb jameslamb merged commit 16c12ef into master Aug 30, 2024
44 checks passed
@jameslamb jameslamb deleted the fix/r-macos-rpath branch August 30, 2024 14:18
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.

[ci] [R-package] macOS clang CMake jobs failing with segfaults
2 participants