Example gn flow#882
Draft
Michael Duggan (miduggan24) wants to merge 5 commits intofeature/CRTP_lightingfrom
Draft
Example gn flow#882Michael Duggan (miduggan24) wants to merge 5 commits intofeature/CRTP_lightingfrom
Michael Duggan (miduggan24) wants to merge 5 commits intofeature/CRTP_lightingfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Script skips copy when templates change, causing stale builds
- Removed the early-exit check that prevented template updates from being copied, allowing GN's timestamp tracking to properly detect and propagate template changes.
- ✅ Fixed: Script argument
app_project_diraccepted but never used- Implemented logic to check for app-specific CustomAppTask files in the app_project_dir before falling back to platform defaults.
Or push these changes by commenting:
@cursor push ff23a04ee8
Preview (ff23a04ee8)
diff --git a/examples/platform/silabs/ensure_local_custom_app_task.py b/examples/platform/silabs/ensure_local_custom_app_task.py
--- a/examples/platform/silabs/ensure_local_custom_app_task.py
+++ b/examples/platform/silabs/ensure_local_custom_app_task.py
@@ -31,10 +31,19 @@
base = os.getcwd()
platform_dir = os.path.normpath(os.path.abspath(os.path.join(base, sys.argv[1])))
+ app_project_dir = os.path.normpath(os.path.abspath(os.path.join(base, sys.argv[2])))
build_dir = os.path.normpath(os.path.abspath(os.path.join(base, sys.argv[3])))
- template_cpp = os.path.join(platform_dir, "CustomAppTask.cpp")
- template_h = os.path.join(platform_dir, "CustomAppTask.h")
+ app_cpp = os.path.join(app_project_dir, "CustomAppTask.cpp")
+ app_h = os.path.join(app_project_dir, "CustomAppTask.h")
+
+ if os.path.isfile(app_cpp) and os.path.isfile(app_h):
+ template_cpp = app_cpp
+ template_h = app_h
+ else:
+ template_cpp = os.path.join(platform_dir, "CustomAppTask.cpp")
+ template_h = os.path.join(platform_dir, "CustomAppTask.h")
+
build_config_dir = os.path.join(build_dir, "config")
dest_cpp = os.path.join(build_config_dir, "CustomAppTask.cpp")
dest_h = os.path.join(build_config_dir, "CustomAppTask.h")
@@ -44,10 +53,6 @@
sys.stderr.write("template not found: %s\n" % path)
sys.exit(1)
- # Only copy when build dir does not have a copy
- if os.path.isfile(dest_cpp):
- return
-
os.makedirs(build_config_dir, exist_ok=True)
shutil.copy2(template_cpp, dest_cpp)
shutil.copy2(template_h, dest_h)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Script receives wrong argument as build directory
- Removed the unnecessary silabs_project_dir argument from the GN action's args list so sys.argv[2] correctly receives root_build_dir as the Python script expects.
Or push these changes by commenting:
@cursor push d91df2b3ec
Preview (d91df2b3ec)
diff --git a/examples/lighting-app/silabs/BUILD.gn b/examples/lighting-app/silabs/BUILD.gn
--- a/examples/lighting-app/silabs/BUILD.gn
+++ b/examples/lighting-app/silabs/BUILD.gn
@@ -50,7 +50,6 @@
script = "${examples_common_plat_dir}/ensure_local_custom_app_task.py"
args = [
rebase_path(examples_common_plat_dir, root_build_dir),
- rebase_path(silabs_project_dir, root_build_dir),
rebase_path(root_build_dir, root_build_dir),
]
inputs = [
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Incomplete file existence check skips copying header file
- Modified the Python script to check both dest_cpp and dest_h files exist before skipping the copy operation.
- ✅ Fixed: CustomAppTask forced into all silabs apps breaks builds
- Removed CustomAppTask from the shared silabs_executable template and added it back to the lighting-app BUILD.gn where it belongs.
Or push these changes by commenting:
@cursor push 3dff85771e
Preview (3dff85771e)
diff --git a/examples/lighting-app/silabs/BUILD.gn b/examples/lighting-app/silabs/BUILD.gn
--- a/examples/lighting-app/silabs/BUILD.gn
+++ b/examples/lighting-app/silabs/BUILD.gn
@@ -121,7 +121,10 @@
}
silabs_executable("lighting_app") {
output_name = "matter-silabs-lighting-example.out"
- include_dirs = [ "include" ]
+ include_dirs = [
+ "include",
+ "${root_build_dir}/config",
+ ]
defines = []
if (silabs_board == "BRD2704A") {
@@ -130,6 +133,7 @@
sources = [
"${examples_common_plat_dir}/main.cpp",
+ "${root_build_dir}/config/CustomAppTask.cpp",
"src/AppTask.cpp",
"src/DataModelCallbacks.cpp",
]
@@ -138,6 +142,7 @@
":sdk",
"${chip_root}/src/app/persistence:deferred",
"${chip_root}/src/platform/logging:default",
+ "${silabs_sdk_build_root}:ensure_custom_app_task",
app_data_model,
]
diff --git a/third_party/silabs/ensure_local_custom_app_task.py b/third_party/silabs/ensure_local_custom_app_task.py
--- a/third_party/silabs/ensure_local_custom_app_task.py
+++ b/third_party/silabs/ensure_local_custom_app_task.py
@@ -44,7 +44,7 @@
sys.exit(1)
# Only copy when build dir does not have a copy
- if os.path.isfile(dest_cpp):
+ if os.path.isfile(dest_cpp) and os.path.isfile(dest_h):
return
os.makedirs(build_config_dir, exist_ok=True)
diff --git a/third_party/silabs/silabs_executable.gni b/third_party/silabs/silabs_executable.gni
--- a/third_party/silabs/silabs_executable.gni
+++ b/third_party/silabs/silabs_executable.gni
@@ -18,8 +18,6 @@
import("${build_root}/toolchain/flashable_executable.gni")
import("silabs_board.gni")
-_custom_app_task_dep = "${silabs_sdk_build_root}:ensure_custom_app_task"
-
template("generate_rps_file") {
forward_variables_from(invoker,
[
@@ -106,17 +104,10 @@
]
flashbundle_name = "" # Stop flashable_executable from making flashbundle.
- # copy CustomAppTask from template into build directory
- _custom_app_task_gen_dir = "${root_build_dir}/config"
-
# Target to generate the s37 file, flashing script, and flashbundle.
flash_target_name = target_name + ".flash_executable"
flashable_executable(flash_target_name) {
forward_variables_from(invoker, "*")
-
- sources += [ "${_custom_app_task_gen_dir}/CustomAppTask.cpp" ]
- include_dirs += [ _custom_app_task_gen_dir ]
- deps += [ _custom_app_task_dep ]
}
# Target to generate the hex file.|
|
||
| # Only copy when build dir does not have a copy | ||
| if os.path.isfile(dest_cpp): | ||
| return |
There was a problem hiding this comment.
Incomplete file existence check skips copying header file
Medium Severity
The early-return guard only checks whether dest_cpp exists, ignoring dest_h. If CustomAppTask.cpp is present but CustomAppTask.h is missing (e.g., partially deleted or a failed prior run), the script returns without copying the header. GN declares both files as outputs, so it will re-run the action when the .h is missing, but the script will keep skipping — resulting in a persistent build failure that's hard to diagnose. Both destination files need to be checked.
Additional Locations (1)
|
|
||
| sources += [ "${_custom_app_task_gen_dir}/CustomAppTask.cpp" ] | ||
| include_dirs += [ _custom_app_task_gen_dir ] | ||
| deps += [ _custom_app_task_dep ] |
There was a problem hiding this comment.
CustomAppTask forced into all silabs apps breaks builds
High Severity
CustomAppTask.cpp is unconditionally added to every silabs_executable target via sources +=. However, CustomAppTask.h includes "AppTaskImpl.h", which only exists in the lighting-app's include directory — no other silabs app (air-quality-sensor, closure, chef, base-platform, dishwasher) provides it. This will cause compilation failures for all non-lighting silabs applications. Additionally, these other apps define their own incompatible AppTask classes with their own GetAppTask() implementations, which would create linker conflicts even if the header were found.
Additional Locations (1)
sarthak shaha (Sarthak-Shaha)
force-pushed
the
feature/CRTP_lighting
branch
2 times, most recently
from
7a401e3 to
b64128e
Compare
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
1 participant
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.
Summary
Related issues
Testing
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines
Note
Medium Risk
Build-graph changes affect all Silabs GN targets by introducing a new action and source injection, which could break builds if the copy step/path assumptions are wrong.
Overview
Moves Silabs examples away from compiling an in-tree
src/CustomAppTask.cppand instead generates a per-buildCustomAppTaskby copying the template fromexamples/platform/silabsinto${root_build_dir}/config.Adds an
ensure_custom_app_taskGNaction(plusensure_local_custom_app_task.py) and wiressilabs_executableto depend on it, automatically adding the generatedCustomAppTask.cppand include path to Silabs firmware targets.Written by Cursor Bugbot for commit 356ba1d. This will update automatically on new commits. Configure here.