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

Add rapids_cpm_nvcomp with prebuilt binary support #190

Merged
merged 23 commits into from
May 16, 2022
Merged

Add rapids_cpm_nvcomp with prebuilt binary support #190

merged 23 commits into from
May 16, 2022

Conversation

robertmaynard
Copy link
Contributor

Expands the versions.json support to include the concept of per cpu architecture pre-built proprietary binary files that are used instead of building from source.

Currently the only package that supports this json entry is nvcomp which has also been added to rapids_cpm.

@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change 5 - DO NOT MERGE Hold off on merging; see PR for details labels May 12, 2022
@robertmaynard
Copy link
Contributor Author

Thinking about this more, the proprietary_binary keys need to encode the operating system as well. So we need something like <arch>-[linux|win|osx]

@robertmaynard robertmaynard added 3 - Ready for Review Ready for review by team and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels May 12, 2022
@robertmaynard
Copy link
Contributor Author

Thinking about this more, the proprietary_binary keys need to encode the operating system as well. So we need something like <arch>-[linux|win|osx]

Implemented

@robertmaynard robertmaynard mentioned this pull request May 12, 2022
Closed
@robertmaynard
Copy link
Contributor Author

Currently trying this out in cudf and realized that I need to add some more install rules.

@robertmaynard robertmaynard added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for Review Ready for review by team labels May 12, 2022
@robertmaynard robertmaynard requested a review from a team as a code owner May 13, 2022 11:53
@robertmaynard robertmaynard added 3 - Ready for Review Ready for review by team and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels May 13, 2022
@robertmaynard robertmaynard mentioned this pull request May 13, 2022
Merged
@robertmaynard
Copy link
Contributor Author

Currently trying this out in cudf and realized that I need to add some more install rules.

Implemented

vyasr
vyasr requested changes May 13, 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.

The logic in this PR looks correct. My comments are all either typo fixes or questions/suggestions for improving the clarity of the code. My suggestions aren't entirely relevant to the scope of this PR, so if you did think they were worthwhile we could push that to a follow-up.

Big picture, maybe this is just the best way to work within CMake's design constraints, but is there any way to rely less on the setting of global state? I had to check out this branch and inspect with some grepping to figure out how versions.json and the override data was pulled into a global variable and then made available. Some of my suggestions involve capturing the checks of that global state into functions.

rapids-cmake/cpm/detail/get_proprietary_binary.cmake Outdated Show resolved Hide resolved
docs/packages/rapids_cpm_versions.rst Outdated Show resolved Hide resolved
docs/packages/rapids_cpm_versions.rst Outdated Show resolved Hide resolved
docs/packages/rapids_cpm_versions.rst Outdated Show resolved Hide resolved
docs/packages/rapids_cpm_versions.rst Outdated Show resolved Hide resolved
testing/cpm/cpm_nvcomp-export.cmake Outdated Show resolved Hide resolved
rapids-cmake/cpm/nvcomp.cmake Outdated Show resolved Hide resolved
testing/cpm/cpm_nvcomp-invalid-arch.cmake Outdated Show resolved Hide resolved
testing/cpm/cpm_nvcomp-override-clears-proprietary_binary.cmake Outdated Show resolved Hide resolved
testing/cpm/cpm_nvcomp-override-clears-proprietary_binary.cmake Show resolved Hide resolved
robertmaynard and others added 12 commits May 13, 2022 14:02
@robertmaynard @vyasr
Update docs/packages/rapids_cpm_versions.rst
6e37baa 
Co-authored-by: Vyas Ramasubramani <[email protected]>
@robertmaynard @vyasr
Update rapids-cmake/cpm/detail/get_proprietary_binary.cmake
632bbf9 
Co-authored-by: Vyas Ramasubramani <[email protected]>
@robertmaynard
simplify rapids_cpm_get_proprietary_binary as found in code review
d40c14d
@robertmaynard @vyasr
Update rapids-cmake/cpm/nvcomp.cmake
b8ffa15 
Co-authored-by: Vyas Ramasubramani <[email protected]>
@robertmaynard @vyasr
Update rapids-cmake/cpm/nvcomp.cmake
b638b29 
Co-authored-by: Vyas Ramasubramani <[email protected]>
@robertmaynard @vyasr
Update rapids-cmake/cpm/nvcomp.cmake
cdc697d 
Co-authored-by: Vyas Ramasubramani <[email protected]>
@robertmaynard
Update comment to be correct
9b94e88
@robertmaynard @vyasr
Update rapids-cmake/cpm/nvcomp.cmake
93d3e04 
Co-authored-by: Vyas Ramasubramani <[email protected]>
@robertmaynard
reorder so exclude variable exists
0819a38
@robertmaynard
Remove unneeded build export set logic
ae4fab0
@robertmaynard
Encode json loading from global properties in a function
453f1f9
@robertmaynard
change license file location
94b241e
@vyasr
Copy link
Contributor

vyasr commented May 13, 2022

I don't yet have any permissions on this repo so I can't resolve threads, so I just looked through all the threads. All of my concerns look to have been addressed except for #190 (comment) and #190 (comment).

@robertmaynard
Copy link
Contributor Author

I don't yet have any permissions on this repo so I can't resolve threads, so I just looked through all the threads. All of my concerns look to have been addressed except for #190 (comment) and #190 (comment).

I believe I addressed these two outstanding comments.

@robertmaynard robertmaynard requested a review from vyasr May 16, 2022 20:04
@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit dae5e96 into rapidsai:branch-22.06 May 16, 2022
@robertmaynard robertmaynard deleted the add_cpm_nvcomp branch May 16, 2022 20:32
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

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

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