-
Notifications
You must be signed in to change notification settings - Fork 85
Update various library versions to meet the minimum required CMake version (>= 3.5) for compatibility with CMake 3.30.4. #858
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
Changes from 17 commits
cf96fcb
40e5d0f
7a6753b
5a0c2ef
aa39627
523730f
fae91c7
b0fbdf6
2823f32
20f7874
a1fc4dd
a7e7beb
19f679a
8c88612
0bc5648
7f92112
18ccd3d
8196f03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| diff --git a/CMakeLists.txt b/CMakeLists.txt | ||
| index a8329097..f906d926 100644 | ||
| --- a/CMakeLists.txt | ||
| +++ b/CMakeLists.txt | ||
| @@ -1,4 +1,5 @@ | ||
| -cmake_minimum_required(VERSION 2.8.12) | ||
| +# [cuCIM patch] Set minimum CMake version to 3.30.0 | ||
| +cmake_minimum_required(VERSION 3.30.0) | ||
|
|
||
| if(CMAKE_EXECUTABLE_SUFFIX) | ||
| set(CMAKE_EXECUTABLE_SUFFIX_TMP ${CMAKE_EXECUTABLE_SUFFIX}) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,8 @@ if (NOT TARGET deps::libopenjpeg) | |
| FetchContent_Declare( | ||
| deps-libopenjpeg | ||
| GIT_REPOSITORY https://github.com/uclouvain/openjpeg.git | ||
| GIT_TAG v2.5.0 | ||
| GIT_TAG v2.5.3 | ||
| PATCH_COMMAND ${GIT_EXECUTABLE} apply "${CMAKE_CURRENT_LIST_DIR}/libopenjpeg.patch" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we be generating this with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @KyleFromNVIDIA Created the follow-up issue: |
||
| GIT_SHALLOW TRUE | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| diff --git a/CMakeLists.txt b/CMakeLists.txt | ||
| index b04561f4..2392c14d 100644 | ||
| --- a/CMakeLists.txt | ||
| +++ b/CMakeLists.txt | ||
| @@ -7,7 +7,8 @@ | ||
| # For this purpose you can define a CMake var: OPENJPEG_NAMESPACE to whatever you like | ||
| # e.g.: | ||
| # set(OPENJPEG_NAMESPACE "GDCMOPENJPEG") | ||
| -cmake_minimum_required(VERSION 3.5) | ||
| +# [cuCIM patch] Set minimum CMake version to 3.30.0 | ||
| +cmake_minimum_required(VERSION 3.30.0) | ||
|
|
||
| if(NOT OPENJPEG_NAMESPACE) | ||
| set(OPENJPEG_NAMESPACE "OPENJPEG") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ if (NOT TARGET deps::libtiff) | |
| GIT_REPOSITORY https://gitlab.com/libtiff/libtiff.git | ||
| GIT_TAG v4.1.0 | ||
| GIT_SHALLOW TRUE | ||
| PATCH_COMMAND ${GIT_EXECUTABLE} apply "${CMAKE_CURRENT_LIST_DIR}/libtiff.patch" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we be generating this with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @KyleFromNVIDIA Created the follow-up issue: |
||
| EXCLUDE_FROM_ALL | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| diff --git a/CMakeLists.txt b/CMakeLists.txt | ||
| index 35b48770..7955fe73 100644 | ||
| --- a/CMakeLists.txt | ||
| +++ b/CMakeLists.txt | ||
| @@ -23,10 +23,12 @@ | ||
| # LIABILITY, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE | ||
| # OF THIS SOFTWARE. | ||
|
|
||
| -cmake_minimum_required(VERSION 2.8.11) # b/c of use of BUILD_INTERFACE generator expression | ||
| +# [cuCIM patch] Set minimum CMake version to 3.30.0 | ||
| +cmake_minimum_required(VERSION 3.30.0) | ||
|
|
||
| -# Default policy is from 2.8.9 | ||
| -cmake_policy(VERSION 2.8.9) | ||
| +# [cuCIM patch] Set default policy to 3.30.0 | ||
| +# Default policy is from 3.30.0 | ||
| +cmake_policy(VERSION 3.30.0) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems since CMake 3.12 it has been possible to specify a range of versions for 1.) Why does having a lower minimum for 2.) For I was wondering if we set that environment variable during the build if some of the patches could be removed?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see in that same docs page that it states:
So it seems ideally upstream projects would add some maximum version like cmake_minimum_required(2.8.9...3.30.0)but unless that is implemented upstream we will still need some patch. Maybe just check whether setting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @grlee77 for the investigation and suggestion!
Indeed, setting it could eliminate the need for some of the patches.
This variable needs to be set by an external command, and something like the following could bypass the error:
I couldn't find a proper way to set the variable for I am afraid that setting Unless the API is broken or the update requires significant code changes, I believe updating libraries is beneficial from a security standpoint and for future work. I think resolving the minimum requirement issue by upgrading the versions is a reasonable approach, and I'd prefer to keep the current PR as is.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @KyleFromNVIDIA ! |
||
| # Set MacOSX @rpath usage globally. | ||
| if (POLICY CMP0020) | ||
| cmake_policy(SET CMP0020 NEW) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By setting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank
Thanks @KyleFromNVIDIA for the information and feedback! Let me also patch those redundant lines.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just updated. Thank you! |
||
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.
Shouldn't we be generating this with
rapids_cpm_generate_patch_command()?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.
cuCIM doesn't really use rapids-cmake today. I think that is the right idea, but there's a lot of effort needed to get cuCIM onto rapids-cmake.
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 @KyleFromNVIDIA and @bdice!
Yes, currently cuCIM doesn't follow the standard RAPIDS build setup, but I’m planning to transition to a rapids-cmake and conda-package-based build setup in the near future.