-
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
[ci] Require CMake 3.28 and replace FetchContent_Populate
with FetchContent_MakeAvailable
#6550
Conversation
if(WIN32) | ||
set(USE_DYNAMIC_VCXX_RUNTIME ON) | ||
endif() | ||
add_subdirectory(${opencl-icd-loader_SOURCE_DIR} ${opencl-icd-loader_BINARY_DIR} EXCLUDE_FROM_ALL) |
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.
At the moment, FetchContent_MakeAvailable performs add_subdirectory() on a CMake-using project's root.
https://gitlab.kitware.com/cmake/cmake/-/issues/22904
cmake/IntegratedOpenCL.cmake
Outdated
message(STATUS "Populated OpenCL Headers") | ||
endif() | ||
set(OPENCL_ICD_LOADER_HEADERS_DIR ${opencl-headers_SOURCE_DIR} CACHE PATH "") # for OpenCL ICD Loader | ||
set(OpenCL_INCLUDE_DIR ${opencl-headers_SOURCE_DIR} CACHE PATH "") # for Boost::Compute | ||
|
||
FetchContent_Declare(OpenCL-ICD-Loader GIT_REPOSITORY ${OPENCL_LOADER_REPOSITORY} GIT_TAG ${OPENCL_LOADER_TAG}) | ||
if (CMAKE_VERSION VERSION_GREATER_EQUAL 3.28) |
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.
Or we can bump minimum required version.
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 support just increasing the project-wide minimum CMake version (in both CMakeLists.txt
and pyproject.toml
) to 3.28.
I like this advice from https://cliutils.gitlab.io/modern-cmake/chapters/intro/installing.html.
Your CMake version should be newer than your compiler. It should be newer than the libraries you are using (especially Boost). New versions work better for everyone.
If you have a built in copy of CMake, it isn't special or customized for your system. You can easily install a new one instead, either on the system level or the user level. Feel free to instruct your users here if they complain about a CMake requirement being set too high.
It's true that 3.28.0 has only been out for 8 months (https://github.com/Kitware/CMake/releases/tag/v3.28.0), but it's already in wide use.
- for Mac users, Homebrew and macports already have v3.30.0
- for Windows users, Chocolatey has v3.30.0 (link) and CMake official binaries have v3.30.0 (link)
The biggest disruption would be for Linux users. If you use apt-get install cmake
on Ubuntu 22.04 today, you'll get v3.22.1. See the table at the bottom of https://cliutils.gitlab.io/modern-cmake/chapters/intro/installing.html or at https://repology.org/project/cmake/versions.
Linux users on distros with older versions would have to either install it a different way (like with pip
or conda
) or like this:
LightGBM/docker/dockerfile-cli
Lines 19 to 22 in f8ec57b
RUN curl -L -o cmake.sh https://github.com/Kitware/CMake/releases/download/v3.29.2/cmake-3.29.2-linux-x86_64.sh && \ | |
chmod +x cmake.sh && \ | |
sh ./cmake.sh --prefix=/usr/local --skip-license && \ | |
rm cmake.sh |
I don't think that's too bad... especially since to build the Python package specifically, scikit-build-core
will handle automatically pip
-installing cmake
if a new-enough version is not found.
And I don't think this would just be for the benefit of integrated OpenCL wheels... there are lots of other benefits we'd get by upgrading to a newer minimum version. See https://cliutils.gitlab.io/modern-cmake/chapters/intro/newcmake.html.
If you feel that this is too much of a risk, then I'd support changing the minimum in CMakeLists.txt
conditionally, like
if(__INTEGRATE_OPENCL)
cmake_minimum_required(VERSION 3.18)
else()
cmake_minimum_required(VERSION 3.28)
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.
Thanks a lot for your opinion and especially for the link to short version of CMake Changelog!
scikit-build-core
will handle automaticallypip
-installingcmake
if a new-enough version is not found.
Cool feature of scikit-build-core
! 👍
I'll go ahead and bump CMake version. Due to the lack of working hands here, I think it's OK to bump version unconditionally.
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.
Thanks for fixing this!
I think we should increase the minimum CMake version instead of having different codepaths for different versions. I left 2 different forms of that suggestion in a comment here.
cmake/IntegratedOpenCL.cmake
Outdated
message(STATUS "Populated OpenCL Headers") | ||
endif() | ||
set(OPENCL_ICD_LOADER_HEADERS_DIR ${opencl-headers_SOURCE_DIR} CACHE PATH "") # for OpenCL ICD Loader | ||
set(OpenCL_INCLUDE_DIR ${opencl-headers_SOURCE_DIR} CACHE PATH "") # for Boost::Compute | ||
|
||
FetchContent_Declare(OpenCL-ICD-Loader GIT_REPOSITORY ${OPENCL_LOADER_REPOSITORY} GIT_TAG ${OPENCL_LOADER_TAG}) | ||
if (CMAKE_VERSION VERSION_GREATER_EQUAL 3.28) |
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 support just increasing the project-wide minimum CMake version (in both CMakeLists.txt
and pyproject.toml
) to 3.28.
I like this advice from https://cliutils.gitlab.io/modern-cmake/chapters/intro/installing.html.
Your CMake version should be newer than your compiler. It should be newer than the libraries you are using (especially Boost). New versions work better for everyone.
If you have a built in copy of CMake, it isn't special or customized for your system. You can easily install a new one instead, either on the system level or the user level. Feel free to instruct your users here if they complain about a CMake requirement being set too high.
It's true that 3.28.0 has only been out for 8 months (https://github.com/Kitware/CMake/releases/tag/v3.28.0), but it's already in wide use.
- for Mac users, Homebrew and macports already have v3.30.0
- for Windows users, Chocolatey has v3.30.0 (link) and CMake official binaries have v3.30.0 (link)
The biggest disruption would be for Linux users. If you use apt-get install cmake
on Ubuntu 22.04 today, you'll get v3.22.1. See the table at the bottom of https://cliutils.gitlab.io/modern-cmake/chapters/intro/installing.html or at https://repology.org/project/cmake/versions.
Linux users on distros with older versions would have to either install it a different way (like with pip
or conda
) or like this:
LightGBM/docker/dockerfile-cli
Lines 19 to 22 in f8ec57b
RUN curl -L -o cmake.sh https://github.com/Kitware/CMake/releases/download/v3.29.2/cmake-3.29.2-linux-x86_64.sh && \ | |
chmod +x cmake.sh && \ | |
sh ./cmake.sh --prefix=/usr/local --skip-license && \ | |
rm cmake.sh |
I don't think that's too bad... especially since to build the Python package specifically, scikit-build-core
will handle automatically pip
-installing cmake
if a new-enough version is not found.
And I don't think this would just be for the benefit of integrated OpenCL wheels... there are lots of other benefits we'd get by upgrading to a newer minimum version. See https://cliutils.gitlab.io/modern-cmake/chapters/intro/newcmake.html.
If you feel that this is too much of a risk, then I'd support changing the minimum in CMakeLists.txt
conditionally, like
if(__INTEGRATE_OPENCL)
cmake_minimum_required(VERSION 3.18)
else()
cmake_minimum_required(VERSION 3.28)
endif()
FetchContent_Populate
with FetchContent_MakeAvailable
and update VS version mapping in OpenCL buildsFetchContent_Populate
with FetchContent_MakeAvailable
Significant refactor of initial idea
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.
Thanks! Totally support this.
@jameslamb Thanks for your review! I'm sorry for pushing after your review, but it's a tiny non-controversial change that just removes code duplication by extracting CMake version into a variable. CI is green after this change, so I'm very confident in it and would like to merge this PR without one more round of reviews. Feel free to drop a comment and I'll revert this change in a follow up PR. |
Totally fine! I agree with that change. |
Fix warning:
https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=16653&view=logs&j=ea56812e-e7ae-55d0-6abc-4a217857fa9f&t=76df8fa4-5d0e-54b7-f50a-d34a7285da97&l=555
LightGBM requires CMake >= 3.18.
LightGBM/CMakeLists.txt
Line 26 in f8ec57b