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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 9, 2020
Merged

[R-package] [ci] Fix failures with R 3.6 and CMake (fixes #3469) #3541

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29d5674
[R-package] Fix failures with R 3.6 and CMake
jameslamb Nov 8, 2020
4e21bbd
min version
jameslamb Nov 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,37 @@
OPTION(USE_MPI "Enable MPI-based parallel learning" OFF)
OPTION(USE_OPENMP "Enable OpenMP" ON)
OPTION(USE_GPU "Enable GPU-accelerated training" OFF)
OPTION(USE_SWIG "Enable SWIG to generate Java API" OFF)
OPTION(USE_HDFS "Enable HDFS support (EXPERIMENTAL)" OFF)
OPTION(USE_TIMETAG "Set to ON to output time costs" OFF)
OPTION(USE_CUDA "Enable CUDA-accelerated training (EXPERIMENTAL)" OFF)
OPTION(USE_DEBUG "Set to ON for Debug mode" OFF)
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(__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)
elseif(__BUILD_FOR_R)
cmake_minimum_required(VERSION 3.6)
else()
cmake_minimum_required(VERSION 3.0)
endif()

if(__BUILD_FOR_R)
set(CMAKE_TRY_COMPILE_TARGET_TYPE "STATIC_LIBRARY")
Copy link
Collaborator

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.

LightGBM/CMakeLists.txt

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()

Copy link
Collaborator Author

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.

Copy link
Collaborator

@StrikerRUS StrikerRUS Nov 9, 2020 •
edited
Loading

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

Copy link
Collaborator Author

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.

endif(__BUILD_FOR_R)

if(USE_CUDA)
Copy link
Collaborator Author

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

PROJECT(lightgbm LANGUAGES C CXX CUDA)
else()
PROJECT(lightgbm LANGUAGES C CXX)
endif()

OPTION(USE_MPI "Enable MPI-based parallel learning" OFF)
OPTION(USE_OPENMP "Enable OpenMP" ON)
OPTION(USE_GPU "Enable GPU-accelerated training" OFF)
OPTION(USE_SWIG "Enable SWIG to generate Java API" OFF)
OPTION(USE_HDFS "Enable HDFS support (EXPERIMENTAL)" OFF)
OPTION(USE_TIMETAG "Set to ON to output time costs" OFF)
OPTION(USE_CUDA "Enable CUDA-accelerated training (EXPERIMENTAL)" OFF)
OPTION(USE_DEBUG "Set to ON for Debug mode" OFF)
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(APPLE)
OPTION(APPLE_OUTPUT_DYLIB "Output dylib shared library" OFF)
endif(APPLE)
Expand Down