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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 42 additions & 7 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ if(__BUILD_FOR_R)
find_package(LibR REQUIRED)
message(STATUS "LIBR_EXECUTABLE: ${LIBR_EXECUTABLE}")
message(STATUS "LIBR_INCLUDE_DIRS: ${LIBR_INCLUDE_DIRS}")
message(STATUS "LIBR_LIBS_DIR: ${LIBR_LIBS_DIR}")
message(STATUS "LIBR_CORE_LIBRARY: ${LIBR_CORE_LIBRARY}")
include_directories(${LIBR_INCLUDE_DIRS})
add_definitions(-DLGB_R_BUILD)
Expand Down Expand Up @@ -828,21 +829,55 @@ if(APPLE AND USE_OPENMP AND NOT BUILD_STATIC_LIB)
)
# add RPATH entries to ensure the loader looks in the following, in the following order:
#
# - (R-only) ${LIBR_LIBS_DIR} (wherever R for macOS stores vendored third-party libraries)
# - ${OpenMP_LIBRARY_DIR} (wherever find_package(OpenMP) found OpenMP at build time)
# - /opt/homebrew/opt/libomp/lib (where 'brew install' / 'brew link' puts libomp.dylib)
# - /opt/local/lib/libomp (where 'port install' puts libomp.dylib)
#

# with some compilers, OpenMP ships with the compiler (e.g. libgomp with gcc)
list(APPEND __omp_install_rpaths "${OpenMP_LIBRARY_DIR}")

# with clang, libomp doesn't ship with the compiler and might be supplied separately
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
list(
APPEND __omp_install_rpaths
"/opt/homebrew/opt/libomp/lib"
"/opt/local/lib/libomp"
)
# It appears that CRAN's macOS binaries compiled with -fopenmp have install names
# of the form:
#
# /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/lib/libomp.dylib
#
# That corresponds to the libomp.dylib that ships with the R framework for macOS, available
# from https://cran.r-project.org/bin/macosx/.
#
# That absolute-path install name leads to that library being loaded unconditionally.
#
# That can result in e.g. 'library(data.table)' loading R's libomp.dylib and 'library(lightgbm)' loading
# Homebrew's. Having 2 loaded in the same process can lead to segfaults and unpredictable behavior.
#
# This can't be easily avoided by forcing R-package builds in LightGBM to use R's libomp.dylib
# at build time... LightGBM's CMake uses find_package(OpenMP), and R for macOS only provides the
# library, not CMake config files for it.
#
# Best we can do, to allow CMake-based builds of the R-package here to continue to work
# alongside CRAN-prepared binaries of other packages with OpenMP dependencies, is to
# ensure that R's library directory is the first place the loader searches for
# libomp.dylib when clang is used.
#
# ref: https://github.com/microsoft/LightGBM/issues/6628
#
if(__BUILD_FOR_R)
list(PREPEND __omp_install_rpaths "${LIBR_LIBS_DIR}")
endif()
endif()
set_target_properties(
_lightgbm
PROPERTIES
BUILD_WITH_INSTALL_RPATH TRUE
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
# with clang, libomp doesn't ship with the compiler and might be supplied separately
INSTALL_RPATH "${OpenMP_LIBRARY_DIR};/opt/homebrew/opt/libomp/lib;/opt/local/lib/libomp;"
else()
# with other compilers, OpenMP ships with the compiler (e.g. libgomp with gcc)
INSTALL_RPATH "${OpenMP_LIBRARY_DIR}"
endif()
INSTALL_RPATH "${__omp_install_rpaths}"
INSTALL_RPATH_USE_LINK_PATH FALSE
)
endif()
Expand Down
10 changes: 10 additions & 0 deletions cmake/modules/FindLibR.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
# LIBR_EXECUTABLE
# LIBR_MSVC_CORE_LIBRARY
# LIBR_INCLUDE_DIRS
# LIBR_LIBS_DIR
# LIBR_CORE_LIBRARY
# and a CMake function to create R.lib for MSVC

Expand Down Expand Up @@ -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 😁

OUTPUT_VARIABLE LIBR_LIBS_DIR
)

set(LIBR_HOME ${LIBR_HOME} CACHE PATH "R home directory")
set(LIBR_EXECUTABLE ${LIBR_EXECUTABLE} CACHE PATH "R executable")
set(LIBR_INCLUDE_DIRS ${LIBR_INCLUDE_DIRS} CACHE PATH "R include directory")
set(LIBR_LIBS_DIR ${LIBR_LIBS_DIR} CACHE PATH "Where R stores vendored third-party libraries")

# where is R.so / R.dll / libR.so likely to be found?
set(
Expand Down Expand Up @@ -237,6 +245,7 @@ if(WIN32 AND MSVC)
LIBR_HOME
LIBR_EXECUTABLE
LIBR_INCLUDE_DIRS
LIBR_LIBS_DIR
LIBR_CORE_LIBRARY
LIBR_MSVC_CORE_LIBRARY
)
Expand All @@ -246,6 +255,7 @@ else()
LIBR_HOME
LIBR_EXECUTABLE
LIBR_INCLUDE_DIRS
LIBR_LIBS_DIR
LIBR_CORE_LIBRARY
)
endif()
Loading