examples: read tag from RAPIDS_BRANCH file#2293
examples: read tag from RAPIDS_BRANCH file#2293rapids-bot[bot] merged 2 commits intorapidsai:release/26.04from
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBumps pre-commit hooks, extends verify-hardcoded-version exclusions, removes two sed-based RMM_TAG replacements from the release script, and changes CMake to source Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/examples/versions.cmake`:
- Around line 10-14: The fatal error message for the RMM_TAG check is misleading
because the condition if(NOT RMM_TAG) also fires when the RAPIDS_BRANCH file
exists but is empty; update the FATAL_ERROR message to explicitly state the file
may be missing or empty (referencing RMM_TAG and the RAPIDS_BRANCH file path
built from CMAKE_CURRENT_LIST_DIR) or alternatively change the check to test
file existence and non-empty contents before emitting the message so the message
matches the actual failure mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5894481-7a29-4cdf-8cbf-32cbaee645e6
📒 Files selected for processing (3)
.pre-commit-config.yamlci/release/update-version.shcpp/examples/versions.cmake
cpp/examples/versions.cmake
Outdated
| if(NOT RMM_TAG) | ||
| message( | ||
| FATAL_ERROR | ||
| "Could not determine branch name to use for checking out rapids-cmake. The file \"${CMAKE_CURRENT_LIST_DIR}/../../RAPIDS_BRANCH\" is missing." | ||
| ) |
There was a problem hiding this comment.
Clarify the fatal message for empty-file cases too.
Line 13 says the file is “missing,” but the if(NOT RMM_TAG) check also triggers when RAPIDS_BRANCH exists but is empty. Please make the message precise.
Suggested patch
if(NOT RMM_TAG)
message(
FATAL_ERROR
- "Could not determine branch name to use for checking out rapids-cmake. The file \"${CMAKE_CURRENT_LIST_DIR}/../../RAPIDS_BRANCH\" is missing."
+ "Could not determine branch name to use for checking out rapids-cmake. The file \"${CMAKE_CURRENT_LIST_DIR}/../../RAPIDS_BRANCH\" is missing or empty."
)
endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(NOT RMM_TAG) | |
| message( | |
| FATAL_ERROR | |
| "Could not determine branch name to use for checking out rapids-cmake. The file \"${CMAKE_CURRENT_LIST_DIR}/../../RAPIDS_BRANCH\" is missing." | |
| ) | |
| if(NOT RMM_TAG) | |
| message( | |
| FATAL_ERROR | |
| "Could not determine branch name to use for checking out rapids-cmake. The file \"${CMAKE_CURRENT_LIST_DIR}/../../RAPIDS_BRANCH\" is missing or empty." | |
| ) | |
| endif() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/examples/versions.cmake` around lines 10 - 14, The fatal error message
for the RMM_TAG check is misleading because the condition if(NOT RMM_TAG) also
fires when the RAPIDS_BRANCH file exists but is empty; update the FATAL_ERROR
message to explicitly state the file may be missing or empty (referencing
RMM_TAG and the RAPIDS_BRANCH file path built from CMAKE_CURRENT_LIST_DIR) or
alternatively change the check to test file existence and non-empty contents
before emitting the message so the message matches the actual failure mode.
| # cmake-format: on | ||
| # ============================================================================= | ||
|
|
||
| set(RMM_TAG release/26.04) |
There was a problem hiding this comment.
I specifically left examples alone because my impression is that they need to work outside of the context of the repo where these supporting files are available. If that's not the case, I support this change.
There was a problem hiding this comment.
I'll defer to @bdice on that, I don't know the answer.
There was a problem hiding this comment.
I think most people building the examples will do so in the context of a cloned repository. We can always change this back if people report issues with building.
cpp/examples/versions.cmake
Outdated
|
|
||
| set(RMM_TAG release/26.04) | ||
| # Use STRINGS to trim whitespace/newlines | ||
| file(STRINGS "${CMAKE_CURRENT_LIST_DIR}/../../RAPIDS_BRANCH" RMM_TAG) |
There was a problem hiding this comment.
Assuming we go this route, wouldn't it be better to include rapids_config.cmake instead? It takes care of actually reading the files.
There was a problem hiding this comment.
I'd tried that but I swear I ran into issues with different levels of nesting and the relative paths. Let me try real quick.
There was a problem hiding this comment.
Ok pushed doing that, that worked without issue so I must have been making a mistake before.
pushd ./cpp/examples
./build.sh
./basic/build/basic_exampleWorked fine.
|
I did the "just trigger any run on the branch" trick here to get It should work on a re-run. |
|
/merge |
0115cf1
into
rapidsai:release/26.04
Description
Fixes these
pre-commiterrors blocking CI:By updating
verify-hardcoded-versionconfiguration and by updating the C++ examples to readRMM_TAGfrom theRAPIDS_BRANCHfile.See rapidsai/pre-commit-hooks#121 for details
Checklist