rapidsai / rapids-cmake

https://docs.rapids.ai/api/rapids-cmake/stable/
Apache License 2.0
30 stars 46 forks source link

Always download repos when they are being patched #525

Closed vyasr closed 9 months ago

vyasr commented 9 months ago

Description

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

Checklist

vyasr commented 9 months ago

@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 commented 9 months ago

@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}

vyasr commented 9 months ago

/merge

jakirkham commented 9 months ago

Thanks all! 🙏