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.

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

[FEA] Bump minimum required CMake version to 3.22.3 #78

Closed
robertmaynard opened this issue Sep 10, 2021 · 9 comments
Closed

[FEA] Bump minimum required CMake version to 3.22.3 #78

robertmaynard opened this issue Sep 10, 2021 · 9 comments
Labels
1 - On Deck To be worked on next feature request New feature or request tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general

Comments

@robertmaynard
Copy link
Contributor

Is your feature request related to a problem? Please describe.
We should continue leveraging latest CMake improvements, and therefore should review
the improvements that 3.21 and 3.22 will bring to rapids-cmake.

Describe the solution you'd like
I would like to see 3.22 become the required CMake version for rapids-cmake 22.X versions

Additional context
CMake 3.21 would bring in the following improvements:

  • set behavior in regards to local/cache variables is not consistent!!! This is big quality of life improvement
  • WriteBasicConfigVersion supports CalVer strings, so workaround in rapids-export can be dropped
  • Less false positive error reports from FindThreads
  • nicer ways to specify toolchain and install directory from the command line
  • improvements to install() to copy runtime dependency will need to be reviews and we can see if they can be used when thirdparty dependencies are missing install rules
  • Cleanup of foreach() and variable leaking. Most likely will need to update rapids-cmake in places due to this

CMake 3.22 would bring in the following improvements:

  • find_library: Infer library prefix and suffix when in script mode. This is important to rapids-cmake as we hook up to projects before the project call which is analogous to script mode.
  • compile_features ignore features for non-enabled languages. This allows projects to express a cxx_std_17 requirement and not fail when used by a project that doesn't enable CUDA.
  • CUDA language level isn't inferred by the C++ language level
  • GNUInstallDirs now aware of conda lib directory requirements
@robertmaynard robertmaynard added feature request New feature or request 0 - Backlog In queue waiting for assignment tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general labels Sep 10, 2021
@robertmaynard robertmaynard added 1 - On Deck To be worked on next and removed 0 - Backlog In queue waiting for assignment labels Oct 20, 2021
@kkraus14
Copy link
Contributor

Potential issue with moving to CMake 3.22:

@robertmaynard
Copy link
Contributor Author

@kkraus14 I believe that the 3.22.0 regression that hit AWS SDK was found during the RC cycle. And the fixe is in 3.22.0 ( aws/aws-sdk-cpp#1810 ).

Are you able to confirm this by building the AWS SDK with 3.22.0?

@kkraus14
Copy link
Contributor

@kkraus14 I believe that the 3.22.0 regression that hit AWS SDK was found during the RC cycle. And the fixe is in 3.22.0 ( aws/aws-sdk-cpp#1810 ).

Are you able to confirm this by building the AWS SDK with 3.22.0?

Yea I can confirm that 3.22 breaks building AWS SDK 1.8.x. I believe this is fixed in more recent 1.9.x releases, but in the issue linked above it also confirms that using 3.22 breaks building AWS SDK 1.9.152, so I assume any version prior to that will be broken.

@robertmaynard
Copy link
Contributor Author

Weird I thought that the following fix for 3.22.0 would fix the problem and only 3.22.0-rc1 and rc2 would have the regression.

https://gitlab.kitware.com/cmake/cmake/-/merge_requests/6442

@kkraus14
Copy link
Contributor

Weird I thought that the following fix for 3.22.0 would fix the problem and only 3.22.0-rc1 and rc2 would have the regression.

https://gitlab.kitware.com/cmake/cmake/-/merge_requests/6442

I've only tested CMake 3.22.2 since that's what's in conda-forge but can confirm that it reproduces the bug above.

@robertmaynard
Copy link
Contributor Author

Weird I thought that the following fix for 3.22.0 would fix the problem and only 3.22.0-rc1 and rc2 would have the regression.
https://gitlab.kitware.com/cmake/cmake/-/merge_requests/6442

I've only tested CMake 3.22.2 since that's what's in conda-forge but can confirm that it reproduces the bug above.

I was able to reproduce and you are correct this was never properly fixed. I have re-opened the CMake issue and we should be able to get a fix for 3.22.2+

@kkraus14
Copy link
Contributor

Thanks @robertmaynard!

@vyasr
Copy link
Contributor

vyasr commented Feb 17, 2022

rmm-python would also benefit from this bump since when we switch to scikit-build we are currently required to include CUDA as one of the project languages due to the way that the C++ rmm component adds libcudacxx's c++17 features to its interface.

@robertmaynard robertmaynard changed the title [FEA] Bump minimum required CMake version to 3.22 [FEA] Bump minimum required CMake version to 3.22.3 Mar 7, 2022
@robertmaynard
Copy link
Contributor Author

robertmaynard commented Mar 7, 2022

Thanks @robertmaynard!

This regression has been fixed in 3.22.3 patch release and the upcoming 3.23.0.

CMake does consider this to be invalid while syntax, and 3.24 will have a policy that makes this an explicit syntax error ( https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7045 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - On Deck To be worked on next feature request New feature or request tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general
Projects
None yet
Development

No branches or pull requests

3 participants