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

Expand rapids_cpm_init to support custom default version files #595

Conversation

robertmaynard
Copy link
Contributor

Description

First step to implelementing #587.

Exposes a new CUSTOM_DEFAULT_VERSION_FILE option to rapids_cpm_init that allows projects to specify a new default versions.json to be used instead of the one packaged with rapids-cmake

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.

@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change 3 - Ready for Review Ready for review by team labels Apr 26, 2024
@robertmaynard robertmaynard requested a review from a team as a code owner April 26, 2024 16:28
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for the most part, but I have a few suggestions regarding formatting and one question about global properties.

rapids-cmake/cpm/init.cmake Outdated Show resolved Hide resolved
rapids-cmake/cpm/init.cmake Outdated Show resolved Hide resolved
if(already_loaded)
return()
endif()
set_property(GLOBAL PROPERTY rapids_cpm_load_presets "ON")
set_property(GLOBAL PROPERTY rapids_cpm_load_presets_${_rapids_preset_version_file} "ON")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to consider here is that the same file might be accessed by different relative paths, and thus will set two different properties. If this is merely a cache for optimization then it might not matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a cache optimization. Since we check if a package has been loaded before adding it, loadin the same file multiple times would become a slow no-op.

Co-authored-by: Kyle Edwards <[email protected]>
@robertmaynard
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 6301b3d into rapidsai:branch-24.06 May 1, 2024
15 checks passed
@robertmaynard robertmaynard deleted the fea/allow_for_custom_default_versions.json branch May 1, 2024 19:12
rapids-bot bot pushed a commit that referenced this pull request May 3, 2024
Extends #595 with support for setting the default and override file used by rapids-cmake via the command line.

Fixes #552 #587

Authors:
  - Robert Maynard (https:/robertmaynard)

Approvers:
  - Vyas Ramasubramani (https:/vyasr)

URL: #596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants