-
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] added command to remove build/ dir in install.libs.R #2909
Conversation
file.copy(src, dest, overwrite = TRUE) | ||
} else { | ||
stop(paste0("Cannot find lib_lightgbm", SHLIB_EXT)) | ||
} | ||
|
||
# clean up the "build" directory | ||
if (dir.exists(build_dir)) { |
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.
build_dir
variable is not defined when use_precompile == TRUE
, isn't it?
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.
😬 yep you're right! Let me fix that
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.
Moved it up in 56635a7. This also exposed that that setwd(source_dir)
was unnecessary.
thanks!
if (dir.exists(build_dir)) { | ||
print("Removing 'build/' directory") | ||
unlink( | ||
x = file.path(R_PACKAGE_SOURCE, "src", "build") |
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.
I think it will be clearer:
x = file.path(R_PACKAGE_SOURCE, "src", "build")
-> x = file.path(build_dir)
(Suggestions are disabled after merging a PR.)
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.
blegh sorry, yeah you're right. That would eliminate a source of duplication. Let me add that right now.
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. |
Another piece of adding Windows CI for the R package (#2335) and, as a result, #629
This PR fixes this Windows-specific NOTE that shows up in
R CMD check --as-cran
:R does have a method for cleaning up intermediate files, with a
cleanup
script (reference). I tried that first, but it didn't work. I think those scripts are used inR CMD build
and not run immediately after installation, which is why adding them with content likerm -r src/build
did not work.