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

Jump to bottom

Update rapids-cmake to require cmake 3.23.1 #227

Merged
merged 8 commits into from
Aug 29, 2022
Merged

Update rapids-cmake to require cmake 3.23.1 #227

merged 8 commits into from
Aug 29, 2022

Conversation

robertmaynard
Copy link
Contributor

@robertmaynard robertmaynard commented Jul 29, 2022 •
edited
Loading

This moves rapids-cmake to require CMake 3.23.1 or newer.

Along with that we are able to do the following improvements:

  • Use CMake's FindGTest as it now provides all the targets we require
  • Simplify install_lib_dir logic
  • Simplify logic around version values that are all zeroes

@robertmaynard robertmaynard added breaking Introduces a breaking change feature request New feature or request 3 - Ready for Review Ready for review by team labels Jul 29, 2022
@robertmaynard robertmaynard requested a review from a team as a code owner July 29, 2022 14:38
@robertmaynard
Copy link
Contributor Author

Due to https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7523 We will need to change the get_gtest logic or maybe require 3.23.4+

@robertmaynard robertmaynard mentioned this pull request Aug 1, 2022
Merged
@robertmaynard
Copy link
Contributor Author

Dropped the changes to thrust as those are now captured in: #231

@vyasr
Copy link
Contributor

vyasr commented Aug 3, 2022

@robertmaynard do you want me to review this now, or do you want to just wait for 3.24? We've been waiting on a CMake upgrade for a while anyway so I'm not opposed, but I thought we were expecting to see 3.24 in time for our 22.10 release anyway.

@robertmaynard
Copy link
Contributor Author

@robertmaynard do you want me to review this now, or do you want to just wait for 3.24? We've been waiting on a CMake upgrade for a while anyway so I'm not opposed, but I thought we were expecting to see 3.24 in time for our 22.10 release anyway.

My thought was we could roll out these changes now, and verify that all consumers are still building correctly. That will give us time to have 3.24 released and conda packages provided. After that we can make another PR to bump the minimum required and update components with 3.24 improvements.

@vyasr
Copy link
Contributor

vyasr commented Aug 4, 2022

That's fine with me. But just FYI... conda-forge/cmake-feedstock#165 😄

@vyasr
Copy link
Contributor

vyasr commented Aug 4, 2022

I'll review this afternoon

bdice
bdice approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All seems good to me.

rapids-cmake/cmake/install_lib_dir.cmake Outdated Show resolved Hide resolved
vyasr
vyasr approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of very minor questions, otherwise LGTM.

testing/cmake/write_git_revision-custom-prefix/CMakeLists.txt Outdated Show resolved Hide resolved
testing/export/export-verify-explicit-patch-version-value-of-zero.cmake Show resolved Hide resolved
testing/export/write_language-nested-dirs/CMakeLists.txt Show resolved Hide resolved
robertmaynard and others added 2 commits August 5, 2022 07:42
@robertmaynard @bdice
Correct spelling mistakes
e2605a3 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@robertmaynard
make sure tests using 3.23.1+
afddb30
@vyasr vyasr mentioned this pull request Aug 5, 2022
Merged
rapids-bot bot pushed a commit that referenced this pull request Aug 11, 2022
@robertmaynard
Update to thrust 1.17 (#231)
a8ee959 
This pulls out the Thrust bump from #227 so that we can roll out changes slowly and have an easier time to track down issues.

Once this is in and RAPIDS is happy, we can go ahead and merge #199 which has breaking changes.

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #231
@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 776f13f into rapidsai:branch-22.10 Aug 29, 2022
@robertmaynard robertmaynard deleted the leverage_cmake_3.23 branch August 29, 2022 16:43
@robertmaynard robertmaynard mentioned this pull request Aug 29, 2022
Merged
rapids-bot bot pushed a commit that referenced this pull request Aug 29, 2022
@robertmaynard
Update rapids_find_generate_module to cmake 3.23 (#256)
5757641 
Missed as part of #227 was an update to the policy version set in `rapids_find_generate_module`

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #256
pdmack added a commit to nv-morpheus/Morpheus that referenced this pull request Nov 8, 2022
@pdmack
Update cmake minimum version based on rapids-cmake
857508b 
Per rapidsai/rapids-cmake#227

Note that this must be manually installed for focal (20.04). The snap version appears to have issues resolving RDKAFKA
@pdmack pdmack mentioned this pull request Nov 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyasr vyasr vyasr approved these changes

@bdice bdice bdice approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team breaking Introduces a breaking change feature request New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robertmaynard @vyasr @bdice