ci: upgrade cppcheck from 2.3 to 2.13 via apt#5943
Conversation
fabit0v
force-pushed
the
cpp-check
branch
4 times, most recently
from
8d45387 to
301252a
Compare
fabit0v
changed the title
There was a problem hiding this comment.
Seems like cppcheck will take 1 hour 43 minutes to finish running: https://github.com/aws/s2n-tls/actions/runs/28401044354/job/84152043651?pr=5943. Will this becomes faster in the future? If so, how much time or how many PRs do we need to do to get it down to a reasonable runtime.
| $CPPCHECK_EXECUTABLE --std=c99 --error-exitcode=-1 --quiet -j "$(nproc)" \ | ||
| --cppcheck-build-dir="$CPPCHECK_BUILD_DIR" \ | ||
| --max-configs=1 \ | ||
| --enable=warning,performance,portability \ |
There was a problem hiding this comment.
The original check has --enable=all and the code that you change to only has warning, performance, and portability check. Does that drop test coverage?
There was a problem hiding this comment.
It primarily drops the style suggestions which aren't actionable and informational messages like "ConfigurationNotChecked" and "unusedFunction", which don't work with -j.
| if [ $FAILED == 1 ]; | ||
| then | ||
|
|
||
| # Config 1: Linux + AWS-LC (primary customer configuration) |
There was a problem hiding this comment.
All these three configs seem very similar to each other. Can we just use --force flag and make it one command?
There was a problem hiding this comment.
--force causes the job to time out at 6 hours because it tells cppcheck to explore every possible #ifdef combination.
| sudo apt-get install -y cppcheck=2.13* | ||
|
|
||
| - name: Cache cppcheck build dir | ||
| uses: actions/cache@v4 |
There was a problem hiding this comment.
Is there a reason that we downgrade actions/cache from version 5 to 4?
There was a problem hiding this comment.
I was testing version 4 at the time. will revert to version 5.
| constParameterCallback | ||
|
|
||
| // Reason: Conditions always true/false due to compile-time constants or platform-dependent macros. | ||
| knownConditionTrueFalse |
There was a problem hiding this comment.
At line 17 of this file: https://github.com/aws/s2n-tls/pull/5943/changes#diff-bc7696cee1ea27205e3e145964ee35b2c7e79d5184b450e2dd1d5218aaee01f1R17, knownConditionTrueFalse has already been scoped to crypto/s2n_libcrypto.c. Why do we enable it globally here? Do we want this suppression to be globally available or just scoped to crypto/s2n_libcrypto.c?
| nullPointerRedundantCheck:tls/s2n_tls13_secrets.c | ||
|
|
||
| // Reason: Const correctness suggestions are low-priority style noise. Suppressed globally. | ||
| constParameterPointer |
There was a problem hiding this comment.
How many errors are these suppressing? All previous suppressions have a scope. Global suppressions might hide problems from us and general not a good idea.
There was a problem hiding this comment.
Most of these suppressions are from using cpp-check 2.19 locally which introduced new findings. I can scope it down to what the current CI runners use (version 2.13)
| checkersReport | ||
|
|
||
| // Reason: Suppressions from old cppcheck version that no longer match. | ||
| unmatchedSuppression |
There was a problem hiding this comment.
This unmatchedSuppression seems to suppress warnings if a suppression in this file doesn't match anything. I don't think we should suppress this error. We should fix the warnings that was generated by this suppression.
There was a problem hiding this comment.
Most of these findings are results from running it locally (Ubuntu 26 uses cpp-check 2.19). I will refactor to focus on the version that the CI runners use (2.13).
2 participants
Goal
Update the cppcheck CI to use Ubuntu's pre-built package with incremental caching and targeted configuration checking.
Why
The build-from-source setup (pinned to cppcheck 2.3 from 2020) requires source patches and keeps breaking. Every upgrade attempt (#3630, #3646, #5186) failed due to compilation issues on newer Ubuntu.
How
Callouts
--forceremoved. It caused 6+ hour timeouts on the CI runner. Instead it is replaced with explicit -D/-U flags which run once per customer configuration. Three runs cover Linux+AWS-LC, Linux+OpenSSL, and Windows, which represent the primary customer code paths. Running with and without --force locally produces identical findings.Testing
Passed locally with cppcheck 2.19 (all three configs, identical results with/without --force)
Draft PR passed on CI with cppcheck 2.13
Resolves #5239
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.