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

[BUG] rapids_cpm_find isn't as invariant as one would expect #49

Closed
robertmaynard opened this issue Jul 30, 2021 · 1 comment · Fixed by #51
Closed

[BUG] rapids_cpm_find isn't as invariant as one would expect #49

robertmaynard opened this issue Jul 30, 2021 · 1 comment · Fixed by #51
Assignees
Labels
2 - In Progress Currenty a work in progress bug Something isn't working

Comments

@robertmaynard
Copy link
Contributor

Describe the bug

Consider the following reduced example of bringing in rmm via CPM and as a dependency via find_package

    rapids_cpm_find(A)
        -> find_package(rmm)
    rapids_cpm_find(rmm INSTALL_EXPORT_SET B)

If package A sets up variables / state that are required to find rmm, the subsequent call to rapids_cpm_find will try and check out rmm and build it by source causing duplicate target errors. If rapids_cpm_find(A) hadn't found a pre-built rmm we would have had no error as CPM is designed to handle that use case.

The current solution to this is the following:

    rapids_cpm_find(A)
        -> find_package(rmm)
   if(NOT TARGET rmm::rmm)
        rapids_cpm_find(rmm INSTALL_EXPORT_SET B)
   endif()

This fixes the biggest issue ( duplicate targets ) but causes rmm not to be entered into the export set of B. This is problematic, as A isn't in the export set of B and therefore we now have dropped the dependency.

The correct logic would be:

    rapids_cpm_find(A)
        -> find_package(rmm)
   if(NOT TARGET rmm::rmm)
        rapids_cpm_find(rmm INSTALL_EXPORT_SET B)
   else()
        rapids_export_package(INSTALL rmm B)
   endif()

Desired behavior
While we have a work around this requires users of rapids-cmake to understand all the permutations of behavior that is possible, the exact thing that rapids-cmake is trying to alleviate.

What should happen is that rapids_cpm_find should handle all the existence checks by looking at the GLOBAL_TARGETS argument. If any of those targets exist, it should record INSTALL_EXPORT_SET information and terminate. That way users get to write 'easy' code and get the correct behavior no matter what is happening

@robertmaynard robertmaynard added bug Something isn't working 2 - In Progress Currenty a work in progress labels Jul 30, 2021
@robertmaynard robertmaynard self-assigned this Jul 30, 2021
@robertmaynard
Copy link
Contributor Author

Note: rapids_find_package doesn't suffer from this problem, since all config based find modules already don't try and add targets that already exist which allowsrapids_find_package to be invariant.

robertmaynard added a commit to robertmaynard/rapids-cmake that referenced this issue Jul 30, 2021
Fixes rapidsai#49

rapids_cpm_find now looks at the `GLOBAL_TARGETS` entries and
wil not call CPMFindPackage if any of them already exist.

This makes it safe to call `rapids_cpm_find` no matter what
existing targets already exist. therefore users of rapids-cmake
don't need to do manual `if(TARGET ...)` checks, resulting
in missing packages in the export set.
robertmaynard added a commit to robertmaynard/rapids-cmake that referenced this issue Jul 30, 2021
Fixes rapidsai#49

rapids_cpm_find now looks at the `GLOBAL_TARGETS` entries and
wil not call CPMFindPackage if any of them already exist.

This makes it safe to call `rapids_cpm_find` no matter what
existing targets already exist. therefore users of rapids-cmake
don't need to do manual `if(TARGET ...)` checks, resulting
in missing packages in the export set.
robertmaynard added a commit to robertmaynard/rapids-cmake that referenced this issue Jul 30, 2021
Fixes rapidsai#49

rapids_cpm_find now looks at the `GLOBAL_TARGETS` entries and
will not call CPMFindPackage if any of them already exist.

This makes it safe to call `rapids_cpm_find` no matter what
existing targets already exist. therefore users of rapids-cmake
don't need to do manual `if(TARGET ...)` checks, resulting
in missing packages in the export set.
robertmaynard added a commit to robertmaynard/rapids-cmake that referenced this issue Jul 30, 2021
Fixes rapidsai#49

rapids_cpm_find now looks at the `GLOBAL_TARGETS` entries and
will not call CPMFindPackage if any of them already exist.

This makes it safe to call `rapids_cpm_find` no matter what
existing targets already exist. therefore users of rapids-cmake
don't need to do manual `if(TARGET ...)` checks, resulting
in missing packages in the export set.
robertmaynard added a commit to robertmaynard/rapids-cmake that referenced this issue Aug 2, 2021
Fixes rapidsai#49

rapids_cpm_find now looks at the `GLOBAL_TARGETS` entries and
will not call CPMFindPackage if any of them already exist.

This makes it safe to call `rapids_cpm_find` no matter what
existing targets already exist. therefore users of rapids-cmake
don't need to do manual `if(TARGET ...)` checks, resulting
in missing packages in the export set.
robertmaynard added a commit to robertmaynard/rapids-cmake that referenced this issue Aug 2, 2021
Fixes rapidsai#49

rapids_cpm_find now looks at the `GLOBAL_TARGETS` entries and
will not call CPMFindPackage if any of them already exist.

This makes it safe to call `rapids_cpm_find` no matter what
existing targets already exist. therefore users of rapids-cmake
don't need to do manual `if(TARGET ...)` checks, resulting
in missing packages in the export set.
@rapids-bot rapids-bot bot closed this as completed in #51 Aug 2, 2021
rapids-bot bot pushed a commit that referenced this issue Aug 2, 2021
Fixes #49

rapids_cpm_find now looks at the `GLOBAL_TARGETS` entries and will not call CPMFindPackage if any of them already exist.

This makes it safe to call `rapids_cpm_find` no matter what existing targets already exist. therefore users of rapids-cmake don't need to do manual `if(TARGET ...)` checks, resulting in missing packages in the export set.

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers: None

URL: #51
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this issue Aug 25, 2021
This combines some general CMake style cleanup and brings new rapids-cmake features to cuml including:

- Usage of `rapids_cmake_write_version_file` to simplify cuml version header writing
- Usage of `rapids_cmake_install_lib_dir` to make sure we install raft correctly on non-debain based distro's ( lib64 ), while also handling conda installation requirements ( always lib no matter the distro )
- Usage of `rapids_cpm` pre-configured pacakges
- Removal of early termination before `rapids_cpm_find` since a better solution now exists ( rapidsai/rapids-cmake#49 )
- Updates the examples to use `find_package(cuml)` since that is best practice.

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #4175
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this issue Sep 7, 2021
This combines some general CMake style cleanup and brings new rapids-cmake features to cugraph including:

- Usage of `rapids_cmake_write_version_file` to simplify cugraph version header writing
- Usage of `rapids_cmake_install_lib_dir` to make sure we install raft correctly on non-debain based distro's ( lib64 ), while also handling conda installation requirements ( always lib no matter the distro )
- Usage of `rapids_cpm` pre-configured pacakges
- Removal of early termination before `rapids_cpm_find` since a better solution now exists ( rapidsai/rapids-cmake#49 )

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Brad Rees (https://github.com/BradReesWork)
  - Rick Ratzel (https://github.com/rlratzel)

URL: #1790
rapids-bot bot pushed a commit to rapidsai/raft that referenced this issue Sep 9, 2021
This combines some general CMake style cleanup and brings new rapids-cmake features to RAFT including:

- Usage of `rapids_cmake_install_lib_dir` to make sure we install raft correctly on non-debain based distro's ( lib64 ), while also handling conda installation requirements ( always lib no matter the distro )
- Usage of `rapids_cpm` pre-configured pacakges
- Removal of early termination before `rapids_cpm_find` since a better solution now exists ( rapidsai/rapids-cmake#49 )

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #320
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this issue Oct 9, 2023
This combines some general CMake style cleanup and brings new rapids-cmake features to cuml including:

- Usage of `rapids_cmake_write_version_file` to simplify cuml version header writing
- Usage of `rapids_cmake_install_lib_dir` to make sure we install raft correctly on non-debain based distro's ( lib64 ), while also handling conda installation requirements ( always lib no matter the distro )
- Usage of `rapids_cpm` pre-configured pacakges
- Removal of early termination before `rapids_cpm_find` since a better solution now exists ( rapidsai/rapids-cmake#49 )
- Updates the examples to use `find_package(cuml)` since that is best practice.

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currenty a work in progress bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant