Skip to content

Conversation

@benfred
Copy link
Member

@benfred benfred commented Oct 3, 2024

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

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
@benfred benfred added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Oct 3, 2024
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>
Copy link
Contributor

@achirkin achirkin Oct 3, 2024

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)

Copy link
Member Author

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!

Copy link
Member Author

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

Copy link
Contributor

@achirkin achirkin Oct 3, 2024

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.

@benfred benfred marked this pull request as ready for review October 3, 2024 07:15
@benfred benfred requested a review from a team as a code owner October 3, 2024 07:15
@cjnolet
Copy link
Member

cjnolet commented Oct 3, 2024

/merge

@rapids-bot rapids-bot bot merged commit 5629977 into rapidsai:branch-24.10 Oct 3, 2024
52 checks passed
@benfred benfred deleted the static_lib branch October 9, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake cpp improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants