Refactor: Replace RapidJSON with nlohmann-json - #7979
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical installer and cache-handling defects, plus unresolved code-quality and dependency issues, block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request replaces RapidJSON with nlohmann-json for ABACUS JSON output and updates build, toolchain, container, CI, and testing integration. It also adds a code-quality scoring utility.
Changes:
- Migrates JSON serialization and tests to
nlohmann::ordered_json. - Updates CMake, installers, toolchains, Dockerfiles, CI, and build metadata.
- Adds code-quality analysis functionality.
File summaries
| File | Review summary |
|---|---|
tools/03_code_analysis/code_quality_score.py |
Final comments cover directory skipping, duplicate caps, literal-aware parsing, ownership heuristics, call-expression rejection, comment boundaries, and focused tests. |
toolchain/toolchain_intel.sh |
Updates toolchain JSON configuration; no final comments. |
toolchain/toolchain_gnu.sh |
Updates toolchain JSON configuration; no final comments. |
toolchain/toolchain_gcc-mkl.sh |
Updates toolchain JSON configuration; no final comments. |
toolchain/toolchain_gcc-aocl.sh |
Updates toolchain JSON configuration; no final comments. |
toolchain/toolchain_aocc-aocl.sh |
Updates toolchain JSON configuration; no final comments. |
toolchain/tests/test_rapidjson_cmake.sh |
Removes the RapidJSON test; replacement nlohmann-json integration coverage is requested. |
toolchain/scripts/tool_kit.sh |
Updates dependency tooling; no final comments. |
toolchain/scripts/stage4/install_stage4.sh |
Integrates JSON installation; no final comments. |
toolchain/scripts/stage4/install_rapidjson.sh |
Removes the RapidJSON installer; no final comments. |
toolchain/scripts/stage4/install_json.sh |
Final comments identify incorrect extraction-directory handling and masked installation failures. |
toolchain/scripts/package_versions.sh |
Pins the nlohmann-json version and checksum; no final comments. |
toolchain/scripts/lib/user_interface.sh |
Updates package interface text; no final comments. |
toolchain/scripts/lib/package_manager.sh |
Updates package management; no final comments. |
toolchain/scripts/lib/config_manager.sh |
Renames package configuration; no final comments. |
toolchain/README.md |
Documents nlohmann-json usage; no final comments. |
toolchain/build_abacus_intel.sh |
Enables the new JSON option; no final comments. |
toolchain/build_abacus_gnu.sh |
Enables the new JSON option; no final comments. |
toolchain/build_abacus_gcc-mkl.sh |
Enables the new JSON option; no final comments. |
toolchain/build_abacus_gcc-aocl.sh |
Enables the new JSON option; no final comments. |
toolchain/build_abacus_aocc-aocl.sh |
Enables the new JSON option; no final comments. |
source/source_relax/relax_driver.cpp |
Renames the JSON feature guard; no final comments. |
source/source_main/driver_run.cpp |
Renames the JSON feature guard; no final comments. |
source/source_io/parse_args.cpp |
Updates JSON build metadata handling; no final comments. |
source/source_io/module_json/test/para_json_test.cpp |
Final comment identifies new global-state reads and writes in the fixture. |
source/source_io/module_json/readin_info.h |
Updates JSON declarations; no final comments. |
source/source_io/module_json/readin_info.cpp |
Updates JSON input handling; no final comments. |
source/source_io/module_json/para_json.cpp |
Updates JSON orchestration; no final comments. |
source/source_io/module_json/output_info.h |
Updates output declarations; no final comments. |
source/source_io/module_json/output_info.cpp |
Final comments identify new PARAM flag dependencies that should be explicit inputs. |
source/source_io/module_json/json_node.h |
Defines JSON path nodes; no final comments. |
source/source_io/module_json/init_info.h |
Updates initialization declarations; no final comments. |
source/source_io/module_json/init_info.cpp |
Final comment requests splitting a comma-separated declaration. |
source/source_io/module_json/general_info.h |
Updates general-info declarations; no final comments. |
source/source_io/module_json/general_info.cpp |
Updates general JSON output; no final comments. |
source/source_io/module_json/abacusjson.h |
Defines the nlohmann-json interface; no final comments. |
source/source_io/module_json/abacusjson.cpp |
Implements nlohmann-json serialization; no final comments. |
source/source_io/CMakeLists.txt |
Gates the JSON module on the new option; no final comments. |
source/source_io/build_info.h.in |
Renames JSON build metadata; no final comments. |
source/source_esolver/esolver_ks.cpp |
Renames the JSON feature guard; no final comments. |
source/CMakeLists.txt |
Updates JSON definitions and linking; no final comments. |
Dockerfile.intel |
Installs nlohmann-json; no final comments. |
Dockerfile.gnu |
Installs nlohmann-json; no final comments. |
Dockerfile.cuda |
Installs nlohmann-json; no final comments. |
CMakeLists.txt |
Final comments identify stale documentation, legacy-cache failure handling, and missing nlohmann-json integration coverage. |
cmake/CollectBuildInfoVars.cmake |
Reports the JSON dependency version; no final comments. |
.github/workflows/test.yml |
Updates CI JSON configuration; no final comments. |
.github/workflows/coverage.yml |
Updates coverage JSON configuration; no final comments. |
.github/workflows/build_test_cmake.yml |
Enables nlohmann-json builds; no final comments. |
Review details
Suppressed comments (4)
CMakeLists.txt:152
- Rule: Focused tests
Severity: warning
Location: CMakeLists.txt:151
Reason: The RapidJSON CMake integration test was removed, but there is no equivalent nlohmann-json configure/link check fornlohmann_jsonConfig.cmakeandnlohmann_json::nlohmann_json; a broken package-prefix or imported-target integration can therefore go undetected.
Suggested action: Replace the deleted test with a focused nlohmann-json CMake integration test that configures with the toolchain prefix and links a minimal target through the imported target.
Exception: not allowed
# enable json support
if(ENABLE_JSON)
find_package(nlohmann_json REQUIRED)
endif()
source/source_io/module_json/init_info.cpp:24
- Rule: One variable per declaration
Severity: error
Location: source/source_io/module_json/init_info.cpp:24
Reason: This newly added comma-separated declaration violates ABACUS baseline rule 8 and is rejected by the governance check.
Suggested action: Split the two declarations into separate statements.
Exception: not allowed
int ntype = ucell->ntype, nelec_total = 0;
source/source_io/module_json/output_info.cpp:35
- Rule: Global dependencies must not increase
Severity: error
Location: source/source_io/module_json/output_info.cpp:35
Reason: This backend rewrite adds a directPARAM.inp.cal_forceread in the JSON output path; the governance check reports it as part of a net increase of sevenGlobalV/GlobalC/PARAMreferences in the PR. The JSON helper now couples output serialization to mutable global configuration instead of receiving the flag it needs.
Suggested action: Passcal_forceexplicitly from the caller (and keep the JSON helper independent ofPARAM) so the PR's global-dependency budget is non-increasing.
Exception: not allowed
if (PARAM.inp.cal_force)
source/source_io/module_json/output_info.cpp:53
- Rule: Global dependencies must not increase
Severity: error
Location: source/source_io/module_json/output_info.cpp:53
Reason: This rewrite adds a directPARAM.inp.cal_stressread in the JSON output path; the governance check reports it as part of a net increase of sevenGlobalV/GlobalC/PARAMreferences in the PR. This makes the serializer depend on mutable global state rather than an explicit input.
Suggested action: Passcal_stressexplicitly from the caller (or pass a narrow output-options object) and remove the newPARAMdependency from this helper.
Exception: not allowed
if (PARAM.inp.cal_stress)
- Files reviewed: 48/49 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
10f3a66 to
d3ddd58
Compare
|
The re-integration exposed a bug that had previously been silently hidden by the RapidJSON-based implementation: the MD path could append SCF data before initializing an |
660dcab to
2890ea2
Compare
RapidJSON seems to be discontinued and has been no release for 10 years, and its CMake configuration is confusing (latest release vs main branch).
This PR replaces it with nlohmann-json for ABACUS JSON output, which is actively maintained, provides a simpler modern C++ interface, and offers well-supported CMake package integration through
nlohmann_jsonConfig.cmakeandnlohmann_json::nlohmann_jsontarget. This also makes downstream integration more straightforward: its distro packages such as Ubuntu'snlohmann-json3-devor Fedora'sjson-develalready ship the completed CMake config and exported target, avoiding the compatibility concerns around RapidJSON's decade-old upstream release.Test shows that semantic JSON behavior is unchanged, but textual floating-point formatting has difference (
0.0000121with RapidJSON vs1.21e-05with nlohmann-json).What's left for future PR: Further refactor and simplify JSON workflow, e.g. remove
jsonKeyNode.