[Silabs] : use different markers to save user preferences and install done#71492
[Silabs] : use different markers to save user preferences and install done#71492Sarthak-Shaha wants to merge 5 commits intoproject-chip:masterfrom
Conversation
Sarthak-Shaha
changed the title
![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 refactors the package installation logic in the Silabs example script to better handle first-time installations and Docker environments. The reviewer identified a high-severity bug where the installation logic would incorrectly execute inside Docker containers, and suggested an improvement to the readability of the variable assignment logic for INSTALL_EVERYTHING.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Silabs package installation logic in the gn_silabs_example.sh script. It introduces a mechanism to track user opt-outs via a .do-not-install-packages file and ensures that the installation script is executed when a previous installation is detected to check for updates. Feedback was provided to correct a misleading comment that stated the installation step was skipped when, in fact, the code triggers an update check.
|
PR #71492: Size comparison from 15b505f to 49a3fe4 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #71492 +/- ##
=======================================
Coverage 54.31% 54.32%
=======================================
Files 1576 1576
Lines 108247 108247
Branches 13401 13400 -1
=======================================
+ Hits 58796 58801 +5
+ Misses 49451 49446 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Silabs package installation logic in the gn_silabs_example.sh script. It introduces a mechanism to track user opt-outs via a .do-not-install-packages file and ensures that the installation prompt is skipped if the user has previously declined. The installation block was also updated to prevent execution within Docker environments. A review comment suggests adding an exit condition to the pip install command to ensure the script terminates if dependency installation fails.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the Silabs package installation logic in gn_silabs_example.sh to support update checks and persistent opt-out via a .do-not-install-packages file. Reviewers identified that the script allows users to skip 'required' packages without a fatal error, which may lead to confusing build failures later. Additionally, it was suggested to make update checks non-fatal to prevent build failures when working offline.
| [Nn]*) | ||
| echo "You won't be asked again" | ||
| touch "$CHIP_ROOT/scripts/setup/silabs/.do-not-install-packages" | ||
| break | ||
| ;; # Case for no/N |
There was a problem hiding this comment.
The prompt explicitly states that these packages are 'required for the build to succeed'. However, if the user chooses 'No', the script merely records the preference and continues execution. This will likely lead to confusing build failures later in the process. Consider exiting the script with an informative error message if the user refuses to install required dependencies, or at least provide a clear warning that the build is expected to fail.
| if [ "$INSTALL_EVERYTHING" == true ] && [ "$USE_DOCKER" != true ]; then # run install-packages | ||
| pip install -r "$CHIP_ROOT/integrations/docker/images/stage-2/chip-build-efr32/requirements.txt" || exit 1 | ||
| python3 "$CHIP_ROOT/scripts/setup/silabs/install-packages.py" || exit 1 | ||
| fi # run install-packages |
There was a problem hiding this comment.
Adding || exit 1 to the update commands and running them on every execution makes the build process strictly dependent on a network connection. If a developer is offline or has network issues, the build will fail even if the SDK was previously installed correctly. Consider making these update checks non-fatal if the initial installation has already been completed (i.e., when .install-packages-done exists).
| if [ "$INSTALL_EVERYTHING" == true ] && [ "$USE_DOCKER" != true ]; then # run install-packages | |
| pip install -r "$CHIP_ROOT/integrations/docker/images/stage-2/chip-build-efr32/requirements.txt" || exit 1 | |
| python3 "$CHIP_ROOT/scripts/setup/silabs/install-packages.py" || exit 1 | |
| fi # run install-packages | |
| if [ "$INSTALL_EVERYTHING" == true ] && [ "$USE_DOCKER" != true ]; then # run install-packages | |
| if [ -f "$CHIP_ROOT/scripts/setup/silabs/.install-packages-done" ]; then | |
| pip install -r "$CHIP_ROOT/integrations/docker/images/stage-2/chip-build-efr32/requirements.txt" || echo "Warning: Failed to check for pip updates." | |
| python3 "$CHIP_ROOT/scripts/setup/silabs/install-packages.py" || echo "Warning: Failed to check for Silabs package updates." | |
| else | |
| pip install -r "$CHIP_ROOT/integrations/docker/images/stage-2/chip-build-efr32/requirements.txt" || exit 1 | |
| python3 "$CHIP_ROOT/scripts/setup/silabs/install-packages.py" || exit 1 | |
| fi | |
| fi # run install-packages |
2 participants
Summary
The install marker changes from #43611 skips the installation steps for checking new versions of SiSDK.
Updated the flow to introduce a new user consent marker
Following the flow chart
Related issues
Testing
Run scripts/examples/gn_silabs_example.sh
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