Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Define per-cluster server and client targets in GN #36917

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ksperling-apple
Copy link
Contributor

Have chip_data_model targets depend on these, instead of making up a source set on the fly.

Early preview of this idea.

Have chip_data_model targets depend on these, instead of making up a source set on the fly.
Copy link

semanticdiff-com bot commented Dec 20, 2024

Review changes with  SemanticDiff

Changed Files
File Status
  src/app/zap_cluster_list.py  1% smaller
  examples/network-manager-app/linux/main.cpp Unsupported file format
  src/app/chip_data_model.gni Unsupported file format
  src/app/clusters/bindings/BUILD.gn Unsupported file format
  src/app/clusters/descriptor/BUILD.gn Unsupported file format
  src/app/clusters/descriptor/descriptor.cpp Unsupported file format
  src/app/clusters/thread-network-directory/BUILD.gn Unsupported file format
  src/app/clusters/thread-network-directory/DefaultThreadNetworkDirectoryStorage.cpp Unsupported file format
  src/app/clusters/thread-network-directory/DefaultThreadNetworkDirectoryStorage.h Unsupported file format
  src/app/clusters/thread-network-directory/ThreadNetworkDirectoryStorage.h Unsupported file format
  src/app/clusters/thread-network-directory/thread-network-directory-server.cpp Unsupported file format
  src/app/clusters/thread-network-directory/thread-network-directory-server.h Unsupported file format
  src/app/clusters/wifi-network-management/BUILD.gn Unsupported file format
  src/app/clusters/wifi-network-management/wifi-network-management-server.cpp Unsupported file format
  src/app/clusters/wifi-network-management/wifi-network-management-server.h Unsupported file format
  src/app/tests/BUILD.gn Unsupported file format
  src/app/tests/TestDefaultThreadNetworkDirectoryStorage.cpp Unsupported file format
  src/app/util/BUILD.gn Unsupported file format
  src/app/util/attribute-storage.h Unsupported file format
  src/app/util/ember-api.h Unsupported file format
  src/app/zap_cluster_list.json  0% smaller

Copy link

PR #36917: Size comparison from c1afc02 to 5728f01

Full report (3 builds for cc32xx, stm32)
platform target config section c1afc02 5728f01 change % change
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 539705 539713 8 0.0
RAM 205800 205800 0 0.0
lock CC3235SF_LAUNCHXL FLASH 573873 573865 -8 -0.0
RAM 205944 205944 0 0.0
stm32 light STM32WB5MM-DK FLASH 484712 484712 0 0.0
RAM 144912 144912 0 0.0

":time-sync-data-provider-test-srcs",
"${chip_root}/src/app",
"${chip_root}/src/app:attribute-persistence",
"${chip_root}/src/app/clusters/thread-network-directory:server",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should name the targets for cluster foo as foo so that the server suffix is not necessary (i.e. typical/default should have the same name as the directory, extra bits should be differently named if necessary)

@@ -75,3 +75,11 @@ source_set("callbacks") {
"${chip_root}/src/app:paths",
]
}

source_set("binding-table-headers") {
sources = [ "binding-table.h" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not split cpp from header. Even this is what happens in practice, this allows for circular/odd dependencies. We should add source sets only when we are able to fix things.

* This function returns true if
* the endpoint contains client of a given cluster.
*/
bool emberAfContainsClient(chip::EndpointId endpoint, chip::ClusterId clusterId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this splits the logic of "if xyz is defined in foo.h, it is implemented in foo.cpp". As a result then we have a "ember-api.h" that is available standalone even though the functions that it defines are not linked in and this breaks dependencies.


import("//build_overrides/chip.gni")

source_set("server") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
source_set("server") {
source_set("thread-network-directory") {


import("//build_overrides/chip.gni")

source_set("server") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
source_set("server") {
source_set("wifi-network-management") {

}

source_set("ember-api-headers") {
sources = [ "ember-api.h" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not have a header without the implementation cpp unless that header defines constants only. I.e. if I depend on foo I should be able to link. This breaks that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants