Prepare librtcx for standalone usage - #22856
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntegrates RAPIDS CMake packaging into librtcx: local VERSION/RAPIDS_BRANCH resolution, switch to rapids_find_package for CUDA tied to an export set, add conditional install/export registration for rtcx, defer zstd validation into embed(), and add a local Changeslibrtcx RAPIDS packaging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/librtcx/embed.cmake`:
- Around line 271-279: The embed() logic currently always requires and links
zstd (target_include_directories/target_link_libraries and the NOT TARGET zstd
check), which breaks when COMPRESSION is set to "none"; update the CMake snippet
to only perform the zstd include/check/link when the COMPRESSION variable is not
"none" (i.e., wrap the zstd-related target_include_directories, the NOT TARGET
zstd FATAL_ERROR check, and the target_link_libraries(... zstd) call in a
conditional that checks COMPRESSION != "none" while leaving
set_target_properties(${RUNNER} ...) unchanged; reference RUNNER and the zstd
target in the conditional).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c27fe1d2-da0f-4117-b0c9-4726ac3e80ae
📒 Files selected for processing (5)
cpp/librtcx/CMakeLists.txtcpp/librtcx/RAPIDS_BRANCHcpp/librtcx/VERSIONcpp/librtcx/cmake/rapids_config.cmakecpp/librtcx/embed.cmake
| @@ -0,0 +1 @@ | |||
| main | |||
There was a problem hiding this comment.
Is this a symlink? This and VERSION should be symlinks.
There was a problem hiding this comment.
I can change it for now, but it'll wind up getting copied as soon as we split this out into its own repo anyway.
bdice
left a comment
There was a problem hiding this comment.
All looks fine aside from the symlink question.
Replace manual configure_package_config_file with rapids_export() for both BUILD and INSTALL tree exports. This generates rtcx-config.cmake (lowercase dash naming) matching RAPIDS conventions. Changes: - Add rapids-export and rapids-find to standalone guard - Use rapids_find_package(CUDAToolkit) with export sets - Add FINAL_CODE_BLOCK for embed.cmake include and rtcx_LIBCXX_DIR - Separate code blocks for install-tree (relative) vs build-tree (absolute) - Build-tree export always active for CPM consumers - Delete rtcxConfig.cmake.in (replaced by rapids_export auto-generation)
9e2c29e to
ece1fb6
Compare
4c3022f to
1972d88
Compare
rapidsmpf#1094 removed assert_eq from rapidsmpf.testing. Copy the implementation into cudf_streaming.testing and update all imports.
|
rapidsai/rapidsmpf#1094 removed the |
## Description <!-- Provide a standalone description of changes in this PR. --> <!-- Reference any issues closed by this PR with "closes NVIDIA#1234". --> <!-- Note: The pull request title will be included in the CHANGELOG. --> This PR makes a number of changes to rtcx's CMake to support using an out-of-tree version of it. ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes.
… fragment names (#22898) Fixes conditional compilation bug introduced in #22856. The `Zstd` functions are always referenced regardless of the specified compression. The dependency is always available regardless of the compression type, and it should be linked into the embed runner executable. It also makes a fix to allow `nullptr` in fragment names. Authors: - Basit Ayantunde (https://github.com/lamarrr) Approvers: - Bradley Dice (https://github.com/bdice) URL: #22898
Description
This PR makes a number of changes to rtcx's CMake to support using an out-of-tree version of it.
Checklist