New ot_tidy.py script for development and CI
#192
Conversation
ziuziakowska
force-pushed
the
ot-tidy
branch
from
6f186e8 to
6a440b3
Compare
There was a problem hiding this comment.
I get this error from the script:
$ scripts/opentitan/ot_tidy.py --ci-files
Could not parse 'clang-tidy' version string.Here's my output of clang-tidy --version:
$ clang-tidy --version
LLVM (http://llvm.org/):
LLVM version 16.0.2
Optimized build.I think it's because my version_line is the second line, not the first
This script is intended to replace the `ot-tidy.sh` script for running clang-tidy on our part of the codebase. A `-j` flag is added to run clang-tidy jobs in parallel, using the run-clang-tidy script that comes with it. On my machine this speeds up checking files for CI from >3 minutes to ~30 seconds. The script also checks for some configuration issues, such as the C compiler not being set to clang when configuring QEMU - this causes some warnings to be enabled that are not recognised by clang and will show up as errors. Signed-off-by: Alice Ziuziakowska <[email protected]>
ziuziakowska
force-pushed
the
ot-tidy
branch
from
6a440b3 to
898b946
Compare
Fixed it to look for that on any line. |
|
Yep, that works now, thanks |
Replaces all usages, updates docs, and removes the older script. Signed-off-by: Alice Ziuziakowska <[email protected]>
ziuziakowska
force-pushed
the
ot-tidy
branch
from
898b946 to
8cb2bfe
Compare
| # SPDX-License-Identifier: Apache2 | ||
|
|
||
| """ | ||
| 'clang-tidy' wrapper script |
There was a problem hiding this comment.
Maybe add a bit more info about the features of this wrapper (and maybe add that clang tidy is the LLVM C linter)?
| eprint(f"No 'run-clang-tidy-{CLANG_MAJOR}' or suitable 'run-clang-tidy' in PATH.") | ||
| sys.exit(1) | ||
|
|
||
| def main(): |
There was a problem hiding this comment.
Not a big deal, but can you try to make the syntax closer to existing scripts/opentitan/*.py scripts?
- try/except exception, with
-dto show the traceback - only exit from main, use exception to return error from subroutines
- use argparser.error to report usage error
Please also keep single letter argument options.
| parser.add_argument('-f', '--files', required=False, nargs="+", action='extend', default=[], | ||
| help="Files to run tidy on") | ||
|
|
||
| args = parser.parse_args() |
There was a problem hiding this comment.
It think there is a regression here (vs. .sh script): we need to be able to pass any argument to clang-tidy, not just files.
| """ | ||
| ) | ||
|
|
||
| parser.add_argument('--ci-files', required=False, action='store_true', |
There was a problem hiding this comment.
I think there is a regression vs the .sh script: how to specify the configuration file?
| using the same parameters as the KeyManager DPE. It is dedicated to unit test purposes. | ||
| * `ot-format.sh` is a simple shell wrapper to run clang-format (code formatter) on OpenTitan files | ||
| * `ot-tidy.sh` is a simple shell wrapper to run clang-tidy (C linter) on OpenTitan files | ||
| * `ot_tidy.py` is a python wrapper script to run clang-tidy (C linter) on OpenTitan files, optionally in parallel and locally or in CI. |
| eprint("'clang-tidy --version' failed.") | ||
| sys.exit(1) | ||
| version_found = False | ||
| for line in p.stdout.decode().split("\n"): |
There was a problem hiding this comment.
I missed this one: use text=True in run so you get a text stream rather than a byte stream you need to decode
| if m.group(1) != str(CLANG_MAJOR): | ||
| eprint(f"'clang-tidy' is major version {m.group(1)}, expecting {CLANG_MAJOR}.") | ||
| sys.exit(1) | ||
| version_found = True |
There was a problem hiding this comment.
You can replace version_found with a else condition on the for loop: if break is not called (i.e. match), then the else condition is executed.
| if which(f"clang-tidy-{CLANG_MAJOR}"): | ||
| return f"clang-tidy-{CLANG_MAJOR}" | ||
| # Otherwise, look for 'clang-tidy' and check its version | ||
| if which("clang-tidy"): |
There was a problem hiding this comment.
Note: which can be removed here: you can catch subprocess.run exception FileNotFoundError which indicates that it fails to resolve the path to the application and also catch CalledProcessError (with check=True) if --version fails.
Both syntax could even be combined (clang-tidy-{CLANG_MAJOR} and clang-tidy) so you could even ensure that a versioned version actually matches its real version (as clang-tidy-{CLANG_MAJOR} are often symlinks)
This replaces the existing
ot-tidy.shscript with a new Python script.This new script should make it easier to run the linter locally in the same way as CI, as the older wrapper script did not have any usage information.
The script also checks for some misconfigurations and reports those - such as QEMU not being configured to use Clang which results in a different incompatible set of warning flags to be used with GCC to be used for
clang-tidy.In addition, this script adds a
-jflag to run linting in parallel, using therun-clang-tidywrapper that comes withclang-tidy. On my machine, this reduced linting time on the same set of files as CI from ~3 minutes to 30 seconds.