search

rapidsai / rapids-cmake

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

[FEA] Relax requirements for versions.json git_url / git_tag for proprietary binaries #702

Open bdice opened 1 month ago

bdice commented 1 month ago

Is your feature request related to a problem? Please describe. cuDF has not supported nvcomp 2.2.0 for a long time, but rapids-cmake's versions.json still has an entry for nvcomp 2.2.0 and the outdated repo page (the nvcomp repo states, "This page will be retired soon" so the git_url will soon be irrelevant). However, we can't remove this outdated information because the versions.json parser expects git_url and git_tag to exist.

Describe the solution you'd like Update the parser so that proprietary packages can be used with only proprietary_binary information, without specifying git_url and git_tag.

Describe alternatives you've considered One alternative would be to allow an empty string like "git_url": "" / "git_tag": "", but that also fails with rapids_cmake can't parse 'nvcomp' json entry, it is missing a `git_url` entry.

Additional context Related to https://github.com/rapidsai/cudf/issues/16757.

bdice commented 1 month ago

@robertmaynard @KyleFromNVIDIA Could you weigh in on this proposal and/or offer guidance on how to implement it?

KyleFromNVIDIA commented 1 month ago

This sounds fine to me. Updating the logic in rapids-cmake with this exception should be pretty straightforward.

robertmaynard commented 1 month ago

The general design across rapids-cmake is to rely on required fields to well exist. Making a required field conditonal based on the existence of another field will require us to change the pinning logic.

There is very minimal validation performed on the git_url or git_tag fields. So something like no_git_checkout_allowed would be fine for the git url for nvcomp.

Given that nvcomp is our only proprietary_binary package, I am not super in favor of increasing our parsing complexity to support that single use case.

bdice commented 1 month ago

@robertmaynard Thanks, that's exactly the kind of feedback I was hoping for. I will open an alternative PR with that change.

edit: See #704.