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

Use rapids-cmake new COMPONENT exporting feature #1154

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Use rapids-cmake new COMPONENT exporting feature #1154

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1115e1
Use rapids-cmake new COMPONENT exporting feature
robertmaynard Jan 18, 2023
61e2eda
Merge branch 'branch-23.04' into fea/use_rapids_export_components
cjnolet Mar 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 20 additions & 43 deletions cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,41 +552,6 @@ install(
DESTINATION include/raft
)

# ##################################################################################################
# * export/install optional components --------------------------------------

include("${rapids-cmake-dir}/export/write_dependencies.cmake")

set(raft_components compiled distributed)
set(raft_install_comp raft raft)
if(TARGET raft_lib)
list(APPEND raft_components compiled-lib)
list(APPEND raft_install_comp compiled)
endif()

foreach(comp install_comp IN ZIP_LISTS raft_components raft_install_comp)
install(
EXPORT raft-${comp}-exports
FILE raft-${comp}-targets.cmake
NAMESPACE raft::
DESTINATION "${lib_dir}/cmake/raft"
COMPONENT ${install_comp}
)
export(
EXPORT raft-${comp}-exports
FILE ${RAFT_BINARY_DIR}/raft-${comp}-targets.cmake
NAMESPACE raft::
)
rapids_export_write_dependencies(
BUILD raft-${comp}-exports "${PROJECT_BINARY_DIR}/raft-${comp}-dependencies.cmake"
)
rapids_export_write_dependencies(
INSTALL raft-${comp}-exports
"${PROJECT_BINARY_DIR}/rapids-cmake/raft/export/${install_comp}/raft-${comp}-dependencies.cmake"
)

endforeach()

# ##################################################################################################
# * install export -----------------------------------------------------------
set(doc_string
Expand Down Expand Up @@ -629,19 +594,31 @@ if(compiled IN_LIST raft_FIND_COMPONENTS)
endif()
]=]
)
set(raft_components compiled distributed)
set(raft_export_sets raft-compiled-exports raft-distributed-exports)
if(TARGET raft_lib)
list(APPEND raft_components compiled)
list(APPEND raft_export_sets raft-compiled-lib-exports)
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point I'm a bit confused by the different targets and export sets that we have in raft. Why do we need both the raft_compiled and raft_lib targets? It used to be necessary for the nn/dist targets so that calling code could use either component independently as header-only or precompiled, but now why can't the raft target just be the header-only target and raft_compiled always be precompiled? And that stems from my other question, why do we need both raft-compiled-exports and raft-compiled-lib-exports?

Copy link
Member

Choose a reason for hiding this comment

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

@vyasr from my understanding at this point, the answer is "we don't need both raft_compiled and raft_lib anymore, but at the current time they aren't really causing harm, so we aren't in an immediate rush to remove them". @robertmaynard please correct me if that assertion is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The raft_compiled target is created even when RAFT_COMPILE_LIBRARY is turned off, so one solution for proper invariant behavior when generating the libraft-headers and libraft was to have the current organization.

If we are okay with making the raft::compiled target conditional all this logic can be removed and we could have a single target raft_compiled and export set ( raft-compiled-exports ). If we are good with this, I would be happy to open a PR that does these changes.

endif()

# Use `rapids_export` for 22.04 as it will have COMPONENT support
include(cmake/modules/raft_export.cmake)
raft_export(
INSTALL raft COMPONENTS compiled distributed EXPORT_SET raft-exports GLOBAL_TARGETS raft compiled
distributed NAMESPACE raft:: DOCUMENTATION doc_string FINAL_CODE_BLOCK code_string
rapids_export(
INSTALL raft
EXPORT_SET raft-exports
COMPONENTS ${raft_components}
COMPONENTS_EXPORT_SET ${raft_export_sets}
GLOBAL_TARGETS raft compiled distributed
cjnolet marked this conversation as resolved.
Show resolved Hide resolved
NAMESPACE raft:: DOCUMENTATION doc_string FINAL_CODE_BLOCK code_string
)

# ##################################################################################################
# * build export -------------------------------------------------------------
raft_export(
BUILD raft EXPORT_SET raft-exports COMPONENTS compiled distributed GLOBAL_TARGETS raft compiled
distributed DOCUMENTATION doc_string NAMESPACE raft:: FINAL_CODE_BLOCK code_string
rapids_export(
BUILD raft
EXPORT_SET raft-exports
COMPONENTS ${raft_components}
COMPONENTS_EXPORT_SET ${raft_export_sets}
GLOBAL_TARGETS raft
compiled distributed DOCUMENTATION doc_string NAMESPACE raft:: FINAL_CODE_BLOCK code_string
)

# ##################################################################################################
Expand Down
123 changes: 0 additions & 123 deletions cpp/cmake/modules/config.cmake.in
View file
Open in desktop

This file was deleted.

Loading