-
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] [ci] Fix failures with R 3.6 and CMake (fixes #3469) #3541
Conversation
set(CMAKE_TRY_COMPILE_TARGET_TYPE "STATIC_LIBRARY") | ||
endif(__BUILD_FOR_R) | ||
|
||
if(USE_CUDA) |
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 moved this down here because set(CMAKE_TRY_COMPILE_TARGET_TYPE)
has to run before PROJECT()
, and I needed to access the result of OPTION(__BUILD_FOR_R)
for it
@jameslamb Wow! Just amazing! Can't believe you've find the root cause! Thank you very much! |
@@ -26,6 +20,16 @@ OPTION(BUILD_STATIC_LIB "Build static library" OFF) | |||
OPTION(__BUILD_FOR_R "Set to ON if building lib_lightgbm for use with the R package" OFF) | |||
OPTION(__INTEGRATE_OPENCL "Set to ON if building LightGBM with the OpenCL ICD Loader and its dependencies included" OFF) | |||
|
|||
if(__BUILD_FOR_R) | |||
set(CMAKE_TRY_COMPILE_TARGET_TYPE "STATIC_LIBRARY") |
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.
It seems that CMAKE_TRY_COMPILE_TARGET_TYPE
is available only in CMake >= 3.6.
https://cmake.org/cmake/help/v3.19/variable/CMAKE_TRY_COMPILE_TARGET_TYPE.html
We should add
if(__BUILD_FOR_R)
cmake_minimum_required(VERSION 3.6)
elseif(__INTEGRATE_OPENCL)
...
here, I believe.
Lines 1 to 9 in 71a1a4f
if(__INTEGRATE_OPENCL) | |
cmake_minimum_required(VERSION 3.11) | |
elseif(USE_GPU OR APPLE) | |
cmake_minimum_required(VERSION 3.2) | |
elseif(USE_CUDA) | |
cmake_minimum_required(VERSION 3.16) | |
else() | |
cmake_minimum_required(VERSION 3.0) | |
endif() |
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.
Oh ok sure! I just added it in 4e21bbd
I also moved all those checks under the OPTION()
calls. It seemed strange to me that we're doing things like IF(__INTEGRATE_OPENCL)
(for example) before OPTION(__INTEGRATE_OPENCL)
. I think that's a mistake. If you look at https://cmake.org/cmake/help/latest/command/if.html#condition-syntax, it says
if(<variable|string>)
True if given a variable that is defined to a value that is not a false constant. False otherwise. (Note macro arguments are not variables.)
I think that IF(THING_THAT_HAS_NOT_BEEN_DEFINED)
will always evaluate to false.
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 that
IF(THING_THAT_HAS_NOT_BEEN_DEFINED)
will always evaluate to false.
I'm not sure but it seems that we define "THING_THAT_HAS_NOT_BEEN_DEFINED
" during passing -DTHING_THAT_HAS_NOT_BEEN_DEFINED
CMake flag. For example, I remember everything was working OK even without OPTION
at all: #3144 (comment).
This option may be used to specify a setting that takes priority over the project’s default value
https://cmake.org/cmake/help/v3.0/manual/cmake.1.html#options
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.
Right, makes sense. I think we just got lucky that all options that were used in code before the OPTION()
calls have default value OFF
, which behaves the same as it not being defined at all.
Anyway, I'm glad that they're all moved to the top now.
So, the reason our builds started to fail was CMake version update on GitHub Actions image, right? |
I actually don't know for sure! By "isolate", I mean that I was able to track down that the error came from the I don't know for sure if the issue was caused by the |
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'm pretty confident that this fixes our persistent CI issues with the R package (#3469)!!!
As a reminder, the CI job
r-package (windows-latest, MINGW, R 3.6, cmake)
often fails with this error:I was able to isolate this tonight, and found it happens during the
PROJECT()
step inCMakeLists.txt
. I don't know why this sometimes happens and sometimes doesn't with the MINGW compilers from Rtools 3.5, but I think I found a permanent fix for it, based on this very good 2018 Stack Overflow answer from @Kamilcuk 😀With this fix, the
r-package (windows-latest, MINGW, R 3.6, cmake)
succeeded 12 consecutive times in a PR on my fork. So I'm pretty confident this fixes it!