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 INSTALL_TARGET argument to rapids_add_test() #692

Merged

Conversation

KyleFromNVIDIA
Copy link
Contributor

Description

If another command, such as mpiexec, is used to execute a target, rapids_add_test() currently has no way of installing the target dependency. Add an INSTALL_TARGET argument to support this use case.

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

If another command, such as mpiexec, is used to execute a target,
rapids_add_test() currently has no way of installing the target
dependency. Add an INSTALL_TARGET argument to support this use case.
@KyleFromNVIDIA KyleFromNVIDIA added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Sep 19, 2024
@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner September 19, 2024 18:25
@KyleFromNVIDIA
Copy link
Contributor Author

Currently testing this PR over at rapidsai/cugraph#4656

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.

I think I understand this. But it's right at the edge of my knowledge of targets/install commands. Feel free to seek a second reviewer if you think it'd be worthwhile.

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.

Works for me. I don't think there's a general solution for inferring the target so making it explicit is good.

rapids-cmake/test/add.cmake Show resolved Hide resolved
@KyleFromNVIDIA
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit caf5dd6 into rapidsai:branch-24.10 Sep 20, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants