Patch Arrow to set CMAKE_POLICY_VERSION_MINIMUM for RapidJSON - #22582
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR scopes a temporary CMake policy change to CMAKE_POLICY_VERSION_MINIMUM=3.5 around RapidJSON discovery, registers that patch in an Arrow override JSON, and passes the override to rapids_cpm_init during native build initialization. ChangesCMake RapidJSON Policy Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
🚥 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
🧹 Nitpick comments (1)
cpp/cmake/thirdparty/patches/override.json (1)
8-8: ⚡ Quick winConsider using shallow clone if patch target is available at the specified tag.
Setting
git_shallow: falseforces a full clone of the Arrow repository, which can significantly increase initial build times (Arrow repo is large). Since the patch targetscpp/cmake_modules/FindRapidJSONAlt.cmake— a core CMake infrastructure file that exists in the apache-arrow-21.0.0 tag — a shallow clone would be sufficient for applying this patch. Shallow clones are preferred unless the patch requires commit history or targets files not present in the specified tag.🤖 Prompt for 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. In `@cpp/cmake/thirdparty/patches/override.json` at line 8, The override JSON currently sets "git_shallow": false causing full clones of the Arrow repo; change it to "git_shallow": true in cpp/cmake/thirdparty/patches/override.json (the JSON key "git_shallow") so the fetch uses a shallow clone for the apache-arrow-21.0.0 tag (since the patched file cpp/cmake_modules/FindRapidJSONAlt.cmake exists in that tag) unless the patch requires history—ensure no other build step needs full history before flipping the flag.
🤖 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/cmake/thirdparty/patches/arrow_rapidjson_cmake_policy_version_minimum.diff`:
- Around line 9-12: The current patch unconditionally saves and restores
CMAKE_POLICY_VERSION_MINIMUM which causes an empty string to be written if the
variable was originally unset; update the logic around the save/restore of
CMAKE_POLICY_VERSION_MINIMUM (the temporary variable
_CMAKE_POLICY_VERSION_MINIMUM_OLD and the restore after find_package(RapidJSON
...)) to first test whether CMAKE_POLICY_VERSION_MINIMUM was defined (using
CMake's DEFINED check), only set _CMAKE_POLICY_VERSION_MINIMUM_OLD when it
exists, set CMAKE_POLICY_VERSION_MINIMUM to 3.5 for the RapidJSON find, and then
restore the previous state by either resetting CMAKE_POLICY_VERSION_MINIMUM to
the saved value or unsetting it when it was originally undefined.
---
Nitpick comments:
In `@cpp/cmake/thirdparty/patches/override.json`:
- Line 8: The override JSON currently sets "git_shallow": false causing full
clones of the Arrow repo; change it to "git_shallow": true in
cpp/cmake/thirdparty/patches/override.json (the JSON key "git_shallow") so the
fetch uses a shallow clone for the apache-arrow-21.0.0 tag (since the patched
file cpp/cmake_modules/FindRapidJSONAlt.cmake exists in that tag) unless the
patch requires history—ensure no other build step needs full history before
flipping the flag.
🪄 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: 5ae8235f-e502-41a2-b859-ca0a53609a07
📒 Files selected for processing (3)
cpp/cmake/thirdparty/patches/arrow_rapidjson_cmake_policy_version_minimum.diffcpp/cmake/thirdparty/patches/override.jsonjava/src/main/native/CMakeLists.txt
|
This is just for the release branch. Does it need to go into the main branch as well? |
|
It will be automatically forward-merged into the main branch, no problem. |
|
/merge |
5e6631f
into
NVIDIA:release/26.06
Description
Provide a patch for apache/arrow#48801
Fixes #22540
Checklist