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.

Sign up for GitHub

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

[R-package] CMake fixes to support MSVC #2963

Merged
merged 18 commits into from
May 2, 2020
Merged

[R-package] CMake fixes to support MSVC #2963

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e59b333
[R-package] CMake fixes to support MSVC
jameslamb Mar 31, 2020
5e87c31
simplifications
jameslamb Mar 31, 2020
8554e20
merged 'master' into r/msvc-support
jameslamb Apr 4, 2020
0124198
Apply suggestions from code review
jameslamb Apr 5, 2020
5820274
removed MSVC_RUNTIME_LIBRARY property
jameslamb Apr 12, 2020
40fa72d
Merge branch 'master' into r/msvc-support
jameslamb Apr 13, 2020
9a6d720
fixed FindLibR search for LIBR_CORE_LIBRARY
jameslamb Apr 14, 2020
25cb78a
merged changes from #2936
jameslamb Apr 26, 2020
e929c3f
merged changes
jameslamb Apr 26, 2020
8558ff4
Update CMakeLists.txt
jameslamb Apr 29, 2020
3ddcc15
made R.lib linking more explicit
jameslamb Apr 29, 2020
aa56d1b
add R_DOT_LIB_FILE to cache
jameslamb Apr 29, 2020
c283c55
changed R.lib CMake name
jameslamb Apr 29, 2020
bb5dcb8
fix placementt tof R_MSVC_CORE_LIBRARY
jameslamb Apr 30, 2020
5f156a3
empty commit
jameslamb Apr 30, 2020
45a7636
Merge branch 'master' into r/msvc-support
jameslamb May 1, 2020
8936a8a
change MSVC library name
jameslamb May 1, 2020
869d400
one more name change
jameslamb May 1, 2020
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
10 changes: 9 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,16 @@ if(WIN32 AND (MINGW OR CYGWIN))
endif()

if(BUILD_FOR_R)
TARGET_LINK_LIBRARIES(lightgbm ${LIBR_CORE_LIBRARY})
if(MSVC)
set_property(
TARGET _lightgbm
PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreadedDLL"
)
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
# https://docs.microsoft.com/en-us/cpp/build/reference/link-input-files?redirectedfrom=MSDN&view=vs-2019
TARGET_LINK_LIBRARIES(_lightgbm ${CMAKE_CURRENT_BINARY_DIR}/R.lib)
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved
else()
TARGET_LINK_LIBRARIES(_lightgbm ${LIBR_CORE_LIBRARY})
endif()
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
endif(BUILD_FOR_R)

install(TARGETS lightgbm _lightgbm
Expand Down
34 changes: 15 additions & 19 deletions R-package/src/cmake/modules/FindLibR.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
# LIBR_HOME
# LIBR_EXECUTABLE
# LIBR_INCLUDE_DIRS
# LIBR_LIB_DIR
# LIBR_CORE_LIBRARY
# and a CMake function to create R.lib for MSVC
Copy link
Collaborator

@StrikerRUS StrikerRUS Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think R_DOT_LIB_FILE should be listed here and included in find_package_handle_standard_args conditionally.
Also just found the following guide: https://cmake.org/cmake/help/v3.0/manual/cmake-developer.7.html?highlight=find_package_handle_standard_args#find-modules.

UPD: Don't you find R_MSVC_CORE_LIBRARY name clearer?

Copy link
Collaborator Author

@jameslamb jameslamb Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I liked R_DOT_LIB_FILE because it was so different from LIBR_CORE_LIBRARY that they were unlikely to be mistaken for each other.

However, I just changed it to R_MSVC_CORE_LIBRARY. I don't think the name is that important and I'm anxious to get this merged. MSVC installation for the R package is currently broken for users (#2963 (comment)) and I believe it has been for a few weeks now.

commit with the changes: c283c55


Expand All @@ -36,8 +35,8 @@ function(create_rlib_for_msvc)
message(FATAL_ERROR "create_rlib_for_msvc() can only be used with MSVC")
endif()

if(NOT EXISTS "${LIBR_LIB_DIR}")
message(FATAL_ERROR "LIBR_LIB_DIR, '${LIBR_LIB_DIR}', not found")
if(NOT EXISTS "${LIBR_CORE_LIBRARY}")
message(FATAL_ERROR "LIBR_CORE_LIBRARY, '${LIBR_CORE_LIBRARY}', not found")
endif()

find_program(GENDEF_EXE gendef)
Expand All @@ -50,7 +49,7 @@ function(create_rlib_for_msvc)

# extract symbols from R.dll into R.def and R.lib import library
execute_process(COMMAND ${GENDEF_EXE}
"-" "${LIBR_LIB_DIR}/R.dll"
"-" "${LIBR_CORE_LIBRARY}"
OUTPUT_FILE "${CMAKE_CURRENT_BINARY_DIR}/R.def"
)
execute_process(COMMAND ${DLLTOOL_EXE}
Expand Down Expand Up @@ -168,23 +167,21 @@ 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='/'))"
OUTPUT_VARIABLE LIBR_LIB_DIR
)

# look for the core R library
find_library(
LIBR_CORE_LIBRARY
NAMES R
HINTS "${CMAKE_CURRENT_BINARY_DIR}" "${LIBR_LIB_DIR}" "${LIBR_HOME}/bin" "${LIBR_LIBRARIES}"
)

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_LIB_DIR ${LIBR_LIB_DIR} CACHE PATH "R shared libraries directory")

# look for the core R library
if(WIN32)
set(LIBR_CORE_LIBRARY ${LIBR_HOME}/bin/${R_ARCH}/R.dll)
else()
find_library(
LIBR_CORE_LIBRARY
NAMES R
HINTS "${CMAKE_CURRENT_BINARY_DIR}" "${LIBR_HOME}/lib" "${LIBR_HOME}/bin/${R_ARCH}" "${LIBR_HOME}/bin" "${LIBR_LIBRARIES}"
)
endif()
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved

set(LIBR_CORE_LIBRARY ${LIBR_CORE_LIBRARY} CACHE PATH "R core shared library")

if(WIN32 AND MSVC)
Expand All @@ -203,6 +200,5 @@ find_package_handle_standard_args(LibR DEFAULT_MSG
LIBR_HOME
LIBR_EXECUTABLE
LIBR_INCLUDE_DIRS
LIBR_LIB_DIR
LIBR_CORE_LIBRARY
)