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

Add config file for cmake to find UCC on system #255

Merged
merged 1 commit into from
Jul 14, 2021

Conversation

zasdfgbnm-nvidia
Copy link
Contributor

@zasdfgbnm-nvidia zasdfgbnm-nvidia commented Jul 13, 2021

This PR makes the installation script writes cmake config files to ${prefix}/lib/cmake/ucc so that cmake projects can easily find UCC installed on the system. PyTorch uses cmake, so this will be helpful for PyTorch to integrate the ucc backend to upstream.

I will add the same thing for UCX later. This PR is tested by: https://github.com/zasdfgbnm/cmake-ucx-ucc, please let me know if you want cmake testes to be included in this repository as well.

@mellanox-github
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@Sergei-Lebedev Sergei-Lebedev left a comment

Choose a reason for hiding this comment

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

pls add copyright to new files

if (PACKAGE_FIND_VERSION_RANGE)
# Package version must be in the requested version range
if ((PACKAGE_FIND_VERSION_RANGE_MIN STREQUAL "INCLUDE" AND PACKAGE_VERSION VERSION_LESS PACKAGE_FIND_VERSION_MIN)
OR ((PACKAGE_FIND_VERSION_RANGE_MAX STREQUAL "INCLUDE" AND PACKAGE_VERSION VERSION_GREATER PACKAGE_FIND_VERSION_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra parenthesis

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

@zasdfgbnm
Copy link
Contributor

zasdfgbnm commented Jul 13, 2021

I have added copyright info, I also updated the commit title.

Copy link
Collaborator

@vspetrov vspetrov left a comment

Choose a reason for hiding this comment

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

@zasdfgbnm-nvidia thanks for the PR. I have a couple of minor comments:

  1. Can we support project name with capital letters? Currently when i need to set custom search path i'm setting -Ducc_DIR= which kind of looks unorthodox. I'd prefer it be: UCC_DIR.
  2. Why cmake-* files go under install/lib? Is it common? Wouldn't it be more natural to put it, e.g., in "install/share" ?

@zasdfgbnm
Copy link
Contributor

Re @vspetrov

  1. ucc_DIR vs UCC_DIR actually depends on whether you are doing find_package(ucc) or find_package(UCC), if your variable name and package name matches, for both cases, CMake should be able to find the correct package. The only difference is inside your cmake scripts, whether you should use ucc_INCLUDE_DIRS vs Ucc_INCLUDE_DIRS vs UCC_INCLUDE_DIRS and target_link_libraries(mytarget ucc::ucc) vs target_link_libraries(mytarget Ucc::Ucc) vs target_link_libraries(mytarget UCC::UCC). I updated my PR, and currently it is doing UCC_INCLUDE_DIRS and target_link_libraries(mytarget ucc::ucc). Let me know if this works for you.

  2. Yes, this is a standard path, for example on my system

gaoxiang@sunnyvale ~/ucc cmake $ ls /usr/share/cmake/
 bash-completion   Modules
gaoxiang@sunnyvale ~/ucc cmake $ ls /usr/lib/cmake/
 AccountsQt5                                            KF5Config                     libmongoc-1.0
 Analitza5                                              KF5ConfigWidgets              libmsym
 AppStreamQt                                            KF5ContactEditor              LibNotificationManager
 arpack-ng                                              KF5Contacts                   libssh
 Astro                                                  KF5CoreAddons                 LibTaskManager
 avogadrolibs                                           KF5Crash                      libwebsockets
 Boost-1.76.0                                           KF5DAV                        libxml2
 boost_atomic-1.76.0                                    KF5DBusAddons                 libzip
 boost_chrono-1.76.0                                    KF5Declarative                llvm
.....
gaoxiang@sunnyvale ~/ucc cmake $ tree /usr/lib/cmake/
/usr/lib/cmake/
|-- AccountsQt5
|   |-- AccountsQt5Config.cmake
|   `-- AccountsQt5ConfigVersion.cmake
|-- Analitza5
|   |-- Analitza5Config.cmake
|   |-- Analitza5ConfigVersion.cmake
|   |-- Analitza5Targets-noconfig.cmake
|   `-- Analitza5Targets.cmake
|-- AppStreamQt
|   |-- AppStreamQtConfig.cmake
|   `-- AppStreamQtConfigVersion.cmake
|-- Astro
|   |-- AstroConfig.cmake
|   |-- AstroConfigVersion.cmake
|   |-- AstroTargets-noconfig.cmake
|   `-- AstroTargets.cmake
......

For another example see: https://github.com/nih-at/libzip/blob/master/CMakeLists.txt#L442

Copy link
Collaborator

@vspetrov vspetrov left a comment

Choose a reason for hiding this comment

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

thanks, looks good

@vspetrov
Copy link
Collaborator

@zasdfgbnm please rebase

@zasdfgbnm
Copy link
Contributor

@vspetrov rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants