Implement a generic "look-up" snippet for finding $CHIP_ROOT#72154
Open
enkiusz wants to merge 11 commits into
Open
Implement a generic "look-up" snippet for finding $CHIP_ROOT#72154enkiusz wants to merge 11 commits into
enkiusz wants to merge 11 commits into
Conversation
github-actions
Bot
added
documentation
for more information, see https://pre-commit.ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the repository root detection logic across various scripts to use a dynamic search for the 'SPECIFICATION_VERSION' file. Review feedback highlights several critical issues, including missing 'Path' imports in multiple files and the accidental removal of the 'TEST_EXTPANID' constant in several Cirque test scripts. Additionally, it is recommended to use '.resolve()' for cross-platform path consistency, implement error handling for root detection failures, and remove a leftover debug script.
I am having trouble creating individual review comments. Click here to see my feedback.
examples/chef/chef.py (37)
The Path class is used here but it has not been imported in this file. This will cause a NameError at runtime. Please verify that the symbol is not already imported or defined in an outer scope.
References
- Before flagging a missing import, verify that the symbol is not already imported within the current file scope.
- When analyzing for potential NameError exceptions, ensure the variable is not already defined in an outer scope before suggesting an initialization.
scripts/tests/matter_yaml_linter.py (24)
The Path class is used here but it has not been imported in this file. This will cause a NameError at runtime. Please verify that the symbol is not already imported or defined in an outer scope.
References
- Before flagging a missing import, verify that the symbol is not already imported within the current file scope.
- When analyzing for potential NameError exceptions, ensure the variable is not already defined in an outer scope before suggesting an initialization.
scripts/tests/run_test_suite.py (52)
The Path class is used here but it has not been imported in this file. This will cause a NameError at runtime. Please verify that the symbol is not already imported or defined in an outer scope.
References
- Before flagging a missing import, verify that the symbol is not already imported within the current file scope.
- When analyzing for potential NameError exceptions, ensure the variable is not already defined in an outer scope before suggesting an initialization.
credentials/development/gen_commissioner_dut_test_plan_table.py (32)
The current implementation for finding the repository root is fragile. Using .resolve() ensures consistency across different platforms and invocation methods. Additionally, handling the case where the root is not found prevents unhandled StopIteration exceptions.
CHIP_ROOT = next((p for p in Path(__file__).resolve().parents if (p / 'SPECIFICATION_VERSION').is_file()), None)
if CHIP_ROOT is None:
raise RuntimeError("Could not find CHIP root (looking for 'SPECIFICATION_VERSION' in parent directories)")
References
- When applying workarounds in build or generation scripts, prioritize consistency across platforms to simplify maintenance.
src/test_driver/linux-cirque/FailsafeTest.py (37)
The constant TEST_EXTPANID was accidentally removed during this refactor. This will likely cause the test to fail if it is used elsewhere in the script.
CHIP_REPO = next(filter(lambda p: (p / 'SPECIFICATION_VERSION').is_file(), Path(__file__).resolve().parents))
TEST_EXTPANID = "fedcba9876543210"
src/test_driver/linux-cirque/IcdDeviceTest.py (37)
The constant TEST_EXTPANID was accidentally removed during this refactor. It should be restored.
CHIP_REPO = next(filter(lambda p: (p / 'SPECIFICATION_VERSION').is_file(), Path(__file__).resolve().parents))
TEST_EXTPANID = "fedcba9876543210"
src/test_driver/linux-cirque/SubscriptionResumptionCapacityTest.py (51)
The constant TEST_EXTPANID was accidentally removed during this refactor. It should be restored.
CHIP_REPO = next(filter(lambda p: (p / 'SPECIFICATION_VERSION').is_file(), Path(__file__).resolve().parents))
TEST_EXTPANID = "fedcba9876543210"
src/test_driver/linux-cirque/SubscriptionResumptionTest.py (45)
The constant TEST_EXTPANID was accidentally removed during this refactor. It should be restored.
CHIP_REPO = next(filter(lambda p: (p / 'SPECIFICATION_VERSION').is_file(), Path(__file__).resolve().parents))
TEST_EXTPANID = "fedcba9876543210"
src/controller/chip-root.py (1-13)
This file appears to be a debug script or a scratchpad used during development. It defines redundant variables and uses print() statements, which should be avoided in the repository to prevent unnecessary output in CI environments. Please remove this file before merging.
References
- Remove leftover debug code, particularly from docstrings, before merging a pull request.
- For messages that need to be visible in certification logs, LOGGER.info is sufficient. Using print() should be avoided to prevent unnecessary output in other environments like CI.
|
PR #72154: Size comparison from 294c51c to bcd14f6 Full report (5 builds for cc32xx, realtek, stm32)
|
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #72154 +/- ##
=======================================
Coverage 55.61% 55.61%
=======================================
Files 1630 1630
Lines 111146 111146
Branches 13397 13397
=======================================
Hits 61812 61812
Misses 49334 49334 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #72154: Size comparison from 03988db to 893a872 Full report (43 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
for more information, see https://pre-commit.ci
|
PR #72154: Size comparison from efc29a8 to daf4794 Full report (43 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces the many ad-hoc os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', ...)) style snippets used throughout the Python code in the SDK with a generic ancestor lookup that finds the repo root by walking Path(__file__).parents for an ancestor containing a SPECIFICATION_VERSION file. The intent is to make these scripts independent of their depth in the tree.
Changes:
- Replaces hard-coded
parents[N]/..-chain root computations withnext(filter(lambda p: (p / 'SPECIFICATION_VERSION').is_file(), Path(__file__).parents))across ~20 scripts/tests and the cirquehelper/paths.py. - Adds an unused
CHIP_REPO = …constant (and afrom pathlib import Pathimport) to four cirque test files, plus blank-line-only edits to several other cirque test files. - Adds a new
src/controller/chip-root.pycontaining two trial expressions andprint()calls.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/controller/chip-root.py | New file with two trial CHIP_ROOT expressions and print()s; not referenced anywhere. |
| src/test_driver/linux-cirque/helper/paths.py | CHIP_REPO_PATH switched from parents[4] to marker-file lookup. |
| src/test_driver/linux-cirque/EchoTest.py, EchoOverTcpTest.py, FailsafeTest.py, IcdDeviceTest.py | Add an unused CHIP_REPO constant and pathlib import. |
| src/test_driver/linux-cirque/{SubscriptionResumption*,SplitCommissioning,PythonCommissioning,MobileDevice,CommissioningWindow,Commissioning}Test.py | Whitespace-only insertions of a blank line. |
| scripts/tests/TestTimeSyncTrustedTimeSourceRunner.py, run_test_suite.py, run_python_test.py, matter_yaml_linter.py | Replace DEFAULT_CHIP_ROOT computation with marker-file lookup. |
| scripts/spec_xml/paths.py | get_chip_root() fallback uses marker-file lookup inside the existing try/except. |
| scripts/setup/nrfconnect/update_ncs.py | chip_root in get_ncs_recommended_revision() uses marker-file lookup. |
| scripts/helpers/iwyu-check.py, clean_runnable_targets.py | proj_root_dir / CHIP_ROOT use marker-file lookup; OUTPUT_ROOT derived from it. |
| scripts/codepregen.py | sdk_root default uses marker-file lookup. |
| scripts/checkout_submodules.py | CHIP_ROOT uses marker-file lookup. |
| scripts/build/build_darwin_framework.py | CHIP_ROOT in get_file_from_pigweed uses marker-file lookup. |
| examples/tv-casting-app/tests/*.py (10 files) | REPO_ROOT switched to marker-file lookup. |
| examples/chef/chef.py | _REPO_BASE_PATH switched to marker-file lookup. |
| docs/conf.py | MATTER_BASE switched to marker-file lookup. |
| credentials/development/gen_commissioner_dut_test_plan_table.py | CHIP_ROOT switched to marker-file lookup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #72154: Size comparison from efc29a8 to a9fc731 Full report (33 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32)
|
enkiusz
force-pushed
the
generic-chip_root-finder
branch
from
a9fc731 to
60f7d65
Compare
|
PR #72154: Size comparison from 92d95f2 to b7541fb Full report (43 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
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.
Summary
In Python code in this SDK there are a significant number of files which need (as a first order of business) to establish a path
to the root of the directory where the repository is located. This is always done by first going from file and then going to
parent directories to end up in the repository root. A number of examples of how this is done in various files is provided below:
CHIP_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), '../..'))MATTER_BASE = Path(__file__).resolve().parents[1]REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..", ".."))CHIP_TOPDIR = os.path.dirname(os.path.realpath(__file__))[:-len(os.path.join('src', 'setup_payload', 'tests'))]CHIP_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), '../../..'))The problems that I see with this code is that the code is dependent on the exact location of the file in the repository. I have
developed a universal solution for establishing the repository root directory that is independent of the file location. See below:
next(filter(lambda p: (p / 'SPECIFICATION_VERSION').is_file(), Path(__file__).parents))What I would like to do is to replace the miriad of ways that the root repository is with the above solution.
Caveats:
SPECIFICATION_VERSIONmight not be the best file to use as a marker for when we have reached the proper path, maybe we should create some.topdirsentinel filePW_PROJECT_ROOTandCHIP_ROOT. I have not copied this behaviour to the generic case but it might be worth to add itTesting
All CI jobs completed successfully.
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