[toolchain] Harden installation failure handling and RapidJSON CMake support#7583
[toolchain] Harden installation failure handling and RapidJSON CMake support#7583QuantumMisaka wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Isn't it duplicated effort of leveraging RapidJSONConfig.cmake (including RapidJSONTargets.cmake)?? There should not be any other fallbacks since the target exported is (and should be) totally decided on the RapidJSON side!
There was a problem hiding this comment.
Addressed. The fallback helper was removed; ABACUS now relies on the exported RapidJSON CMake target and fails if the package config does not provide it.
If the system-installed package has a incomplete CMake configuration, for example, RapidJSON installed from |
| if(ENABLE_RAPIDJSON) | ||
| find_package(RapidJSON CONFIG REQUIRED) | ||
| abacus_add_feature_definitions(__RAPIDJSON) | ||
| target_link_libraries(abacus_external_deps INTERFACE RapidJSON) | ||
| include(cmake/AbacusRapidJSON.cmake) | ||
| abacus_configure_rapidjson(abacus_external_deps) | ||
| endif() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Addressed. The fallback path was dropped; ENABLE_RAPIDJSON now requires the exported RapidJSON target and links it through abacus_external_deps.
mohanchen
added
the
Compile & CICD & Docs & Dependencies
Growl1234
left a comment
Growl1234
left a comment
There was a problem hiding this comment.
Honestly, I don't think it's worth bringing in too many helpers just for testing small case; the maintenance burden of toolchain is already high, and fully assigning it to an AI agent should have not been really a good idea...
At least, keeping only non-overlapping behavioral checks would be clearer and less brittle.
| EOF | ||
| } | ||
|
|
||
| write_test_project() { |
There was a problem hiding this comment.
Could we avoid duplicating the production RapidJSON block in this fixture? The test then needs the source-text checks below to ensure that CMakeLists.txt stays in sync with this copy. A behavioral test of the actual top-level configuration would leave a single source of truth.
There was a problem hiding this comment.
Addressed in 10f35c561. The duplicated fixture block was removed; the test now runs the real top-level configure and checks the generated codemodel.
| rm -rf "$tmpdir" | ||
| } | ||
|
|
||
| test_variable_only_package_fails() { |
There was a problem hiding this comment.
This is the same variable-only negative case that test_top_level_rejects_variable_only_package() exercises immediately below. The latter covers the real consumer, so the standalone case can be removed without losing regression coverage.
There was a problem hiding this comment.
Addressed in 10f35c561. The standalone variable-only case was removed; coverage remains in the real top-level rejection test.
| if(ENABLE_RAPIDJSON) | ||
| find_package(RapidJSON CONFIG REQUIRED) | ||
| if(NOT TARGET RapidJSON) | ||
| message(FATAL_ERROR "RapidJSON package configuration did not define the RapidJSON target") |
There was a problem hiding this comment.
Also, we can make message clearer:
| message(FATAL_ERROR "RapidJSON package configuration did not define the RapidJSON target") | |
| message( | |
| FATAL_ERROR | |
| "RapidJSON was found, but target RapidJSON is missing. Check if your RapidJSON installation provides a complete exported CMake configuration." | |
| ) |
There was a problem hiding this comment.
Applied in 10f35c561.
| >"${build_dir}.log" 2>&1 | ||
| } | ||
|
|
||
| assert_abacus_target_has_rapidjson_usage() { |
There was a problem hiding this comment.
I'm considering if this goes beyond the contract changed in this PR. The existing target_link_libraries(... RapidJSON) path is not modified here; the new behavior is only that a config package must define the canonical RapidJSON target. The two top-level configure cases already cover that contract, so could we drop the File API/Python inspection and keep this test focused?
3 participants
Linked Issue
Fix #7565 #7566 #7569
Summary
This PR hardens the ABACUS toolchain installation flow for issues found during toolchain/install review.
Changes include:
Root Cause
The wrapper scripts could mask failures from lower-level installers or argument validation paths, making a failed install appear successful.
RapidJSON was installed as headers only, which left CMake package discovery fragile for builds that expect an installed package layout.
Validation
Validated on the SAI CPU-MISC/rush-cpu environment from commit
583c5dc66.Checks run:
toolchain/tests/test_wrapper_failure_propagation.shtoolchain/tests/test_installer_argument_failures.shtoolchain/tests/test_rapidjson_cmake.shtoolchain/toolchain_gnu.sh -j 2installation./build_abacus_gnu.sh -j 2with RapidJSON enabledexamples/02_scf/01_pw_Si2The final real calculation job was Slurm
619964, completed withExitCode=0:0, five SCF iterations (DS1-DS5), andTOTAL Time : 2in ABACUS output.Note: the full verification environment hit intermittent GitHub/proxy TLS EOFs while fetching some release tarballs. After supplying checksum-matching source archives, the same installation
continued and passed. These network failures also confirmed that wrapper-level failures now propagate as non-zero exits.