-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add a static library for cuvs #382
Conversation
For the cuml integration, we need to be able to statically link to cuvs in order build the python wheels. This change adds a static target that lets us do that
cpp/CMakeLists.txt
Outdated
target_link_libraries( | ||
cuvs_static | ||
PUBLIC rmm::rmm raft::raft ${CUVS_CTK_MATH_DEPENDENCIES} | ||
PRIVATE nvidia::cutlass::cutlass $<TARGET_NAME_IF_EXISTS:OpenMP::OpenMP_CXX> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this target also link cuvs-cagra-search
?
(just for recap: recently I added cuvs-cagra-search
static target that holds only internally-used symbols for a subset of files, so that I could set specific CMake target flags only for these symbols)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah - I agree this should link cuvs-cagra-search (and tried in the previous commit) . The error I was hitting was :
CMake Error in CMakeLists.txt:
export called with target "cuvs_static" which requires target
"cuvs-cagra-search" that is not in any export set.
https://github.com/rapidsai/cuvs/actions/runs/11155746120/job/31007185860
Will try your suggestion - thanks for this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works great - thanks! applied in latest commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we still need the cuvs-cagra-search
in the install export set? Theoretically, we shouldn't be exposing that as a library and nobody outside cuVS should need to link against it. I suspect the error you saw previously was indirectly caused by the linker error and thus failure of CMake to register cuvs-cagra-search
.
/merge |
For the cuml integration, we need to be able to statically link to cuvs in order build the python wheels. This change adds a static target that lets us do that