fix(cmake): install DeePMD config for C library builds#5475
Merged
Conversation
Installing DeePMD-kit from a precompiled C library skipped the CMake package config generation, so LAMMPS built-in mode could not resolve find_package(DeePMD). Generate DeePMDConfig.cmake and DeePMDTargets.cmake for DEEPMD_C_ROOT builds, and include the same package metadata in the packaged libdeepmd_c tarball. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGenerates and installs CMake package config/version files into lib/cmake/, adds templates declaring an imported DeePMD C library target, conditions installing an imported-targets file vs exported targets on DEEPMD_C_ROOT, moves include(CMakePackageConfigHelpers) earlier, and adds an install-time consumer verification step. ChangesCMake Package Configuration for Pre-Compiled C Library
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@source/api_c/CMakeLists.txt`:
- Around line 63-74: The PACKAGE_C block in source/api_c/CMakeLists.txt uses
variables config_file, version_file, generated_dir, and targets_export_name that
are only set when BUILD_CPP_IF AND NOT BUILD_PY_IF; either wrap the entire
PACKAGE_C section in the same guard (if(BUILD_CPP_IF AND NOT BUILD_PY_IF) ...
endif) or explicitly initialize those variables (config_file, version_file,
generated_dir, targets_export_name) unconditionally inside the if(PACKAGE_C)
branch so configure_package_config_file(), configure_file(), and install(FILES
...) always have defined values; update the block referencing
configure_package_config_file, configure_file, and install(FILES ...)
accordingly to prevent undefined-variable failures when PACKAGE_C is TRUE but
BUILD_CPP_IF/BUILD_PY_IF differ.
In `@source/cmake/DeePMDImportedTargets.cmake.in`:
- Around line 4-5: The INTERFACE_INCLUDE_DIRECTORIES for the imported target is
pointing to "`@DEEPMD_INCLUDE_C_DIR`@/deepmd" which causes downstream includes
like `#include` <deepmd/c_api.h> to resolve to include/deepmd/deepmd/c_api.h;
change the INTERFACE_INCLUDE_DIRECTORIES value to the install include root
"`@DEEPMD_INCLUDE_C_DIR`@" (leave IMPORTED_LOCATION as "`@deepmd_c`@") and make the
identical adjustment in the packaged-target template so exported consumers get
the correct include root; update the PROPERTIES block that sets
INTERFACE_INCLUDE_DIRECTORIES accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb3a563e-178e-41b1-9d32-c6ab11ac9a01
📒 Files selected for processing (5)
source/CMakeLists.txtsource/api_c/CMakeLists.txtsource/cmake/Config.cmake.insource/cmake/DeePMDImportedTargets.cmake.insource/cmake/DeePMDPackagedTargets.cmake.in
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/cmake/DeePMDPackagedTargets.cmake.in (1)
1-5: ⚡ Quick winAdd a target-existence guard around the imported-library
add_library()to prevent duplicate imported-target definitions.CMake’s
install(EXPORT ...)-generated Targets files don’t guarantee an idempotency guard for each imported target; whilefind_package()typically caches successful results (so repeated calls are less likely to re-include), this hand-written template can still be included directly or under different conditions, which would makeadd_library(... IMPORTED)fail ifDeePMD::deepmd_calready exists. Guarding the declaration is low-cost and removes that risk.♻️ Wrap the declaration in a target-existence guard
+if(NOT TARGET `@CMAKE_PROJECT_NAME`@::`@LIB_DEEPMD_C`@) add_library(`@CMAKE_PROJECT_NAME`@::`@LIB_DEEPMD_C`@ SHARED IMPORTED) set_target_properties( `@CMAKE_PROJECT_NAME`@::`@LIB_DEEPMD_C`@ PROPERTIES IMPORTED_LOCATION "${PACKAGE_PREFIX_DIR}/lib/@CMAKE_SHARED_LIBRARY_PREFIX@@LIB_DEEPMD_C@@CMAKE_SHARED_LIBRARY_SUFFIX@" INTERFACE_INCLUDE_DIRECTORIES "${PACKAGE_PREFIX_DIR}/include") +endif()🤖 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 `@source/cmake/DeePMDPackagedTargets.cmake.in` around lines 1 - 5, Wrap the imported target declaration in a target-existence guard: check if the target `@CMAKE_PROJECT_NAME`@::`@LIB_DEEPMD_C`@ does not exist before calling add_library(... IMPORTED) and move the subsequent set_target_properties(...) (the IMPORTED_LOCATION and INTERFACE_INCLUDE_DIRECTORIES properties) inside that same guard so you don't attempt to redefine an already-existing imported target; use CMake's if (NOT TARGET <target>) ... endif() around the add_library and set_target_properties calls.
🤖 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.
Nitpick comments:
In `@source/cmake/DeePMDPackagedTargets.cmake.in`:
- Around line 1-5: Wrap the imported target declaration in a target-existence
guard: check if the target `@CMAKE_PROJECT_NAME`@::`@LIB_DEEPMD_C`@ does not exist
before calling add_library(... IMPORTED) and move the subsequent
set_target_properties(...) (the IMPORTED_LOCATION and
INTERFACE_INCLUDE_DIRECTORIES properties) inside that same guard so you don't
attempt to redefine an already-existing imported target; use CMake's if (NOT
TARGET <target>) ... endif() around the add_library and set_target_properties
calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 76378965-02e3-412f-93fb-79d2ef8fc09b
📒 Files selected for processing (4)
source/CMakeLists.txtsource/api_c/CMakeLists.txtsource/cmake/DeePMDImportedTargets.cmake.insource/cmake/DeePMDPackagedTargets.cmake.in
🚧 Files skipped from review as they are similar to previous changes (2)
- source/api_c/CMakeLists.txt
- source/cmake/DeePMDImportedTargets.cmake.in
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5475 +/- ##
==========================================
- Coverage 82.25% 82.25% -0.01%
==========================================
Files 833 833
Lines 89094 89099 +5
Branches 4227 4225 -2
==========================================
+ Hits 73287 73288 +1
- Misses 14515 14519 +4
Partials 1292 1292 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Cover the downstream CMake scenario from issue deepmodeling#5035 by configuring a minimal consumer with find_package(DeePMD REQUIRED CONFIG) against the install prefix and linking DeePMD::deepmd_c. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@source/install/build_from_c.sh`:
- Around line 21-23: Several shell variables and command arguments are unquoted
causing word-splitting/globbing (ShellCheck SC2046/SC2086); update usages such
as CONSUMER_TMP_DIR (rm -rf ${CONSUMER_TMP_DIR} and mkdir -p
${CONSUMER_TMP_DIR}), SCRIPT_PATH, INSTALL_PREFIX, BUILD_TMP_DIR, and all
cmake/cmake --build invocations (including -j${NPROC} and any -D...=... args) to
use quoted expansions (e.g., "${CONSUMER_TMP_DIR}", "${SCRIPT_PATH}",
"${INSTALL_PREFIX}", "${BUILD_TMP_DIR}", and quote individual -D values or the
-j argument) so paths with spaces or special chars are safe and to eliminate
ShellCheck warnings.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 90fd3ce6-8da1-45cb-941f-4abcefcbb0a6
📒 Files selected for processing (1)
source/install/build_from_c.sh
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes CMake package discovery for builds that consume a precompiled libdeepmd_c via DEEPMD_C_ROOT, so downstream projects (e.g., LAMMPS built-in mode) can reliably find_package(DeePMD) and link DeePMD::deepmd_c.
Changes:
- Generate/install
DeePMDConfig.cmake+ version file forDEEPMD_C_ROOTbuilds and install a generated targets file that defines an importedDeePMD::deepmd_c. - Add templates for imported/packaged
DeePMDTargets.cmakegeneration. - Extend the CI-facing
build_from_c.shto build a small consumer project that validatesfind_package(DeePMD CONFIG)andDeePMD::deepmd_c.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| source/CMakeLists.txt | Generate/install CMake package config also for DEEPMD_C_ROOT builds and install generated imported targets file. |
| source/cmake/Config.cmake.in | Fix component-check package name used by the installed config. |
| source/cmake/DeePMDImportedTargets.cmake.in | New template to define an imported DeePMD::deepmd_c for DEEPMD_C_ROOT installs. |
| source/cmake/DeePMDPackagedTargets.cmake.in | New template to define an imported DeePMD::deepmd_c inside the libdeepmd_c tarball layout. |
| source/api_c/CMakeLists.txt | Add generation/installation of config + targets files into the packaged libdeepmd_c staging tree. |
| source/install/build_from_c.sh | Add a consumer CMake build step to validate installed package discovery/target availability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Guard the hand-written DeePMD::deepmd_c imported target templates so repeated find_package(DeePMD) calls in the same configure step do not attempt to redefine the target. Also quote path expansions in the C library build test script. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
wanghan-iapcm
approved these changes
May 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
libdeepmd_cskipped CMake package config generation.find_package(DeePMD)because noDeePMDConfig.cmake/DeePMDTargets.cmakewas installed.Change
DeePMDConfig.cmakeforDEEPMD_C_ROOTbuilds.DeePMD::deepmd_ctarget pointing at the imported C library.libdeepmd_ctarball.Verification
git diff --checkfind_package(DeePMD REQUIRED CONFIG)exposesDeePMD::deepmd_cwith the expected imported library/include paths.Fixes #5035
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Summary by CodeRabbit