-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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] adding routine registration in R package (fixes #1910) #2911
[R-package] adding routine registration in R package (fixes #1910) #2911
Conversation
To disable this you should have
|
hey thank you! And sorry, I didn't mean to post this on this PR and notify you. Meant to leave it as a note for myself on another PR I'm working on in my fork. Too many tabs open 😬 |
1b41a35
to
86f271d
Compare
This PR has been rebased to |
With these changes plus the other changes merged in over the last few days, (0 / 0 / 0) my mac (`gcc`, macOS 10.14)
(0/0/0) Azure DevOps linux ( Some of these logs have 1 or two NOTEs, but they're not actually issues that will show up on CRAN. If you test a package that doesn't exist on CRAN today, you get a "hey this is a new package or new maintainer" note. The other one I see is about some |
This is awesome! 🎉 Also, I remember there are some NOTEs about the latest release date:
Maybe we should start counting NOTEs at CI and allow only 2? |
Good point about the other NOTEs, we may encounter those at some point. I agree, we should start to only allow 2 so we don't have things like #2928 pop back up. From what I've seen so far on #2936 we have two other NOTEs that are Windows-specific that I'll fix in a followup PR (#2936 (comment)). I'm going to add these and some other comments on the current state to #629 since we're tracking progress there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we exclude lightgbm_R.cpp
from VS solution?
@StrikerRUS I think we should. I've been experimenting with MSBuild on my fork, trying to add something like the approach in the Python package but struggling with it. I think we should not try to build the R package with MSBuild (just sticking to MinGW or MSVC bundled with Visual Studio). I'll remove it from those files. |
@jameslamb |
Yeah that's right. Like this approach in the Python package: https://github.com/microsoft/LightGBM/blob/master/python-package/setup.py#L135-L148 Sorry, not that familiar with |
f1d7604
to
daa9da5
Compare
just rebased to |
Seems that corresponding commit was lost. |
ah! will fix it |
daa9da5
to
820ea9b
Compare
Co-Authored-By: Nikita Titov <[email protected]>
820ea9b
to
1666f67
Compare
Thanks for the reviews! Going to merge since CI checks are passing and I think I've addressed all remaining comments. Now that this is merged, everyone building |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
NOTE: This PR is a work in progress. I branched off of #2901 . The diff for this PR will get smaller once that PR is merged.Now that #2901 has been merged, this PR is ready for review!
This fixes this
R CMD CHECK
note:Changes in this PR:
CMakeLists.txt
to not even compile lightgbm_R.cpp in builds not meant for the R package. Non-R users will get slightly faster builds / smaller libraries, and we don't have to annotate different parts oflightgbm_R.cpp
with#ifdef LGB_R_BUILD
R_registerRoutines
andR_useDynamicSymbols
I was very pleasantly surprised to learn that we could address this without changing
LGBM_SE
toSEXP
everywhere 😀 . Maybe we'll decide to do that in the future to deal with whatever changes R 4.0.0 introduces.All the entries in
R_CallMethodDef CallEntries
were created with this Python script. I don't think it should be checked into the repo, as future updates to this should be small, such as those we'd need to do for #2850 .get-signatures.py (click me)