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

Make git tag and repository optional #703

Open
wants to merge 1 commit into
base: branch-24.12
Choose a base branch
from

Conversation

KyleFromNVIDIA
Copy link
Contributor

Description

To allow the removal of nvcomp 2.2.0, we make the git tag and repo arguments optional, since all nvcomp versions now use the proprietary binary instead.

Fixes: #702

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

To allow the removal of nvcomp 2.2.0, we make the git tag and repo
arguments optional, since all nvcomp versions now use the proprietary
binary instead.

Fixes: rapidsai#702
@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner October 15, 2024 21:05
@KyleFromNVIDIA KyleFromNVIDIA added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Oct 15, 2024
@bdice
Copy link
Contributor

bdice commented Oct 16, 2024

@KyleFromNVIDIA CI is failing:

CMake Error at /__w/rapids-cmake/rapids-cmake/build/cpm/cpm_nvcomp-override-clears-proprietary_binary-makefile-build/_deps/rapids-cmake-src/rapids-cmake/cpm/detail/package_details.cmake:78 (message):
  rapids_cmake can't parse 'nvcomp' json entry, it is missing a `git_tag`
  entry

I started with a different approach, which skipped the validation of git_tag and git_url if a key proprietary_binary existed. It was basically like this:

    rapids_cpm_json_get_value(version)
    rapids_cpm_json_get_value(git_url)
    rapids_cpm_json_get_value(git_tag)
    rapids_cpm_json_get_value(proprietary_binary)

    foreach(var IN ITEMS version)
      if(NOT ${var})
        message(FATAL_ERROR "rapids_cmake can't parse '${package_name}' json entry, it is missing a `${var}` entry"
        )
      endif()
    endforeach()
    foreach(var IN ITEMS git_url git_tag)
      if(NOT ${var} AND NOT proprietary_binary)
        message(FATAL_ERROR "rapids_cmake can't parse '${package_name}' json entry, it is missing a `${var}` entry and no `proprietary_binary` was provided"
        )
      endif()
    endforeach()

However, that also failed because of something with export rules not being able to find the dependencies without a git_url / git_tag? I got stuck and then you opened this PR, so I didn't pursue it further.

@bdice
Copy link
Contributor

bdice commented Oct 16, 2024

An alternative approach proposed by @robertmaynard is in #704. I think we can close this if the other approach is acceptable.

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.

[FEA] Relax requirements for versions.json git_url / git_tag for proprietary binaries
2 participants