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 rapids_cuda_set_runtime #429

Merged

Conversation

robertmaynard
Copy link
Contributor

Description

Add rapids_cuda_set_runtime which is the per target version of rapids_cuda_init_runtime.
Fixes #427

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

@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change labels Jun 13, 2023
@robertmaynard robertmaynard requested a review from a team as a code owner June 13, 2023 16:03
rapids-cmake/cuda/set_runtime.cmake Outdated Show resolved Hide resolved
rapids-cmake/cuda/set_runtime.cmake Outdated Show resolved Hide resolved
testing/cuda/set_runtime-on-interface-target.cmake Outdated Show resolved Hide resolved
rapids-cmake/cuda/set_runtime.cmake Show resolved Hide resolved
rapids-cmake/cuda/set_runtime.cmake Show resolved Hide resolved
rapids-cmake/cuda/set_runtime.cmake Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Jun 22, 2023

Do we need to address the interface/private question you raised before we finalize?

@robertmaynard
Copy link
Contributor Author

Do we need to address the interface/private question you raised before we finalize?

I think we should address this before merging.

Currently the approach is that we us PUBLIC linking for all targets (except INTERFACE). After thinking about this more I think that PRIVATE by default might be the best approach as it has the least amount of suprises. By that I mean if you call rapids_cuda_set_runtime on a target I don't think users expect it would have an impact on anything that links to that target.

Note: I am ignoring INTERFACE targets from the above. Since INTERFACE targets can only have INTERFACE linking the behavior with those is pre-determined. I do need to update the docs no matter what to explicitly state what behavior you will get based on the target type you provide.

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me now. The only question I have is whether there are any cases where we would want PUBLIC linking even for non-INTERFACE targets, in which case we'd want it parametrizable. I'm approving since I don't see any important use cases, but please add that option if you do.

@robertmaynard
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 6f59951 into rapidsai:branch-23.08 Jun 29, 2023
@robertmaynard robertmaynard deleted the per_target_cuda_runtime_api branch June 29, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[FEA] Implement a function to link a target appropriately to the CUDA runtime
3 participants