GH-48801: [C++] Set CMAKE_POLICY_VERSION_MINIMUM for RapidJSON#49993
Conversation
We already set this variable when adding projects through `ExternalProject_Add()`, but the config file that RapidJSON exports sets `cmake_minimum_required()` to a version older than 3.5. Set the policy minimum when searching for the just-built RapidJSON.
|
|
|
|
KyleFromNVIDIA
changed the title
|
|
There was a problem hiding this comment.
Pull request overview
This PR addresses CMake 4.x configuration failures caused by RapidJSON’s exported RapidJSONConfig.cmake using an old cmake_minimum_required() version by temporarily setting CMAKE_POLICY_VERSION_MINIMUM when FindRapidJSONAlt.cmake calls find_package(RapidJSON).
Changes:
- Temporarily sets
CMAKE_POLICY_VERSION_MINIMUMto3.5aroundfind_package(RapidJSON ...)in the RapidJSON “Alt” find module. - Restores
CMAKE_POLICY_VERSION_MINIMUMafterward.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set(_CMAKE_POLICY_VERSION_MINIMUM_OLD ${CMAKE_POLICY_VERSION_MINIMUM}) | ||
| set(CMAKE_POLICY_VERSION_MINIMUM 3.5) | ||
| find_package(RapidJSON ${find_package_args}) | ||
| set(CMAKE_POLICY_VERSION_MINIMUM ${_CMAKE_POLICY_VERSION_MINIMUM_OLD}) |
There was a problem hiding this comment.
Bad review. Since ${_CMAKE_POLICY_VERSION_MINIMUM_OLD} is unquoted, if it's unset, CMAKE_POLICY_VERSION_MINIMUM will be unset rather than being set to an empty string. There is no scenario where CMAKE_POLICY_VERSION_MINIMUM being an empty string is valid, so the existing code covers all valid scenarios properly.
|
Error message: rapidsai/cudf#22540 |
|
GH-48801 is for bundled RapidJSON but this is for system RapidJSON. |
|
See my explanation at #48801 (comment). I think this PR is worth doing regardless. |
github-actions
Bot
added
awaiting committer review
|
The failure in https://github.com/apache/arrow/actions/runs/26114813357/job/76866779105?pr=49993 looks unrelated to this PR. How can I get unstuck here? |
|
It's already fixed in main. Rebasing on main will fix it. |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 1ef5e4a. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
We already set this variable when adding projects through
ExternalProject_Add(), but the config file that RapidJSON exports setscmake_minimum_required()to a version older than 3.5. Set the policy minimum when searching for the just-built RapidJSON.What changes are included in this PR?
Set
CMAKE_POLICY_VERSION_MINIMUMinFindRapidJSONAlt.cmakeAre these changes tested?
Tested locally
Are there any user-facing changes?