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.

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

[FEA] Modernize CMake Build Structure #3375

Closed
mdemoret-nv opened this issue Jan 14, 2021 · 4 comments
Closed

[FEA] Modernize CMake Build Structure #3375

mdemoret-nv opened this issue Jan 14, 2021 · 4 comments
Labels
Build or Dep Issues related to building the code or dependencies CMake feature request New feature or request inactive-30d inactive-90d Tech Debt Issues related to debt

Comments

@mdemoret-nv
Copy link
Contributor

Discovered in PR #3367, our CMake build architecture could benefit from moving to a more modern design using targets instead of the current implementation. This is a blocker for migrating to scikit-build. A non-complete list of improvements to make:

  • Remove all uses of CMAKE_CXX_FLAGS and CMAKE_CUDA_FLAGS
  • Use generator functions for option dependent target_include_directories and target_link_libraries
    • For example, $<$<BOOL:BUILD_CUML_MPI_COMMS>:${MPI_CXX_INCLUDE_PATH}>
  • Switch to target based architecture and export necessary includes and linked libraries
  • Create a configuration file to install allowing other CMake libraries to use find_package(CUML)
  • Use test configuration in cmake (Optional)
  • Decrease use of variables that wont be cached or exported (Optional but recommended)

A first pass was done for the linked PR and can be seen here: https://github.com/rapidsai/cuml/blob/6c2ebde695508ebdcab9ea641719d96f2775849b/cpp/CMakeLists.txt (Very much a WIP)

@mdemoret-nv mdemoret-nv added feature request New feature or request ? - Needs Triage Need team to review and classify Build or Dep Issues related to building the code or dependencies CMake labels Jan 14, 2021
@mdemoret-nv mdemoret-nv added the Tech Debt Issues related to debt label Jan 14, 2021
@dantegd
Copy link
Member

dantegd commented Jan 15, 2021

There is a related issue as well regarding cmake 3.18 features: #2194, linking it so that we close both

@wphicks wphicks removed the ? - Needs Triage Need team to review and classify label Jan 15, 2021
@github-actions
Copy link

This issue has been marked stale due to no recent activity in the past 30d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be marked rotten if there is no activity in the next 60d.

@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@dantegd
Copy link
Member

dantegd commented May 26, 2021

PR #3844 addressed this, we can open new issues for further improvements when/as needed.

@dantegd dantegd closed this as completed May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build or Dep Issues related to building the code or dependencies CMake feature request New feature or request inactive-30d inactive-90d Tech Debt Issues related to debt
Projects
None yet
Development

No branches or pull requests

3 participants