[Cmake] Add option to exclude the integration files at sdk level from cmake.#43193
[Cmake] Add option to exclude the integration files at sdk level from cmake.#43193jadhavrohit924 wants to merge 4 commits intoproject-chip:masterfrom
Conversation
jadhavrohit924
requested review from
andy31415 and
shubhamdp
and removed request for
a team and
Copilot
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement to the CMake build system. It adds a new option, CHIP_APP_ENABLE_CLUSTER_CODEGEN_INTEGRATION, to conditionally include codegen integration source files. This allows for more flexible builds, particularly for code-driven examples that do not use the generated code. The all-devices-app for esp32 is refactored to leverage this new option, greatly simplifying its CMakeLists.txt by replacing long, manually maintained source file lists with directory-based source discovery. The changes are extensive but applied consistently and correctly across numerous cluster build files. This is a solid refactoring that enhances maintainability.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new CMake option CHIP_APP_ENABLE_CLUSTER_CODEGEN_INTEGRATION to allow applications to exclude cluster codegen integration files at the SDK level. It also refactors the all-devices-app ESP32 example to use directory-based source inclusion (SRC_DIRS) instead of explicit file lists, simplifying maintenance.
Changes:
- Added
CHIP_APP_ENABLE_CLUSTER_CODEGEN_INTEGRATIONoption (default ON) to chip_data_model.cmake for controlling codegen integration file inclusion - Wrapped CodegenIntegration.cpp and related codegen files in conditional blocks across ~50 cluster app_config_dependent_sources.cmake files
- Refactored examples/all-devices-app/esp32/main/CMakeLists.txt from explicit SRCS lists to SRC_DIRS for easier maintenance
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/chip_data_model.cmake | Adds CHIP_APP_ENABLE_CLUSTER_CODEGEN_INTEGRATION option with default value ON |
| src/app/clusters/*/app_config_dependent_sources.cmake | Wraps CodegenIntegration files in conditional blocks across multiple clusters |
| src/app/clusters/icd-management-server/app_config_dependent_sources.cmake | Wraps codegen integration, but incorrectly includes cluster implementation files in conditional |
| src/app/clusters/boolean-state-configuration-server/app_config_dependent_sources.cmake | Wraps CodegenIntegration but missing other codegen-dependent files from conditional |
| src/app/clusters/on-off-server/app_config_dependent_sources.cmake | Provides both codegen and code-driven implementations with if/else structure |
| src/app/clusters/level-control/app_config_dependent_sources.cmake | Provides both codegen and code-driven implementations with if/else structure |
| examples/all-devices-app/esp32/main/CMakeLists.txt | Refactors from explicit file lists to SRC_DIRS, but pulls in extra unintended files |
There was a problem hiding this comment.
The ICDManagementCluster.cpp and ICDManagementCluster.h files should not be inside the CHIP_APP_ENABLE_CLUSTER_CODEGEN_INTEGRATION conditional block. These files are always included in the BUILD.gn source_set and are not part of the codegen integration sources. Only CodegenIntegration.cpp should be conditionally included, as indicated by the app_config_dependent_sources.gni file. These cluster implementation files should be moved to a separate TARGET_SOURCES block outside the conditional, similar to how other clusters like groups-server and software-diagnostics-server handle this.
src/app/clusters/boolean-state-configuration-server/app_config_dependent_sources.cmake
Outdated
Show resolved
Hide resolved
|
PR #43193: Size comparison from 1be2abb to 085356b Full report (27 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32)
|
There was a problem hiding this comment.
rather than making these includes at cmake-level, how about including cluster dependencies in https://github.com/project-chip/connectedhomeip/blob/master/BUILD.gn#L144 (i.e. default libchip build) and then we can have only codegen specific files in cmake?
This should then avoid having to manually add individual clusters to apps and get LTO cleaning out what is not needed.
There was a problem hiding this comment.
Actually, instead of "default" we probably want https://github.com/project-chip/connectedhomeip/blob/master/src/lib/BUILD.gn#L19
That one gets placed into libchip.
There was a problem hiding this comment.
Added to libchip
jadhavrohit924
force-pushed
the
cmake-sdk-build
branch
from
085356b to
d254eaf
Compare
src/app/clusters/scenes-server/app_config_dependent_sources.cmake
Outdated
Show resolved
Hide resolved
|
PR #43193: Size comparison from 4f9f696 to 1b4d57d Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
3 participants
Summary
Related issues
Testing
CI should pass.