[R-package] CMake fixes to support MSVC #2963
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StrikerRUS
reviewed
Mar 31, 2020
StrikerRUS
reviewed
Apr 5, 2020
Co-Authored-By: Nikita Titov <[email protected]>
StrikerRUS
approved these changes
Apr 6, 2020
There was a problem hiding this comment.
I'm OK with these changes. However, I'd suggest to wait resolving of this thread.
thank you! Absolutely agree. |
StrikerRUS
requested changes
Apr 6, 2020
Closed
jameslamb
added a commit
to jameslamb/LightGBM
that referenced
this pull request
Apr 15, 2020
jameslamb
added a commit
that referenced
this pull request
Apr 26, 2020
* first test of appveyor * strings are strings * lil bit of build script * fixing paths * removed unnecessary file * updated CRAN URL * added a lot more printing * fixing paths * more stuff * fixed paths * more stuff * more path guessing * even more paths * more stuff * moar logz * and now for something totally different * please work * ok could be a thing * changing directories * we might be in business * fixed install syntax * tryinv mingw * more mingw * ignore Suggests check * trying Azure DevOps * just run bare minimum for Azure DevOps * fixed build dir thing * trying to set libPaths * more testing * trying R 3.6.3 * R 3.6.3 * this feels right * still messing around with libraries * more paths * removed duplication in Windows testing code * simpler * fixed conda stuff * more conda stuff * more fixes * fixed testing script * moved AppVeyor setup to the top * commenting * ch-ch-ch-ch-chaaaanges * paths * plz work * fixed conda stuff in Windows CI * uncommented stuff to test a full build * fixed quotes and removed some unnecessary stuff * added install.libs.R change * quotes are impoortant * added commented-out stuff back in * added Windows script, download retries, and MSVC linking * minor fixes * cleaned up debugging code in FindLibR * cleaned up debugging code and moved R first in CI * fixed vsts-ci indentation * cut documentation stuff out of MSVC build * fix R CMD check for Azure * misc whitespace changes * Added echoing of build logs from R CMD check * cut out more documentation tests * fixed NOTE about imports from Matrix * moved some changes out of this PR and into #2963 * fixed whitespace stuff * added check on number of NOTES * adding better checks * fixing check on NOTEs * removing unnecessary variable * Update .ci/test_r_package_windows.ps1 Co-Authored-By: Nikita Titov <[email protected]> * some changes * fix quoting * trying MINGW on Azure DevOps * fixing paths * more paths * fixing paths * testing paths * fixing slashes * pinned CTAN mirror * get better logs * made sure Azure finds MinGW, fixed search for LIBR_CORE_LIBRARY, stopped building R docs on Azure * Apply suggestions from code review Co-Authored-By: Nikita Titov <[email protected]> * added CXX, CC for Windows builds and changed back to building docs on all MINGW builds * stored LIBR_CORE_LIBRARY hints in one variable * Apply suggestions from code review Co-Authored-By: Nikita Titov <[email protected]> * changes from code review * increased parallel builds for Azure CI * Apply suggestions from code review Co-Authored-By: Nikita Titov <[email protected]> Co-authored-by: Nikita Titov <[email protected]>
|
I updated this with the changes from #2936 today, then I tested this tonight on my Windows laptop and it is working (details below). @guolinke @StrikerRUS @Laurae2 as a reminder, I think we should merge it without waiting to have CI for Visual Studio. That will be done as part of #2965 . Right now we have multiple reports that Visual Studio builds of the R package are broken (#2970, #2995), so I think we should merge this ASAP. how I tested locally + the results
I ran the following command to install the R package. Rscript build_r.R >C:\Users\James\repos\LightGBM\log.txtA bit of the log went to the console, the rest to the logs. console logs
log.txtYou can see from the logs that this succeeded, and that it used MSVC (didn't fail back to Some issues have reported being able to install the library on Windows and then getting runtime errors, e.g. #2714 and #2970 , so I ran the unit tests to be sure. cd R-package/tests
Rscript testthat.R >C:\Users\James\repos\LightGBM\test-log.txtThe tests passed!
test-log.txt |
|
created #3025 to document the two compiler warnings seen in the logs from #2963 (comment) |
StrikerRUS
requested changes
Apr 27, 2020
Co-Authored-By: Nikita Titov <[email protected]>
StrikerRUS
reviewed
Apr 29, 2020
| @@ -10,7 +10,6 @@ | |||
| # LIBR_HOME | |||
| # LIBR_EXECUTABLE | |||
| # LIBR_INCLUDE_DIRS | |||
| # LIBR_LIB_DIR | |||
| # LIBR_CORE_LIBRARY | |||
| # and a CMake function to create R.lib for MSVC | |||
There was a problem hiding this comment.
I think R_DOT_LIB_FILE should be listed here and included in find_package_handle_standard_args conditionally.
Also just found the following guide: https://cmake.org/cmake/help/v3.0/manual/cmake-developer.7.html?highlight=find_package_handle_standard_args#find-modules.
UPD: Don't you find R_MSVC_CORE_LIBRARY name clearer?
There was a problem hiding this comment.
I liked R_DOT_LIB_FILE because it was so different from LIBR_CORE_LIBRARY that they were unlikely to be mistaken for each other.
However, I just changed it to R_MSVC_CORE_LIBRARY. I don't think the name is that important and I'm anxious to get this merged. MSVC installation for the R package is currently broken for users (#2963 (comment)) and I believe it has been for a few weeks now.
commit with the changes: c283c55
StrikerRUS
approved these changes
Apr 30, 2020
| @@ -48,14 +49,16 @@ function(create_rlib_for_msvc) | |||
| \nDo you have Rtools installed with its MinGW's bin/ in PATH?") | |||
| endif() | |||
|
|
|||
| set(R_MSVC_CORE_LIBRARY "${CMAKE_CURRENT_BINARY_DIR}/R.lib" CACHE PATH "R.lib file location") | |||
There was a problem hiding this comment.
I'm sorry I was mistaken in my previous comment about the name. It should follow naming style from the official dev guide and start with LIBR_ (not R_) like all other output variables.
https://cmake.org/cmake/help/v3.0/manual/cmake-developer.7.html#find-modules
Also, I find location a little bit misleading in terms that it refers to "directory", "place", IMHO.
All other changes look OK to me! Thanks!
There was a problem hiding this comment.
ok how about 8936a8a
|
@jameslamb Seems that this PR broke our builds for R docs. Can you please take a look?
Full logs (clickable): |
|
Just created #3045 . I don't think we should rely on comments in closed pull requests. I'll look 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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As of #2901 , when you build the R package the R core library now needs to be linked to
lib_lightgbm. As a result, I think that building the R package is broken for MSVC right now. We have code to create the .lib file for linking but aren't currently linking it.If you try to build the R package from the version on
masterwith MSVC you will get a linker error.In this PR, I propose adding that step to
CMakeLists.txt. I've also proposed some simplifications toFindLibR.cmake. It seems that the variableLIBR_LIB_DIRisn't actually necessary. It's found using the R codeR.home('lib')...there is nothing special about this code. I learned thatR.home(some_directory)returns a string with the value ofsome_directorytacked onto the end ofR.home(), regardless of whether or not that exists!Given that, we might as well just use
"${LIBR_HOME}/lib"directly in the search forLIBR_CORE_LIBRARYand not go through that extra step with R.The code from this PR is working on #2936 , and you can confirm that it works with MSVC in the Azure builds for that PR and with MinGW in the AppVeyor builds for that PR. You can confirm that these changes don't break linux and mac by looking at the Travis builds for that PR.
Splitting this out into its own PR to fix the MSVC problem quicker and to make #2936 easier to review.