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 15 commits
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
4 changes: 4 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,11 @@ if(WIN32 AND (MINGW OR CYGWIN))
endif()

if(BUILD_FOR_R)
if(MSVC)
TARGET_LINK_LIBRARIES(_lightgbm ${R_MSVC_CORE_LIBRARY})
else()
TARGET_LINK_LIBRARIES(_lightgbm ${LIBR_CORE_LIBRARY})
endif(MSVC)
endif(BUILD_FOR_R)

install(TARGETS lightgbm _lightgbm
Expand Down
45 changes: 25 additions & 20 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 @@ -9,8 +9,8 @@
# LIBR_FOUND
# LIBR_HOME
# LIBR_EXECUTABLE
# R_MSVC_CORE_LIBRARY
# 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 @@ -27,6 +27,7 @@ if(NOT ("${R_ARCH}" STREQUAL "x64"))
endif()

# Creates R.lib and R.def in the build directory for linking with MSVC
# https://docs.microsoft.com/en-us/cpp/build/reference/link-input-files?redirectedfrom=MSDN&view=vs-2019
function(create_rlib_for_msvc)

message("Creating R.lib and R.def")
Expand All @@ -36,8 +37,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 @@ -48,14 +49,16 @@ function(create_rlib_for_msvc)
\nDo you have Rtools installed with its MinGW's bin/ in PATH?")
endif()

set(R_MSVC_CORE_LIBRARY "${CMAKE_CURRENT_BINARY_DIR}/R.lib" CACHE PATH "R.lib file location")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sorry I was mistaken in my previous comment about the name. It should follow naming style from the official dev guide and start with LIBR_ (not R_) like all other output variables.
https://cmake.org/cmake/help/v3.0/manual/cmake-developer.7.html#find-modules

Also, I find location a little bit misleading in terms that it refers to "directory", "place", IMHO.

All other changes look OK to me! Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok how about 8936a8a


# 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}
"--input-def" "${CMAKE_CURRENT_BINARY_DIR}/R.def"
"--output-lib" "${CMAKE_CURRENT_BINARY_DIR}/R.lib"
"--output-lib" "${R_MSVC_CORE_LIBRARY}"
)
endfunction(create_rlib_for_msvc)

Expand Down Expand Up @@ -168,19 +171,12 @@ 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
)

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")

# where is R.so / R.dll / libR.so likely to be found?
set(LIBR_PATH_HINTS "${CMAKE_CURRENT_BINARY_DIR}" "${LIBR_LIB_DIR}" "${LIBR_HOME}/bin/${R_ARCH}" "${LIBR_HOME}/bin" "${LIBR_LIBRARIES}")
set(LIBR_PATH_HINTS "${CMAKE_CURRENT_BINARY_DIR}" "${LIBR_HOME}/bin/${R_ARCH}" "${LIBR_HOME}/bin" "${LIBR_LIBRARIES}")

# look for the core R library
find_library(
Expand Down Expand Up @@ -213,10 +209,19 @@ endif()
# define find requirements
include(FindPackageHandleStandardArgs)

find_package_handle_standard_args(LibR DEFAULT_MSG
LIBR_HOME
LIBR_EXECUTABLE
LIBR_INCLUDE_DIRS
LIBR_LIB_DIR
LIBR_CORE_LIBRARY
)
if(WIN32 AND MSVC)
find_package_handle_standard_args(LibR DEFAULT_MSG
LIBR_HOME
LIBR_EXECUTABLE
LIBR_INCLUDE_DIRS
LIBR_CORE_LIBRARY
R_MSVC_CORE_LIBRARY
)
else()
find_package_handle_standard_args(LibR DEFAULT_MSG
LIBR_HOME
LIBR_EXECUTABLE
LIBR_INCLUDE_DIRS
LIBR_CORE_LIBRARY
)
endif()