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] always name the shared library 'lightgbm', not 'lib_lightgbm' #6432

Merged
merged 7 commits into from
May 1, 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
13 changes: 12 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -461,11 +461,22 @@ if(BUILD_STATIC_LIB)
else()
add_library(_lightgbm SHARED)
endif()

# R expects libraries of the form <project>.{dll,dylib,so}, not lib_<project>.{dll,dylib,so}
if(__BUILD_FOR_R)
set_target_properties(
_lightgbm
PROPERTIES
PREFIX ""
OUTPUT_NAME "lightgbm"
)
endif()

# LightGBM headers include openmp, cuda, R etc. headers,
# thus PUBLIC is required for building _lightgbm_swig target.
target_link_libraries(_lightgbm PUBLIC lightgbm_capi_objs lightgbm_objs)

if(MSVC)
if(MSVC AND NOT __BUILD_FOR_R)
set_target_properties(_lightgbm PROPERTIES OUTPUT_NAME "lib_lightgbm")
endif()

Expand Down
2 changes: 1 addition & 1 deletion R-package/NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,4 @@ importFrom(parallel,detectCores)
importFrom(stats,quantile)
importFrom(utils,modifyList)
importFrom(utils,read.delim)
useDynLib(lib_lightgbm , .registration = TRUE)
useDynLib(lightgbm , .registration = TRUE)
2 changes: 1 addition & 1 deletion R-package/R/lightgbm.R
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ NULL
#' @import methods
#' @importFrom Matrix Matrix
#' @importFrom R6 R6Class
#' @useDynLib lib_lightgbm , .registration = TRUE
#' @useDynLib lightgbm , .registration = TRUE
NULL

# Suppress false positive warnings from R CMD CHECK about
Expand Down
6 changes: 3 additions & 3 deletions R-package/src/install.libs.R
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ if (!makefiles_already_generated) {
}

# build the library
message("Building lib_lightgbm")
message(paste0("Building lightgbm", SHLIB_EXT))
.run_shell_command(build_cmd, build_args)
src <- file.path(lib_folder, paste0("lib_lightgbm", SHLIB_EXT), fsep = "/")
src <- file.path(lib_folder, paste0("lightgbm", SHLIB_EXT), fsep = "/")

# Packages with install.libs.R need to copy some artifacts into the
# expected places in the package structure.
Expand All @@ -247,7 +247,7 @@ if (file.exists(src)) {
}

} else {
stop(paste0("Cannot find lib_lightgbm", SHLIB_EXT))
stop(paste0("Cannot find lightgbm", SHLIB_EXT))
}

# clean up the "build" directory
Expand Down
20 changes: 1 addition & 19 deletions build-cran-package.sh
Original file line number Diff line number Diff line change
Expand Up @@ -165,23 +165,6 @@ cd "${TEMP_R_DIR}"
-e 's/\.\..*fast_double_parser\.h/LightGBM\/fast_double_parser\.h/' \
src/include/LightGBM/utils/common.h

# When building an R package with 'configure', it seems
# you're guaranteed to get a shared library called
# <packagename>.so/dll/dylib. The package source code expects
# 'lib_lightgbm.so', not 'lightgbm.so', to comply with the way
# this project has historically handled installation
echo "Changing lib_lightgbm to lightgbm"
for file in R/*.R; do
sed \
-i.bak \
-e 's/lib_lightgbm/lightgbm/' \
"${file}"
done
sed \
-i.bak \
-e 's/lib_lightgbm/lightgbm/' \
NAMESPACE

# 'processx' is listed as a 'Suggests' dependency in DESCRIPTION
# because it is used in install.libs.R, a file that is not
# included in the CRAN distribution of the package
Expand All @@ -191,8 +174,7 @@ cd "${TEMP_R_DIR}"
DESCRIPTION

echo "Cleaning sed backup files"
rm R/*.R.bak
rm NAMESPACE.bak
rm *.bak

cd "${ORIG_WD}"

Expand Down
36 changes: 0 additions & 36 deletions build_r.R
Original file line number Diff line number Diff line change
Expand Up @@ -398,42 +398,6 @@ description_contents <- gsub(
)
writeLines(description_contents, DESCRIPTION_FILE)

# CMake-based builds can't currently use R's builtin routine registration,
# so have to update NAMESPACE manually, with a statement like this:
#
# useDynLib(lib_lightgbm, LGBM_DatasetCreateFromFile_R, ...)
#
# See https://cran.r-project.org/doc/manuals/r-release/R-exts.html#useDynLib for
# documentation of this approach, where the NAMESPACE file uses a statement like
# useDynLib(foo, myRoutine, myOtherRoutine)
NAMESPACE_FILE <- file.path(TEMP_R_DIR, "NAMESPACE")
namespace_contents <- readLines(NAMESPACE_FILE)
dynlib_line <- grep(
pattern = "^useDynLib"
, x = namespace_contents
)

c_api_contents <- readLines(file.path(TEMP_SOURCE_DIR, "src", "lightgbm_R.h"))
c_api_contents <- c_api_contents[startsWith(c_api_contents, "LIGHTGBM_C_EXPORT")]
c_api_contents <- gsub(
pattern = "LIGHTGBM_C_EXPORT SEXP "
, replacement = ""
, x = c_api_contents
, fixed = TRUE
)
c_api_symbols <- gsub(
pattern = "\\(.*"
, replacement = ""
, x = c_api_contents
)
dynlib_statement <- paste0(
"useDynLib(lib_lightgbm, "
, toString(c_api_symbols)
, ")"
)
namespace_contents[dynlib_line] <- dynlib_statement
writeLines(namespace_contents, NAMESPACE_FILE)

# NOTE: --keep-empty-dirs is necessary to keep the deep paths expected
# by CMake while also meeting the CRAN req to create object files
# on demand
Expand Down
Loading