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

Enable components installation using CMake #621

Merged
merged 2 commits into from
May 6, 2022
Merged

Enable components installation using CMake #621

merged 2 commits into from
May 6, 2022

Conversation

jjacobelli
Copy link
Contributor

Enable components installation using CMake. This feature will be used to merge all CPP conda recipes into one, doing a top level build for all the libs, and only installing the right component while packaging.

@jjacobelli jjacobelli added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 22, 2022
@jjacobelli jjacobelli requested a review from a team as a code owner April 22, 2022 15:25
@jjacobelli jjacobelli self-assigned this Apr 22, 2022
ajschmidt8
ajschmidt8 reviewed Apr 22, 2022
View reviewed changes
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

This looks okay to me, but I'm no CMake expert. So I'll defer to @robertmaynard for this one.

robertmaynard
robertmaynard requested changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

The install rules in the raft_export.cmake need to be updated as well. As it currently stands explicitly specifying the components during install will lead to no CMake config files being installed which stops consumers from using it.

@robertmaynard
Copy link
Contributor

The install logic for the CMake raft-config components https://github.com/rapidsai/raft/blob/branch-22.06/cpp/CMakeLists.txt#L445
needs to be updated as well.

@jjacobelli
Copy link
Contributor Author

@robertmaynard Hey, I updated the requested files, please let me know if it is what you expected

robertmaynard
robertmaynard requested changes May 3, 2022
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

We are incorrectly installing all the component related CMake files in the raft component instead of having them in the proper named component.

cpp/cmake/modules/raft_export.cmake Show resolved Hide resolved
cpp/CMakeLists.txt Outdated
@@ -448,6 +457,7 @@ foreach(comp IN LISTS raft_components)
FILE raft-${comp}-targets.cmake
NAMESPACE raft::
DESTINATION "${lib_dir}/cmake/raft"
COMPONENT raft
Copy link
Contributor

Choose a reason for hiding this comment

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

This install needs to map to the component not raft. so raft_distance_lib goes to COMPONENT raft_distance

@jjacobelli jjacobelli force-pushed the components branch 4 times, most recently from d486dae to 2ebe88b Compare May 5, 2022 14:47
robertmaynard
robertmaynard approved these changes May 5, 2022
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Works locally as desired

@cjnolet
Copy link
Member

cjnolet commented May 5, 2022

@Ethyling strangely, there appears to be a cmake error in CI

@robertmaynard
Copy link
Contributor

@Ethyling strangely, there appears to be a cmake error in CI

Ahh we have to make sure the install(DIRECTORY only occurs if the directory exists. This will mean we need to move the raft_export commands to be the last functions called in the root CMakeLists.txt

@cjnolet
Copy link
Member

cjnolet commented May 6, 2022

@gpucibot merge

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented May 6, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7a01581 into rapidsai:branch-22.06 May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajschmidt8 ajschmidt8 ajschmidt8 left review comments

@robertmaynard robertmaynard robertmaynard approved these changes

Assignees

@jjacobelli jjacobelli

Labels
CMake cpp 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.
4 participants
@jjacobelli @robertmaynard @cjnolet @ajschmidt8