Move clang formating away from zap regen all #41543
Conversation
![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 clang-formatting logic by moving it from zap_regen_all.py to codegen.py. This is a good change as it places the formatting logic closer to where the files are generated. The implementation is sound and consistent with the previous behavior. I have one suggestion to make the code for collecting C++ files more concise and Pythonic.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
|
||
| subprocess.check_call([getClangFormatBinary(), "-i"] + cpp_files) | ||
| except Exception: | ||
| traceback.print_exc() |
There was a problem hiding this comment.
Thank you for working on this! This will really help me out when done.
When I do codegen on top of this PR locally, I get:
Traceback (most recent call last):
File "/Users/bzbarsky/connectedhomeip/./scripts/codegen.py", line 187, in main
subprocess.check_call([getClangFormatBinary(), "-i"] + cpp_files)
File "/Users/bzbarsky/connectedhomeip/.environment/cipd/packages/pigweed/lib/python3.11/subprocess.py", line 413, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/Users/bzbarsky/connectedhomeip/.environment/cipd/packages/pigweed/bin/clang-format', '-i', ... long list of paths ... ]' returned non-zero exit status 1.
but then codegen claims to succeed even though it failed, because this code is swallowing the exception (which admittedly the old code did too).
The error messages before that look like a long list of:
RelativeHumidityMeasurement/MetadataProvider.h: No such file or directory
and such, so it looks like this is passing in relative paths that are not relative to the cwd?
There was a problem hiding this comment.
Thanks for your review!! I missed the join method to select the right path, leading to an error related to the paths
I'm working on fixing it, and when I finish it I will request a new review
|
PR #41543: Size comparison from 3e5d0a5 to 49813b3 Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
This reverts commit 8ce2f32.
|
PR #41543: Size comparison from 3e5d0a5 to b4be92f Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
3 participants
Summary
The main goal is to separate the logic to apply clang formatting, for that I move the logic to codegen.py
Related issues
ticket #38774
Testing
you can check the time and the output on the ZAP Regeneration job or run it locally with the command "./scripts/run_in_build_env.sh scripts/tools/zap_regen_all.py"
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