Toolchain/CMake: Improve dependency handling#7449
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR shifts RapidJSON and cereal integration to rely on proper CMake installs/packages (instead of manual header copying / FetchContent fallbacks) and updates CI/toolchain workflows to run consistently under bash and install required stage4 tools.
Changes:
- Install RapidJSON and cereal via CMake build+install in the toolchain, and stop exporting include paths via
CPATH. - Update project CMake to require
RapidJSONandcerealviafind_package(...), removing the customFindCereal.cmakeand RapidJSON FetchContent fallback. - CI workflows: standardize
defaults.run.shell: bash, ensurexztooling is present, and install stage4 external tools before building.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/scripts/stage4/install_rapidjson.sh | Switch RapidJSON toolchain install to CMake configure/install; remove CPATH export. |
| toolchain/scripts/stage4/install_cereal.sh | Switch cereal toolchain install to CMake configure/install; remove CPATH export. |
| toolchain/root_requirements/install_requirements_ubuntu.sh | Add xz-utils dependency. |
| toolchain/root_requirements/install_requirements_fedora.sh | Add xz dependency. |
| toolchain/build_abacus_intel.sh | Remove manual CEREAL/RAPIDJSON paths passed to CMake. |
| toolchain/build_abacus_gnu.sh | Remove manual CEREAL/RAPIDJSON paths passed to CMake. |
| toolchain/build_abacus_gcc-mkl.sh | Remove manual CEREAL/RAPIDJSON paths passed to CMake. |
| toolchain/build_abacus_gcc-aocl.sh | Remove manual CEREAL/RAPIDJSON paths passed to CMake. |
| cmake/FindCereal.cmake | Remove custom cereal find-module (move to config package). |
| CMakeLists.txt | Require RapidJSON/cereal via find_package(...) and drop FetchContent/include-directories logic. |
| .github/workflows/version_check.yml | Set default bash shell for run: steps. |
| .github/workflows/toolchain_quick.yaml | Set default bash shell for run: steps. |
| .github/workflows/toolchain_full.yaml | Set default bash shell for run: steps. |
| .github/workflows/test.yml | Install stage4 external tools via toolchain script (broader than dftd4-only). |
| .github/workflows/pytest.yml | Set default bash shell for run: steps. |
| .github/workflows/performance.yml | Set default bash shell for run: steps. |
| .github/workflows/mirror_gitee.yml | Set default bash shell for run: steps. |
| .github/workflows/interface.yml | Set default bash shell for run: steps. |
| .github/workflows/dynamic.yml | Set default bash shell; add toolchain external tools install; source toolchain env before build. |
| .github/workflows/doxygen.yml | Set default bash shell for run: steps. |
| .github/workflows/devcontainer.yml | Set default bash shell for run: steps. |
| .github/workflows/cuda.yml | Install xz-utils; install stage4 external tools; source toolchain env before build. |
| .github/workflows/coverage.yml | Install xz-utils; install stage4 external tools; source toolchain env before build. |
| .github/workflows/build_test_makefile.yml | Set default bash shell for run: steps. |
| .github/workflows/build_test_cmake.yml | Add toolchain install step, source toolchain env; set default bash shell; matrix toolchain args. |
| .github/workflows/ase_plugin_test.yml | Install xz-utils; install stage4 external tools; source toolchain env before build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
If possible, it's better that some other libraries such as libRI and libComm also be applied a simple CMake system along with properly-generated config files. |
b58a7b3 to
833f30e
Compare
|
I have switched to interface targets, which provide a more modern CMake approach for ensuring stable propagation of dependency usage requirements. |
Following this change there must be some more refactors, for example, |
|
LGTM mostly, the comments below are making by GPT for reference: Thanks for the cleanup here. The overall direction makes sense: moving cereal / RapidJSON / Libxc discovery toward the toolchain-provided standard CMake package flow, and removing some of the legacy custom lookup logic. That said, I do not think this is ready to merge yet. I see two blocking risks in the current version:
The switch to
At the same time, the toolchain wrapper scripts no longer pass Even if the default in-tree toolchain flow still works because I think we need to preserve one of those compatibility paths before merging, either by keeping a fallback
This PR moves feature/macro propagation to target-level Those old Even if this does not break every test immediately, it is still a configuration regression and should be resolved in the same PR. I would strongly recommend migrating the existing test directories to the new disabling mechanism (for example, the new Because of these two points, I would request changes for now. |
|
@QuantumMisaka I will push back on these points. For Libxc, removing The correct CMake-side usage should be to rely on the dependency's own config file if it's built with CMake. For example, if Preserving the old prefix-style For the feature propagation point, I agree that For example, the TDDFT LCAO test failure exposed by CI was caused by an old Therefore, I agree that remaining If there are still |
QuantumMisaka
left a comment
There was a problem hiding this comment.
Thanks for the cleanup. I agree with the CMake direction here: consuming Libxc/cereal/RapidJSON through their installed config packages and using CMAKE_PREFIX_PATH for installation prefixes is the right long-term interface. I do not think we should preserve prefix-style Libxc_DIR as the preferred API.
However, the current PR is not yet consistent with that new contract in the Toolchain and user-facing install paths. I verified this on the SAI environment:
find_package(Libxc REQUIRED)works withCMAKE_PREFIX_PATH=/opt/devtools/libxc/libxc-7.0.0-avx2.- It also works with exact
Libxc_DIR=/opt/devtools/libxc/libxc-7.0.0-avx2/lib/cmake/Libxc. - It fails with the old documented prefix-style
Libxc_DIR=/opt/devtools/libxc/libxc-7.0.0-avx2, which is expected under standard CMake semantics. - More importantly, sourcing an existing ABACUS Toolchain setup generated with
--with-libxc=systemdoes not expose Libxc throughCMAKE_PREFIX_PATH; configuring this PR then fails atCMakeLists.txt:859 (find_package)until the Libxc prefix is added manually. - I also verified the PR's default CMake-install approach for cereal and RapidJSON: installing both with the new CMake install commands produces
cerealConfig.cmake/RapidJSONConfig.cmake, andfind_package(... CONFIG REQUIRED)finds the expectedcereal::cerealandRapidJSONtargets. So the default install path is fine; the problem is the non-install/system/user discovery contract.
Requests:
-
toolchain/scripts/stage3/install_libxc.sh:__SYSTEM__and user-provided modes still validate headers/libs and export Makefile-style flags, but the CMake build now needs a discoverableLibxcConfig.cmake. Please make these modes export a usable Libxc prefix throughCMAKE_PREFIX_PATH, or fail early with a clear message that Libxc must be CMake-installed and discoverable as a package config. -
toolchain/scripts/stage4/install_cereal.sh:__SYSTEM__mode currently only checks forcereal/cereal.hpp. A header-only system install will pass Toolchain setup and then fail at ABACUS configure withfind_package(cereal CONFIG REQUIRED). Please validatecerealConfig.cmake/cereal::cerealand export the prefix throughCMAKE_PREFIX_PATH, or fail in the Toolchain stage. -
toolchain/scripts/stage4/install_rapidjson.sh: same issue as cereal. The system path only provesrapidjson/rapidjson.hexists, but ABACUS now requiresRapidJSONConfig.cmakeand theRapidJSONtarget. -
Docs:
docs/advanced/install.mdanddocs/quick_start/easy_install.mdstill recommend prefix-styleLibxc_DIR,CEREAL_INCLUDE_DIR, and say definingLibxc_DIRenables Libxc. Please update these to the new contract:-DENABLE_LIBXC=ONplusCMAKE_PREFIX_PATH=<install-prefixes>, while noting that<Package>_DIRmust point to the exact config-file directory if used directly. -
Optional cleanup:
toolchain/build_abacus_aocc-aocl.shstill passes oldCEREAL_INCLUDE_DIRandRapidJSON_DIR=$RAPIDJSON_ROOT. These are not the main blocker if setup exports the right prefix, but they keep the obsolete interface alive and should be removed for consistency with the other wrapper scripts.
These two sentences sounds inconsistent... And as mentioned above,
The variable
It's clear that with current clean-up the "non-install" path is no longer supported. I don't really understand why one would prefer that way...
Thanks, I will work on this. |
|
Regarding Cereal and RapidJSON detection, I'll also try cleanups |
|
And currently, the system finding logic of cereal would possibly be not always valid because Unfortunately, building cereal would not pull out a library (e.g. Regarding libxc, I've checked the system installation of Ubuntu (to 22.04) and Fedora (to 43) through https://pkgs.org, and the results show that all those systems build DEB/RPM package of libXC with CMake, and |
QuantumMisaka
left a comment
There was a problem hiding this comment.
I re-tested the current head (06dca8d8768169dd09c40f14a18af0bfd0eff976) with the PR Toolchain-installed cereal/RapidJSON packages and SAI's existing CMake-package Libxc.
The Toolchain side for the default install path looks good for the packages touched here: the PR installs
cereal-22a1b36/lib/cmake/cereal/cerealConfig.cmakerapidjson-24b5e7a/lib/cmake/RapidJSON/RapidJSONConfig.cmake
and exports their install prefixes through CMAKE_PREFIX_PATH. A minimal GNU Toolchain run also succeeds with and without --enable-cuda --gpu-ver=70.
However, the source build is still blocked when ENABLE_LCAO=ON and ENABLE_LIBXC=ON, even though CMake configuration succeeds and finds all package configs:
Libxc_DIR=/opt/devtools/libxc/libxc-7.0.0-avx2/lib/cmake/Libxccereal_DIR=/tmp/abacus-pr7449-verify/toolchain/install/cereal-22a1b36/lib/cmake/cerealRapidJSON_DIR=/tmp/abacus-pr7449-verify/toolchain/install/rapidjson-24b5e7a/lib/cmake/RapidJSON
CPU/GNU build command failed at rdmft_tools.cpp:
source/source_hamilt/module_xc/xc_functional.h:9:10: fatal error: xc.h: No such file or directory
The same failure appears in the GNU+CUDA configuration after CMake finds CUDA 12.4/nvcc and generates successfully.
Root cause: the PR correctly propagates RapidJSON and cereal through abacus_external_deps, but Libxc is only linked to ${ABACUS_BIN_NAME}:
target_link_libraries(${ABACUS_BIN_NAME} PRIVATE Libxc::xc)
abacus_add_feature_definitions(USE_LIBXC)At the same time, USE_LIBXC is propagated to object libraries such as rdmft; those targets include headers that reach <xc.h>, but they do not receive Libxc::xc's INTERFACE_INCLUDE_DIRECTORIES. Previously this was hidden by global include_directories(${Libxc_INCLUDE_DIRS}).
So I do not think this PR is ready aside from documentation. Please propagate Libxc usage requirements to the targets that compile Libxc-dependent sources, preferably consistently with the new design, e.g. by adding Libxc::xc to abacus_external_deps when ENABLE_LIBXC=ON rather than linking it only to the final executable.
|
Thanks for reporting this issue. I'll have a look at why this could happen and give a further clean-up. |
|
This cleanup can hopefully fix the issue. It might take some time before submitting a change related to Docs as I'm currently busy on CMake regression fixings at another repos. |
4483166 to
908b32d
Compare
Cereal and RapidJSON have their own CMake build systems, which should be used during toolchain installation rather than relying on direct file copies. Therefore, the custom logic for finding these two dependencies should no longer be needed, as it brings avoidable issues and maintenance work.
This PR switches the installation method of the two packages to CMake, and removes the related custom logic, including the
FindCereal.cmakefile. TheFetchContentfallback is also dropped, since installing these dependencies is the responsibility of the toolchain.On the ABACUS CMake side, cereal and RapidJSON are now consumed through interface targets, so their include directories and compile definitions are propagated through CMake usage requirements in a more explicit and stable way.
The
FindLibxc.cmakefile is also removed in favor of libxc's own CMake config files.