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

[R-package] Use Rprintf for logging in the R package (fixes #1440, fixes #1909) #2901

Merged
merged 41 commits into from
Mar 24, 2020
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
d23e086
[R-package] started cutting over from custom R-to-C interface to R.h
jameslamb Feb 29, 2020
7e03224
replaced LGBM_SE with SEXP
jameslamb Mar 11, 2020
88b4950
fixed error about ocnflicting definitions of length
jameslamb Mar 11, 2020
f12eaa2
got linking working
jameslamb Mar 11, 2020
35e1b42
more stuff
jameslamb Mar 11, 2020
608f5a8
eliminated R CMD CHECK note about printing
jameslamb Mar 12, 2020
aa87eb6
switched from hard-coded include dir to the one from FindLibR.cmake
jameslamb Mar 12, 2020
11ea756
cleaned up formatting in FindLibR.cmake
jameslamb Mar 12, 2020
04b235b
commented-out everything in CI that does not touch R
jameslamb Mar 12, 2020
f54d9ac
more changes
jameslamb Mar 12, 2020
08756cd
trying to get better logs
jameslamb Mar 12, 2020
631e2ed
tried ignoring
jameslamb Mar 12, 2020
1c2a732
added error message to confirm a suspicion
jameslamb Mar 12, 2020
c915389
still trying to find R during R CMD CHECK
jameslamb Mar 12, 2020
eae206c
restore full CI
jameslamb Mar 12, 2020
6038fb3
fixed comment
jameslamb Mar 12, 2020
3e318d8
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 12, 2020
e0c4668
changed strategy for finding LIBR_HOME on Windows
jameslamb Mar 12, 2020
bd66c76
Removed 32-bit Windows stuff in FindLibR.cmake
jameslamb Mar 15, 2020
098b437
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 16, 2020
722b6bd
Update CMakeLists.txt
jameslamb Mar 17, 2020
36b8fa6
Update CMakeLists.txt
jameslamb Mar 17, 2020
cc35fff
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 17, 2020
6757692
removed some duplication in cmake scripts
jameslamb Mar 17, 2020
f54abcb
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 17, 2020
5bfab69
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 17, 2020
1e658db
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 17, 2020
0ad7299
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 17, 2020
7337dcb
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 17, 2020
a8a2bbe
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 17, 2020
7b30a19
added LIBR_CORE_LIBRARY back
jameslamb Mar 17, 2020
9adc7a9
small fixes to CMakeLists
jameslamb Mar 18, 2020
20f8452
simplified FindLibR.cmake
jameslamb Mar 18, 2020
4375584
some fixes for windows
jameslamb Mar 18, 2020
561ac4e
Apply suggestions from code review
jameslamb Mar 22, 2020
2403a61
allowed for directly passing LIBR_EXECUTABLE to FindLibR.cmake
jameslamb Mar 22, 2020
5b60e8e
reorganized FindLibR.cmake to catch more cases
jameslamb Mar 23, 2020
c9dd50d
clean up inconsistencies in R calls in FindLibR.cmake
jameslamb Mar 23, 2020
c00c928
Update R-package/src/cmake/modules/FindLibR.cmake
jameslamb Mar 23, 2020
20aeb82
removed unnecessary log messages
jameslamb Mar 23, 2020
ab870d8
removed unnecessary unset() call
jameslamb Mar 23, 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
13 changes: 13 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ if(USE_R35)
endif(USE_R35)

if(BUILD_FOR_R)
list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake/modules")
find_package(LibR REQUIRED)
message(STATUS "LIBR_HOME [${LIBR_HOME}]")
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
message(STATUS "LIBR_EXECUTABLE [${LIBR_EXECUTABLE}]")
message(STATUS "LIBR_INCLUDE_DIRS [${LIBR_INCLUDE_DIRS}]")
message(STATUS "LIBR_LIB_DIR [${LIBR_LIB_DIR}]")
message(STATUS "LIBR_CORE_LIBRARY [${LIBR_CORE_LIBRARY}]")
include_directories(${LIBR_INCLUDE_DIRS})
ADD_DEFINITIONS(-DLGB_R_BUILD)
endif(BUILD_FOR_R)

Expand Down Expand Up @@ -301,6 +309,11 @@ if(WIN32 AND (MINGW OR CYGWIN))
TARGET_LINK_LIBRARIES(_lightgbm IPHLPAPI)
endif()

if(BUILD_FOR_R)
TARGET_LINK_LIBRARIES(lightgbm ${LIBR_CORE_LIBRARY})
TARGET_LINK_LIBRARIES(_lightgbm ${LIBR_CORE_LIBRARY})
endif(BUILD_FOR_R)

install(TARGETS lightgbm _lightgbm
RUNTIME DESTINATION ${CMAKE_INSTALL_PREFIX}/bin
LIBRARY DESTINATION ${CMAKE_INSTALL_PREFIX}/lib
Expand Down
199 changes: 199 additions & 0 deletions R-package/src/cmake/modules/FindLibR.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
# CMake module used to find the location of R's
# dll and header files.
#
# Borrows heavily from xgboost's R package:
#
# * https://github.com/dmlc/xgboost/blob/master/cmake/modules/FindLibR.cmake
#
# Defines the following:
# LIBR_FOUND
# LIBR_HOME
# LIBR_EXECUTABLE
# LIBR_INCLUDE_DIRS
# LIBR_LIB_DIR
# LIBR_CORE_LIBRARY
# and a CMake function to create R.lib for MSVC

if(NOT R_ARCH)
if("${CMAKE_SIZEOF_VOID_P}" STREQUAL "4")
set(R_ARCH "i386")
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved
else()
set(R_ARCH "x64")
endif()
endif()

if(NOT ("${R_ARCH}" STREQUAL "x64"))
message(FATAL_ERROR "LightGBM's R package currently only supports 64-bit operating systems")
endif()

# Creates R.lib and R.def in the build directory for linking with MSVC
function(create_rlib_for_msvc)

message("Creating R.lib and R.def")

# various checks and warnings
if(NOT WIN32 OR NOT 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")
endif()

find_program(GENDEF_EXE gendef)
find_program(DLLTOOL_EXE dlltool)

if(NOT GENDEF_EXE OR NOT DLLTOOL_EXE)
message(FATAL_ERROR "Either gendef.exe or dlltool.exe not found!\
\nDo you have Rtools installed with its MinGW's bin/ in PATH?")
endif()

# extract symbols from R.dll into R.def and R.lib import library
execute_process(COMMAND ${GENDEF_EXE}
"-" "${LIBR_LIB_DIR}/R.dll"
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"
)
endfunction(create_rlib_for_msvc)

# R version information is used to search for R's libraries in
# the registry on Windows. Since this code is orchestrated by
# an R script (src/install.libs.R), that script uses R's built-ins to
# find the version of R and pass it through as a CMake variable
if(CMAKE_R_VERSION)
message("R version passed into FindLibR.cmake: ${CMAKE_R_VERSION}")
else()
message(FATAL_ERROR "Expected CMAKE_R_VERSION to be passed in but none was provided. Check src/install.libs.R")
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
endif()

# Find R executable
if(APPLE)

find_library(LIBR_LIBRARIES R)

if(LIBR_LIBRARIES MATCHES ".*\\.framework")
set(LIBR_HOME "${LIBR_LIBRARIES}/Resources")
set(LIBR_EXECUTABLE "${LIBR_HOME}/R")
else()
get_filename_component(_LIBR_LIBRARIES "${LIBR_LIBRARIES}" REALPATH)
get_filename_component(_LIBR_LIBRARIES_DIR "${_LIBR_LIBRARIES}" DIRECTORY)
set(LIBR_EXECUTABLE "${_LIBR_LIBRARIES_DIR}/../bin/R")
endif()
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved

# Unix
elseif(UNIX)
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved

# attempt to find R executable
if(NOT LIBR_EXECUTABLE)

# CRAN may run RD CMD CHECK instead of R CMD CHECK,
# which can lead to this infamous error:
# 'R' should not be used without a path -- see par. 1.6 of the manual
find_program(
LIBR_EXECUTABLE
NO_DEFAULT_PATH
HINTS "${CMAKE_CURRENT_BINARY_DIR}" "/usr/bin" "/usr/lib/" "/usr/local/bin/"
NAMES R R.exe
)
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved

if(LIBR_EXECUTABLE MATCHES ".*lightgbm\\.Rcheck.*")
message(FATAL_ERROR "If you are seeing this error, it means you are running R CMD check and R is using '${LIBR_EXECUTABLE}'.\
\nEdit src/cmake/modulesFindLibR.cmake and add your R path to HINTS near this error")
endif()
endif()

# Windows
else()

# if R executable not available, query R_HOME path from registry
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved
if(NOT LIBR_HOME)

# Try to find R's location in the registry
# ref: https://cran.r-project.org/bin/windows/base/rw-FAQ.html#Does-R-use-the-Registry_003f
get_filename_component(
LIBR_HOME
"[HKEY_LOCAL_MACHINE\\SOFTWARE\\R-core\\R\\${CMAKE_R_VERSION};InstallPath]"
ABSOLUTE
)

if(NOT LIBR_HOME)
get_filename_component(
LIBR_HOME
"[HKEY_CURRENT_USER\\SOFTWARE\\R-core\\R\\${CMAKE_R_VERSION};InstallPath]"
ABSOLUTE
)
endif()

if(NOT LIBR_HOME)
message(FATAL_ERROR "\nUnable to locate R executable.\
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
\nEither add its location to PATH or provide it through the LIBR_EXECUTABLE CMake variable")
endif()
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved

# set exe location based on R_ARCH
set(LIBR_EXECUTABLE "${LIBR_HOME}/bin/${R_ARCH}/R.exe")

endif()

endif()

if(NOT LIBR_EXECUTABLE)
message(FATAL_ERROR "Unable to locate R executable.\
\nEither add its location to PATH or provide it through the LIBR_EXECUTABLE CMake variable"
)
endif()
jameslamb marked this conversation as resolved.
Show resolved Hide resolved

# ask R for the home path
execute_process(
COMMAND ${LIBR_EXECUTABLE} "--slave" "--vanilla" "-e" "cat(normalizePath(R.home(), winslash='/'))"
OUTPUT_VARIABLE LIBR_HOME
)

# ask R for the include dir
execute_process(
COMMAND ${LIBR_EXECUTABLE} "--slave" "--no-save" "-e" "cat(normalizePath(R.home('include'), winslash='/'))"
OUTPUT_VARIABLE LIBR_INCLUDE_DIRS
)

# C:/PROGRA~1/R/R-36~1.1/include
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved

# ask R for the lib dir
execute_process(
COMMAND ${LIBR_EXECUTABLE} "--slave" "--no-save" "-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")
set(LIBR_CORE_LIBRARY ${LIBR_CORE_LIBRARY} CACHE PATH "R core shared library")

if(WIN32 AND MSVC)

# create a local R.lib import library for R.dll if it doesn't exist
if(NOT EXISTS "${CMAKE_CURRENT_BINARY_DIR}/R.lib")
create_rlib_for_msvc()
endif()

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
)
11 changes: 11 additions & 0 deletions R-package/src/install.libs.R
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,17 @@ if (!use_precompile) {
}
cmake_cmd <- paste0(cmake_cmd, " -DBUILD_FOR_R=ON ")

# Pass in R version, used to help find R executable for linking
R_version_string <- paste(
R.Version()[["major"]]
, R.Version()[["minor"]]
, sep = "."
)
cmake_cmd <- sprintf(
paste0(cmake_cmd, " -DCMAKE_R_VERSION='%s' ")
, R_version_string
)

# Check if Windows installation (for gcc vs Visual Studio)
if (WINDOWS) {
if (use_mingw) {
Expand Down
10 changes: 5 additions & 5 deletions include/LightGBM/R_object_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@

// 64bit pointer
#if INTPTR_MAX == INT64_MAX
typedef int64_t R_xlen_t;
typedef int64_t xlen_t;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to rename this type to avoid conflicts with R_xlen_t defined in R's headers.

#else
typedef int R_xlen_t;
typedef int xlen_t;
#endif

#else
Expand All @@ -58,7 +58,7 @@
unsigned int gccls : 3;
};

typedef int R_xlen_t;
typedef int xlen_t;
#endif // R_VER_ABOVE_35

struct lgbm_primsxp {
Expand Down Expand Up @@ -110,8 +110,8 @@ typedef struct LGBM_SER {
} LGBM_SER, *LGBM_SE;

struct lgbm_vecsxp {
R_xlen_t length;
R_xlen_t truelength;
xlen_t length;
xlen_t truelength;
};

typedef struct VECTOR_SER {
Expand Down
39 changes: 29 additions & 10 deletions include/LightGBM/utils/log.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
#include <iostream>
#include <stdexcept>

#ifdef LGB_R_BUILD
#define R_NO_REMAP
#define R_USE_C99_IN_CXX
#include <R_ext/Error.h>
#include <R_ext/Print.h>
#endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for those who are new to it:

  • R_NO_REMAP: Do not change symbols like Rf_error() to error()
  • R_USE_C99_IN_CXX: Make sure that Rvprintf() is defined. (reference)

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: R_USE_C99_IN_CXX is defined in R_ext/Print.h in R 4.0. Will this break when using R 4.0?

namespace LightGBM {

#if defined(_MSC_VER)
Expand Down Expand Up @@ -57,15 +64,13 @@ namespace LightGBM {
if ((pointer) == nullptr) LightGBM::Log::Fatal(#pointer " Can't be NULL at %s, line %d .\n", __FILE__, __LINE__);
#endif


enum class LogLevel: int {
Fatal = -1,
Warning = 0,
Info = 1,
Debug = 2,
};


/*!
* \brief A static Log class
*/
Expand Down Expand Up @@ -107,19 +112,33 @@ class Log {
vsprintf(str_buf, format, val);
#endif
va_end(val);
fprintf(stderr, "[LightGBM] [Fatal] %s\n", str_buf);
fflush(stderr);
throw std::runtime_error(std::string(str_buf));

// R code should write back to R's error stream,
// otherwise to stderr
#ifndef LGB_R_BUILD
Copy link
Contributor

Choose a reason for hiding this comment

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

We should agree on preprocessor directive indentations on a later PR. ping @guolinke @StrikerRUS @jameslamb for later.

fprintf(stderr, "[LightGBM] [Fatal] %s\n", str_buf);
fflush(stderr);
throw std::runtime_error(std::string(str_buf));
#else
Rf_error("[LightGBM] [Fatal] %s\n", str_buf);
#endif
}

private:
static void Write(LogLevel level, const char* level_str, const char *format, va_list val) {
if (level <= GetLevel()) { // omit the message with low level
// write to STDOUT
printf("[LightGBM] [%s] ", level_str);
vprintf(format, val);
printf("\n");
fflush(stdout);
// R code should write back to R's output stream,
// otherwise to stdout
#ifndef LGB_R_BUILD
printf("[LightGBM] [%s] ", level_str);
vprintf(format, val);
printf("\n");
fflush(stdout);
#else
Rprintf("[LightGBM] [%s] ", level_str);
Rvprintf(format, val);
Rprintf("\n");
#endif
}
}

Expand Down