Skip to content

[DO NOT MERGE] Test CCCL update#2051

Closed
KyleFromNVIDIA wants to merge 1 commit into
NVIDIA:mainfrom
KyleFromNVIDIA:test-cccl-fix
Closed

[DO NOT MERGE] Test CCCL update#2051
KyleFromNVIDIA wants to merge 1 commit into
NVIDIA:mainfrom
KyleFromNVIDIA:test-cccl-fix

Conversation

@KyleFromNVIDIA

Copy link
Copy Markdown
Member

No description provided.

@coderabbitai

coderabbitai Bot commented Apr 30, 2026

Copy link
Copy Markdown
📝 Walkthrough

Summary by CodeRabbit

  • Chores
    • Added new configuration options for rapids-cmake repository and branch settings, enabling more flexible dependency management.

Walkthrough

The change updates the SPDX copyright year in cmake/rapids_config.cmake and introduces two new public CMake variables: rapids-cmake-repo and rapids-cmake-branch. These variables are configured with hardcoded default values and can be overridden based on additional configuration logic.

Changes

Cohort / File(s) Summary
CMake Configuration
cmake/rapids_config.cmake
Updated SPDX copyright year and added two new CMake variables (rapids-cmake-repo and rapids-cmake-branch) with repository and branch defaults that precede version checking logic.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is only a template with contributor guidelines and does not describe the actual changes being made in this PR. Replace the template with an actual description explaining what the CCCL update entails and why these CMake changes are necessary for testing.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title indicates this is a test PR with explicit 'DO NOT MERGE' prefix, which relates to the CMake changes for testing a CCCL update.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Review rate limit: 9/10 reviews remaining, refill in 6 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cmake/rapids_config.cmake (1)

31-38: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Avoid hardcoding a personal fork and mutable branch in the default dependency path.

Line 31 and Line 32 force all builds to fetch KyleFromNVIDIA/rapids-cmake@update-cccl, and Line 36 becomes effectively unreachable. This changes global dependency provenance and undermines reproducibility/supply-chain safety for normal builds.

💡 Proposed fix
-set(rapids-cmake-repo KyleFromNVIDIA/rapids-cmake)
-set(rapids-cmake-branch update-cccl)
+if(NOT rapids-cmake-repo)
+  set(rapids-cmake-repo rapidsai/rapids-cmake)
+endif()
+
+if(NOT rapids-cmake-branch)
+  set(rapids-cmake-branch "${_rapids_branch}")
+endif()
 if(NOT rapids-cmake-version)
   set(rapids-cmake-version "${RAPIDS_VERSION_MAJOR_MINOR}")
 endif()
-if(NOT rapids-cmake-branch)
-  set(rapids-cmake-branch "${_rapids_branch}")
-endif()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmake/rapids_config.cmake` around lines 31 - 38, The defaults unconditionally
set rapids-cmake-repo and rapids-cmake-branch to a personal fork and mutable
branch, making rapids-cmake-branch fallback unreachable; change this so you only
assign defaults when the variables are not already set (use if(NOT
rapids-cmake-repo) and if(NOT rapids-cmake-branch)) and point the default repo
to the official upstream (e.g., rapids/rapids-cmake) and the default branch to
the established project branch fallback (use _rapids_branch or
RAPIDS_VERSION_MAJOR_MINOR as appropriate) instead of the personal
"KyleFromNVIDIA/rapids-cmake" and "update-cccl" values so rapids-cmake-repo,
rapids-cmake-branch, rapids-cmake-version, RAPIDS_VERSION_MAJOR_MINOR and
_rapids_branch behave as true configurable defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@cmake/rapids_config.cmake`:
- Around line 31-38: The defaults unconditionally set rapids-cmake-repo and
rapids-cmake-branch to a personal fork and mutable branch, making
rapids-cmake-branch fallback unreachable; change this so you only assign
defaults when the variables are not already set (use if(NOT rapids-cmake-repo)
and if(NOT rapids-cmake-branch)) and point the default repo to the official
upstream (e.g., rapids/rapids-cmake) and the default branch to the established
project branch fallback (use _rapids_branch or RAPIDS_VERSION_MAJOR_MINOR as
appropriate) instead of the personal "KyleFromNVIDIA/rapids-cmake" and
"update-cccl" values so rapids-cmake-repo, rapids-cmake-branch,
rapids-cmake-version, RAPIDS_VERSION_MAJOR_MINOR and _rapids_branch behave as
true configurable defaults.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 631d384f-a278-4bb8-99ee-78c75bbddab6

📥 Commits

Reviewing files that changed from the base of the PR and between 3da8994 and 747f5c2.

📒 Files selected for processing (1)
  • cmake/rapids_config.cmake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant