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 to use rapids-cmake 21.10 pre-configured packages #854

Merged
merged 1 commit into from
Sep 9, 2021
Merged

Update to use rapids-cmake 21.10 pre-configured packages #854

merged 1 commit into from
Sep 9, 2021

Conversation

robertmaynard
Copy link
Contributor

Since rapids-cmake 21.10 offers pre-configured packages we can use those in rmm.

This removes the need to manually manage the following scripts:

  • thrust
  • gtest
  • spdlog

@robertmaynard robertmaynard added CMake non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Aug 26, 2021
@robertmaynard robertmaynard requested a review from a team as a code owner August 26, 2021 15:36
harrism
harrism approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Just out of curiousity, is it possible to use the rapids_cpm_* functions if you need to pull a specific branch or from someone's fork of the repo? I have this need for a PR that depends on a PR branch of libcu++ for example.

But this PR looks good.

@robertmaynard
Copy link
Contributor Author

Just out of curiousity, is it possible to use the rapids_cpm_* functions if you need to pull a specific branch or from someone's fork of the repo? I have this need for a PR that depends on a PR branch of libcu++ for example.

But this PR looks good.

Currently it requires the developer to have a fork of rapids_cmake where they modify the associated json entries ( https://github.com/rapidsai/rapids-cmake/blob/branch-21.10/rapids-cmake/cpm/versions.json ).

I think what I can do is extend rapids_cpm_init to offer an OVERRIDE parameter which would point to a custom json file with the new branch / repo information. Any project not listed in the override file would use the rapids-cmake defaults.

@robertmaynard
Copy link
Contributor Author

Just out of curiousity, is it possible to use the rapids_cpm_* functions if you need to pull a specific branch or from someone's fork of the repo? I have this need for a PR that depends on a PR branch of libcu++ for example.
But this PR looks good.

Currently it requires the developer to have a fork of rapids_cmake where they modify the associated json entries ( https://github.com/rapidsai/rapids-cmake/blob/branch-21.10/rapids-cmake/cpm/versions.json ).

I think what I can do is extend rapids_cpm_init to offer an OVERRIDE parameter which would point to a custom json file with the new branch / repo information. Any project not listed in the override file would use the rapids-cmake defaults.

I have created rapidsai/rapids-cmake#73 to track my current thoughts on how rapids-cmake can offer this feature.

@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit fe88c50 into rapidsai:branch-21.10 Sep 9, 2021
@robertmaynard robertmaynard deleted the fea/update_with_rapids_cmake_new_features branch September 9, 2021 13:09
robertmaynard added a commit to robertmaynard/cudf that referenced this pull request Sep 9, 2021
@robertmaynard
Only install thrust when using a non 'system' version
7c522f3 
Previously cudf presumed that no system version of Thrust
matched the version cudf required, but with the merger
of rapidsai/rmm#854 this isn't true.
robertmaynard added a commit to robertmaynard/rmm that referenced this pull request Sep 9, 2021
@robertmaynard
RMM shouldn't install thrust
8d5acd5 
When we merged rapidsai#854 we enabled
the installation of Thrust as part of RMM install. This causes
downstream issues as projects like cuDF want to use a patched
version to improve compile times.

Since RMM is header only we don't need to ensure our consumers
use the same Thrust version, so we can skip installing it.
This was referenced Sep 9, 2021
Closed
Merged
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Sep 13, 2021
@robertmaynard
Only install thrust when using a non 'system' version (#9206)
28a6292 
Previously cudf presumed that no system version of Thrust matched the version cudf required, but with the merger
of rapidsai/rmm#854 this isn't true.

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

Approvers:
  - Jake Hemstad (https://github.com/jrhemstad)
  - Mark Harris (https://github.com/harrism)

URL: #9206
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrism harrism harrism approved these changes

Assignees
No one assigned
Labels
CMake improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robertmaynard @harrism