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

Thrust when exported now automatically calls thrust_create_target #467

Conversation

robertmaynard
Copy link
Contributor

Description

Instead of having each consuming project inject a code block into the project-config.cmake we instead have rapids-cmake call thrust_create_target right after find_package(Thrust), only when thrust was found.

This fixes two existing issues:

  1. It removes the need for rapids_cpm_thrust to remove the the custom Thrust target from the global target set. Now that target will exist when rapids-cmake tries to promote it to global
  2. It removes issues when multiple calls to find_package(thrust) occur before any calls to thrust_create_target. This creates some vexing issues when the thrust find calls resolve to different major versions of thrust

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

Instead of having each consuming project inject a code block into
the `project-config.cmake` we instead have rapids-cmake call
`thrust_create_target` right after `find_package(Thrust)`,
only when thrust was found.

This fixes two existing issues:
  1. It removes the need for `rapids_cpm_thrust` to remove the
  the custom Thrust target from the global target set. Now that
  target will exist when rapids-cmake tries to promote it to global
  2. It removes issues when multiple calls to `find_package(thrust)`
  occur before any calls to `thrust_create_target`. This creates
  some vexing issues when the thrust find calls resolve to different
  major versions of thrust
@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change 3 - Ready for Review Ready for review by team labels Sep 28, 2023
@robertmaynard robertmaynard requested a review from a team as a code owner September 28, 2023 15:01
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

LGTM

rapids-cmake/cpm/thrust.cmake Outdated Show resolved Hide resolved
testing/export/CMakeLists.txt Outdated Show resolved Hide resolved
testing/export/export_package-install-post-find-code.cmake Outdated Show resolved Hide resolved
rapids-cmake/cpm/thrust.cmake Show resolved Hide resolved
These commands now offer a syntax that allows termination
based on no export set being given, or a external condition
variable.
@robertmaynard robertmaynard force-pushed the fea/export-dependencies-auto-call-thrust_create_target branch from e7ff0d5 to ac69df5 Compare October 3, 2023 13:56
@vyasr
Copy link
Contributor

vyasr commented Oct 9, 2023

/merge

@rapids-bot rapids-bot bot merged commit 3bc1394 into rapidsai:branch-23.12 Oct 9, 2023
17 checks passed
@robertmaynard robertmaynard deleted the fea/export-dependencies-auto-call-thrust_create_target branch October 9, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants