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] move R source files into R-package, reduce duplication in build_r.R #3087

Merged
merged 3 commits into from
May 25, 2020

Conversation

jameslamb
Copy link
Collaborator

In this PR, I propose moving the R-specific C++ source files into the R-package directory. For CRAN builds, it's necessary for those files to be at the root level of the R-package's src/ directory (not src/src/ like we currently use when we copy in files). R CMD BUILD automatically compiles any .c, .cc, or .cpp files it finds in src/.

I also think there's a nice organizational benefit to having as much of the R package's code inside R-package/ as possible.

This PR also proposes some simplifications to build_r.R. We were repeating the string literal "lightgbm_r" a lot, and I think it would be better to store it in one constant at the top of the script.

This is only moving files around and will not change anything for our users.

How this helps us get to CRAN

This is the next PR on the road to #629 . I'm working on #2960 in my fork. That PR includes build steps for the R package which only use the CRAN toolchain (e.g. no Visual Studio, no CMake).

That PR on my fork right now has a large diff that would be difficult to review, and it needs a bit more work. So I'd like to propose the first batch of changes here.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@jameslamb
Copy link
Collaborator Author

There are some other files that should be updated:
https://github.com/microsoft/LightGBM/blob/master/CMakeLists.txt
https://github.com/microsoft/LightGBM/blob/master/.github/CODEOWNERS

I don't think anything in CMakeLists.txt needs to be updated. The R-specific parts only get run once that file has been copied into an R package, for example lightgbm_r/ directory created by Rscript build_r.R. That is why all R builds are working right now on CI without touching CMakeLists.txt (except Mac on Travis, which is failing across all branches for unknown reasons). We already don't support building the library for R by running cmake from the repo root, so I don't think this breaks anything.

Good catch on CODEOWNERS! Didn't even think of that. Now we'll be able to eliminate these three lines in that file:

https://github.com/microsoft/LightGBM/blob/master/.github/CODEOWNERS#L18-L20

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 16, 2020

Currently looking into this cpplint warning.

R-package/src/lightgbm_R.cpp:8:  R-package/src/lightgbm_R.cpp should include its header file R-package/src/lightgbm_R.h  [build/include] [5]

Here's the relevant source code in cpplint: https://github.com/cpplint/cpplint/blob/6e3e178b031169be858ba6a3cf96d263a04c2b17/cpplint.py#L2213-L2237

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 16, 2020

Currently looking into this cpplint warning.

R-package/src/lightgbm_R.cpp:8:  R-package/src/lightgbm_R.cpp should include its header file R-package/src/lightgbm_R.h  [build/include] [5]

Here's the relevant source code in cpplint: https://github.com/cpplint/cpplint/blob/6e3e178b031169be858ba6a3cf96d263a04c2b17/cpplint.py#L2213-L2237

it was this: cpplint/cpplint#139

The fix is using "lightgbm_R.h" instead of "./lightgbm_R.h", which I did in 1e97aae

@jameslamb
Copy link
Collaborator Author

Ok I think this is ready! @Laurae2 or @guolinke I won't merge until one of you has a chance to review

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM!

Removing my "changes requested" review status.

@StrikerRUS StrikerRUS self-requested a review May 17, 2020 14:49
@StrikerRUS StrikerRUS dismissed their stale review May 17, 2020 14:51

Adrdessed

@jameslamb jameslamb merged commit 4d43e96 into microsoft:master May 25, 2020
@jameslamb jameslamb deleted the r/moving-files branch June 7, 2020 19:30
ChipKerchner pushed a commit to ChipKerchner/LightGBM that referenced this pull request Jun 10, 2020
… build_r.R (microsoft#3087)

* [R-package] move R source files into R-package

* fix linting warning

* stuff
ChipKerchner pushed a commit to ChipKerchner/LightGBM that referenced this pull request Jun 11, 2020
… build_r.R (microsoft#3087)

* [R-package] move R source files into R-package

* fix linting warning

* stuff
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants