Skip to content

Always download repos when they are being patched#525

Merged
rapids-bot[bot] merged 15 commits intorapidsai:branch-24.02from
vyasr:feat/always_download_if_patching
Jan 17, 2024
Merged

Always download repos when they are being patched#525
rapids-bot[bot] merged 15 commits intorapidsai:branch-24.02from
vyasr:feat/always_download_if_patching

Conversation

@vyasr
Copy link
Contributor

@vyasr vyasr commented Jan 16, 2024

Description

This PR updates rapids_cpm_find to force downloading of a package whenever a nonempty patch command exists.

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

@vyasr vyasr added bug Something isn't working non-breaking Introduces a non-breaking change labels Jan 16, 2024
@vyasr vyasr self-assigned this Jan 16, 2024
@vyasr vyasr force-pushed the feat/always_download_if_patching branch from 6d19aac to 06b9467 Compare January 16, 2024 23:17
@vyasr
Copy link
Contributor Author

vyasr commented Jan 16, 2024

@robertmaynard this is close, but there is an issue with passing an empty patch command. The issue comes from passing empty strings to rapids_cpm_find and how they are parsed by cmake_parse_arguments, which seems to drop that string even if it is quoted in the rapids_cpm_find function call. I tried using the cmake_parse_arguments(parse_argv 0...) syntax instead, but that causes the cpm_find-options-escaped-* tests to fail because quotes around CPM args are no longer preserved. I'm not sure what the best approach is here and could use some advice.

@robertmaynard
Copy link
Contributor

@robertmaynard this is close, but there is an issue with passing an empty patch command. The issue comes from passing empty strings to rapids_cpm_find and how they are parsed by cmake_parse_arguments, which seems to drop that string even if it is quoted in the rapids_cpm_find function call. I tried using the cmake_parse_arguments(parse_argv 0...) syntax instead, but that causes the cpm_find-options-escaped-* tests to fail because quotes around CPM args are no longer preserved. I'm not sure what the best approach is here and could use some advice.

@vyasr I tried playing around as well and I don't see a great solution to this. I think we are going to do is change around rapids_cpm_generate_patch_command to generate the entire CPM/FetchContent string of PATCH_COMMAND <....> or an empty string. Therefore we only pass a PATCH_COMMAND arg when we truly have a patch.

That would mean a change to each rapids_cpm_find invocation in rapids-cmake/cpm but it should be pretty easy:
PATCH_COMMAND ${patch_command} becomes ${patch_command}

@bdice bdice mentioned this pull request Jan 17, 2024
7 tasks
@vyasr vyasr marked this pull request as ready for review January 17, 2024 18:11
@vyasr vyasr requested a review from a team as a code owner January 17, 2024 18:11
@vyasr vyasr requested a review from bdice January 17, 2024 21:09
@vyasr
Copy link
Contributor Author

vyasr commented Jan 17, 2024

/merge

@rapids-bot rapids-bot bot merged commit 2b94de8 into rapidsai:branch-24.02 Jan 17, 2024
@jakirkham
Copy link
Member

Thanks all! 🙏

PointKernel pushed a commit to PointKernel/rapids-cmake that referenced this pull request Jan 23, 2024
This PR updates `rapids_cpm_find` to force downloading of a package whenever a nonempty patch command exists.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Bradley Dice (https://github.com/bdice)

URL: rapidsai#525
@vyasr vyasr deleted the feat/always_download_if_patching branch February 10, 2024 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants