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] prevent symbol lookup conflicts (fixes #4045) #4155

Merged
merged 13 commits into from
Apr 30, 2021
Merged

[R-package] prevent symbol lookup conflicts (fixes #4045) #4155

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7cb47d0
[R-package] prevent symbol lookup conflicts
jameslamb Apr 2, 2021
791cf6c
add unit tests
jameslamb Apr 2, 2021
f2a584e
only run test on Windows
jameslamb Apr 2, 2021
cc3e6fc
Merge branch 'fix/r-registration' of github.com:microsoft/LightGBM
jameslamb Apr 22, 2021
8ed693a
Merge branch 'master' of github.com:microsoft/LightGBM
jameslamb Apr 22, 2021
e7b254d
Merge branch 'master' of github.com:microsoft/LightGBM
jameslamb Apr 25, 2021
6668390
move to .Call() calls
jameslamb Apr 25, 2021
a2ccac5
fix references
jameslamb Apr 25, 2021
f89aa2c
testing registration for CMake builds
jameslamb Apr 25, 2021
2d8de84
revert NAMESPACE changes
jameslamb Apr 25, 2021
42d01c8
revert testing changes
jameslamb Apr 25, 2021
d70071c
Merge branch 'master' into fix/r-registration
jameslamb Apr 29, 2021
ad229c7
Merge branch 'master' into fix/r-registration
jameslamb Apr 30, 2021
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
2 changes: 2 additions & 0 deletions R-package/src/lightgbm_R.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,8 @@ static const R_CallMethodDef CallEntries[] = {
{NULL, NULL, 0}
};

LIGHTGBM_C_EXPORT void R_init_lightgbm(DllInfo *dll);

void R_init_lightgbm(DllInfo *dll) {
R_registerRoutines(dll, NULL, CallEntries, NULL, NULL);
R_useDynamicSymbols(dll, FALSE);
Expand Down
17 changes: 17 additions & 0 deletions R-package/tests/testthat/test_registration.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
ON_WINDOWS <- .Platform$OS.type == "windows"

# this test is used to catch silent errors in routine registration,
# like the one documented in
# https://github.com/microsoft/LightGBM/issues/4045#issuecomment-812289182
test_that("lightgbm routine registration worked", {
testthat::skip_if_not(ON_WINDOWS)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this skip_if_not() is to work around this error I saw on Linux jobs in CI

* checking tests ...
  Running ‘testthat.R’
 � � ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure (test_registration.R:8:5): lightgbm routine registration worked ─────
  dll_info[["dynamicLookup"]] is not FALSE
  
  `actual` is NULL
  `expected` is a logical vector (FALSE)
  ── Error (test_registration.R:11:5): lightgbm routine registration worked ──────
  Error: no applicable method for 'getDLLRegisteredRoutines' applied to an object of class "NULL"
  Backtrace:
      █
   1. └─base::getDLLRegisteredRoutines(dll_info[["path"]]) test_registration.R:11:4
  
  [ FAIL 2 | WARN 0 | SKIP 3 | PASS 634 ]
  Error: Test failures
  Execution halted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah! The problem with this test isn't actually about operating system. This test will fail with an error like the one above on LightGBM's CMake-based builds and pass on CRAN builds. That's because, by using install.libs.R to build lib_lightgbm instead of R's built-in machinery, the routine registration stuff doesn't work.

Given that as of this PR the package will now use literal R symbols (e.g. .Call(LGBM_BoosterGetCurrentIteration_R,...), I don't think this test is necessary. Any failures in the registration will result in "object not found errors".

I'm going to remove this test, and will provide more explanation in a separate comment for how to make this work with CMake-based builds.


dll_info <- getLoadedDLLs()[["lightgbm"]]

# check that dynamic lookup has been disabled
expect_false(dll_info[["dynamicLookup"]])

# check that all the expected symbols show up
registered_routines <- getDLLRegisteredRoutines(dll_info[["path"]])[[".Call"]]
expect_gt(length(registered_routines), 20L)
})