Conversation
Use the pre-commit framework to check cmake, clang-format, spellcheck, git, and shell scripts.
Also the default install dir.
Add -C option
Spelling, formating, caps, etc.
add the commit that was just lint fixes.
Add gcovr pip install and cmake targets and configurations for running coverage and collecting and processing the results.
The lint target excludes, e.g. clang-format-fix and clang-tidy.
Make lint, lint-manual, and coverage as not producing a file. Remove targets.mk as a source for help messages in help target.
| No tests were found!!! | ||
| ``` | ||
|
|
||
| #### Pre-Commit for Linting |
There was a problem hiding this comment.
Would you like to move this section outside the root README? It's useful for Beman devs, but not for users.
I would move it into a file docs/dev-lint.md (or whatever).
| @@ -0,0 +1,32 @@ | |||
| cfgv==3.4.0 | |||
There was a problem hiding this comment.
Is it required to have these files in the repo root? If no, I would create a dev/ directory.
Run a script to install and run pre-commit in a python3 virtual environment and tweak shellcheckrc to allow the venv activation source as part of the script. shellcheck...............................................................Passed
Format errors and such,
Suggests that some refactoring is in order, but since this is also testing shellcheck itself, it's an OK exercise. Since there isn't a venv created in the CI action that runs shellcheck, it can't read the script to check for issues.
| set(CMAKE_CXX_FLAGS_ASAN | ||
| "-O3 -g -DNDEBUG -fsanitize=undefined" | ||
| CACHE STRING "C++ ASAN Flags" FORCE) | ||
|
|
There was a problem hiding this comment.
I think we should consider having CXX_FLAGS_HARDENED -- have a look at the recommendations here: https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html
Specifically I think these:
-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -D_GLIBCXX_ASSERTIONS
would be relevant. The GLIBCXX one links a version of the standard library that asserts on all preconditions -- I've found bugs in our code base using that one.
|
@steve-downey , does precommit tool can be used for also applying the linter fixes? e.g. run CC: @camio suggested that we should be able to achieve this behaviour with pre-commit, as per my understanding. |
| @@ -0,0 +1,10 @@ | |||
| #!/usr/bin/env bash | |||
| # shellcheck disable=SC1091 | |||
There was a problem hiding this comment.
Should we move it in .shellcheck rc?
There was a problem hiding this comment.
I'm undecided. I thought about it, but it would also allow a class of real errors to slip by?
| #!/usr/bin/env bash | ||
| # shellcheck disable=SC1091 | ||
|
|
||
| python3 -m venv .venv |
There was a problem hiding this comment.
Should we add small doc in docs/?
There was a problem hiding this comment.
Writing is hard. I put it off until we'd agreed this was a promising direction. I'll add a doc now.
Applying fix-ups is a dependent on the tool being applied and usually the arguments to the tool. I've started with just checks by default and no automatic fixes. There is configured a clang-format fix that will apply changes, in addition to the check of changes, and that can be run to make changes, although it's not that much simpler than a find and xargs or |
`nd`` was flagged as a possible typo, change it as it's not really significant.
neatudarius
left a comment
neatudarius
left a comment
There was a problem hiding this comment.
LGTM 👍
What's the plan with this PR? Will you merge it soon?
|
Yes, plan to merge soon.
…On Tue, Sep 3, 2024, 06:20 Darius Neațu ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM 👍
What's the plan with this PR? Will you merge it soon?
—
Reply to this email directly, view it on GitHub
<#52 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVNZ5QTW2IFVT422D5NXDDZUWEPXAVCNFSM6AAAAABMUXNL5OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENZXGAYTMNRXGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
3 participants
Add the pre-commit and configurations to run lint checks via the framework, add a GH action for the lint checks, and since most of the infrastructure was now there, add a coverage report to the system. Not hooked to CI, as I couldn't figure out how to grab the resulting report usefully yet.