[R-package] fixed some minor issues with testing on Windows #2908
Conversation
| @@ -52,22 +57,28 @@ if (!use_precompile) { | |||
| build_cmd <- "mingw32-make.exe _lightgbm" | |||
| system(paste0(cmake_cmd, " ..")) # Must build twice for Windows due sh.exe in Rtools | |||
| } else { | |||
| try_vs <- 0L | |||
There was a problem hiding this comment.
Why this initialization was removed?
There was a problem hiding this comment.
This isn't necessary, I think maybe it is a misunderstanding about how system() works. You don't need to allocate an int and then fill it. system() will just return it.
There was a problem hiding this comment.
OK, so in R variable defined in nested scope remains visible in outer scope later, right?
There was a problem hiding this comment.
That's right. Python is the same way
for i in range(5):
x = i
if True:
print(x)If you think that isn't obvious though I can restore this! But I'd change it to 1L so that that default assumption is "VS didn't work" instead of "assume it worked"
There was a problem hiding this comment.
I see, thanks for the explanation!
I'd better leave it up to other reviewers.
There was a problem hiding this comment.
gently ping @guolinke
There was a problem hiding this comment.
I think it could be remvoed.
There was a problem hiding this comment.
great, thank you
|
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. |
I started working on #2335 tonight, specifically adding CI for Windows.
In this PR, I'd like to propose a few small fixes to start improving support on Windows.
.gitignore. These come from multi-architecture R package builds on Windows, and shouldn't be checked into source controlinstall.libs.R, added some logs and clarified existing logsI tested these changes locally on Windows 10, using Visual Studio 16 2019, with
Rscript build_r.R