Skip to content

Commit e3a3aff

Browse files
authored
Add include/cppcheck linting and extend strict warnings to every build (#81)
Introduces two new whole-repo static analysis passes, applies the strict first-party warning set to every build that compiles our own code (not just host), fixes what those turned up, and hardens the release workflows. New lint tooling - `script/check-includes.sh` + `check_includes.py`: an include-what-you-use pass over `src/`, `include/`, `tests/`, `host_examples/`, and `examples/`, built on clang-include-cleaner. Files with guarded ESP branches are analyzed twice: a host pass, and an ESP pass with `-DESP_PLATFORM` against stub headers materialized from `script/esp_stubs.py`, so ESP-only code paths are checked without a cross toolchain. A .cpp may still rely on its matching .h's includes. Supports `--fix` for unused-include removal. - `script/cppcheck.sh`: whole-program cppcheck (warning, style, unusedFunction) over first-party sources. It sees every caller in a single pass, so it can flag functions that are dead beyond the public API surface. - `clang-tidy.sh` now passes `--warnings-as-errors='*'` so findings fail CI even if `.clang-tidy` ever loses its WarningsAsErrors line. - Both new checks run as CI jobs and are required by the merge gate. `cppcheck` is pinned to 2.21.0 and built from source (cached) because the distro package lags several minor versions and reports differently than the version developers run locally. Warning flags - Extracted the wrapper warning set into `opus_wrapper_warning_flags()` and applied it to the ESP-IDF component build as well as host. ESP-only code paths (`#ifdef ESP_PLATFORM` branches, Xtensa sources) previously compiled under strict warnings nowhere, since the host build never sees them. - Added `-Wmissing-prototypes`/`-Wmissing-declarations` and `-Wold-style-cast` to the set, and applied the same flags to test sources and `host_examples`. - `-Werror` stays gated behind `ENABLE_WERROR` (default off), so consumers building these source-distributed files with an arbitrary toolchain get warnings only. CI turns it on for the host sanitizer build and for both PlatformIO example builds, which act as the pinned ESP gate. Fixes for the newly enabled warnings - Replaced C-style casts with `static_cast` across `src/`, tests, and the benchmark examples. The end-trim arithmetic now stays in 64-bit rather than narrowing early: the value derives from an untrusted granule position and can exceed `SIZE_MAX` on 32-bit targets. - Wrapped `OPUS_SET_GAIN`/`OPUS_SET_BITRATE` call sites in diagnostic pragmas -- the C-style cast is inside the vendored libopus macro and can't be fixed at the call site. - Made file-local helpers in `opus_to_wav` static, added its missing `<cstdint>`, and zero-initialized `EncodeResult` so the early error-return path returns defined fields. - Annotated the public accessors whose only callers are downstream consumers, which cppcheck's unusedFunction cannot see. API - `decode()` and `conceal_loss()` on both decoders are now [[nodiscard]], with a `warn_unused_result` fallback below C++17. These report errors only through their return code, so ignoring it silently loses failures. - `OggOpusDecoder'`s constructor is now explicit. - `ogg_opus_decoder.h` uses `<cstddef>`/`<cstdint>` instead of the C headers. Release and CI workflows - release-drafter authenticates through a GitHub App token instead of `GITHUB_TOKEN` plus a deploy key; the workflow's default token is now granted no permissions at all. - `publish.yml`: added a timeout to every job and re-granted contents: read to the Espressif job, whose job-level permissions block had replaced the workflow default that checkout relies on. - Fixed the PlatformIO cache key: the nested expression inside hashFiles() was passed as a literal string, so the key never varied by example and never matched a real file. - Dependabot PRs now carry `dependencies`/`github-actions` labels, and the release-drafter version-resolver labels are `force-major`/`force-minor` to avoid colliding with ordinary triage labels.
1 parent 5c9fbb5 commit e3a3aff

29 files changed

Lines changed: 1479 additions & 66 deletions

.github/dependabot.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,7 @@ updates:
44
directory: "/"
55
schedule:
66
interval: "weekly"
7+
labels:
8+
- "dependencies"
9+
- "github-actions"
710
open-pull-requests-limit: 10

.github/release-drafter.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,11 @@ categories:
1111
version-resolver:
1212
major:
1313
labels:
14-
- "major"
14+
- "force-major"
1515
- "breaking-change"
1616
minor:
1717
labels:
18-
- "minor"
18+
- "force-minor"
1919
- "new-feature"
2020
patch:
2121
labels:

.github/workflows/ci.yml

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,70 @@ jobs:
6565
CLANG_TIDY: clang-tidy-18
6666
run: ./script/clang-tidy.sh
6767

68+
include-check:
69+
name: Include check
70+
if: github.event.action != 'labeled'
71+
runs-on: ubuntu-latest
72+
timeout-minutes: 15
73+
steps:
74+
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
75+
with:
76+
submodules: false
77+
78+
- name: Initialize library submodules
79+
run: git submodule update --init --depth 1 lib/opus lib/micro-ogg-demuxer
80+
81+
- name: Install clang-include-cleaner
82+
run: |
83+
sudo apt-get update
84+
sudo apt-get install -y clang-tools-18
85+
86+
- name: Run include check
87+
env:
88+
CLANG_INCLUDE_CLEANER: clang-include-cleaner-18
89+
run: ./script/check-includes.sh
90+
91+
cppcheck:
92+
name: Cppcheck
93+
if: github.event.action != 'labeled'
94+
runs-on: ubuntu-latest
95+
timeout-minutes: 15
96+
env:
97+
# Pin cppcheck and build it from source: ubuntu-latest's apt package lags
98+
# by several minor versions and produces different findings than the
99+
# version developers run locally (Homebrew). Bump deliberately, in step
100+
# with local installs. The build is cached, so only the first run per
101+
# version pays the compile.
102+
CPPCHECK_VERSION: "2.21.0"
103+
steps:
104+
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
105+
with:
106+
submodules: false
107+
108+
- name: Initialize library submodules
109+
run: git submodule update --init --depth 1 lib/opus lib/micro-ogg-demuxer
110+
111+
- name: Cache pinned cppcheck
112+
id: cache-cppcheck
113+
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
114+
with:
115+
path: ~/cppcheck-install
116+
key: cppcheck-${{ env.CPPCHECK_VERSION }}-${{ runner.os }}
117+
118+
- name: Build pinned cppcheck
119+
if: steps.cache-cppcheck.outputs.cache-hit != 'true'
120+
run: |
121+
git clone --depth 1 --branch "$CPPCHECK_VERSION" https://github.com/danmar/cppcheck /tmp/cppcheck
122+
cmake -S /tmp/cppcheck -B /tmp/cppcheck/build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX="$HOME/cppcheck-install"
123+
cmake --build /tmp/cppcheck/build -j"$(nproc)"
124+
cmake --install /tmp/cppcheck/build
125+
126+
- name: Add pinned cppcheck to PATH
127+
run: echo "$HOME/cppcheck-install/bin" >> "$GITHUB_PATH"
128+
129+
- name: Run cppcheck
130+
run: ./script/cppcheck.sh
131+
68132
build:
69133
name: Build
70134
if: github.event.action != 'labeled'
@@ -133,7 +197,7 @@ jobs:
133197

134198
- name: Configure CMake with sanitizers
135199
run: >
136-
cmake -B build -DENABLE_SANITIZERS=ON
200+
cmake -B build -DENABLE_SANITIZERS=ON -DENABLE_WERROR=ON
137201
-DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache
138202
-DOPUS_ALLOCATION_MODE=${{ matrix.alloc_mode }} tests
139203
@@ -339,7 +403,7 @@ jobs:
339403
path: |
340404
~/.platformio
341405
examples/${{ matrix.example }}/.pio
342-
key: pio-${{ matrix.example }}-${{ hashFiles('examples/${{ matrix.example }}/platformio.ini') }}
406+
key: pio-${{ matrix.example }}-${{ hashFiles(format('examples/{0}/platformio.ini', matrix.example)) }}
343407
restore-keys: |
344408
pio-${{ matrix.example }}-
345409
@@ -358,6 +422,8 @@ jobs:
358422
needs:
359423
- pre-commit
360424
- lint
425+
- include-check
426+
- cppcheck
361427
- build
362428
- test
363429
- conformance

.github/workflows/publish.yml

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ jobs:
1313
verify-version:
1414
name: Verify version matches tag
1515
runs-on: ubuntu-latest
16+
timeout-minutes: 5
1617
steps:
1718
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
1819
- name: Check library.json and idf_component.yml against the release tag
@@ -31,6 +32,9 @@ jobs:
3132
ci-gate:
3233
name: CI gate
3334
runs-on: ubuntu-latest
35+
timeout-minutes: 5
36+
# Reads check-run conclusions via the API; no checkout, so contents is not
37+
# needed, but checks:read is.
3438
permissions:
3539
checks: read
3640
steps:
@@ -57,8 +61,9 @@ jobs:
5761
5862
publish-platformio:
5963
name: Publish to PlatformIO
60-
needs: [verify-version, ci-gate]
6164
runs-on: ubuntu-latest
65+
timeout-minutes: 15
66+
needs: [verify-version, ci-gate]
6267
environment: platformio
6368
steps:
6469
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
@@ -79,12 +84,14 @@ jobs:
7984

8085
publish-espressif:
8186
name: Publish to Espressif
82-
needs: [verify-version, ci-gate]
8387
runs-on: ubuntu-latest
88+
timeout-minutes: 15
89+
needs: [verify-version, ci-gate]
8490
environment: espressif
8591
permissions:
86-
contents: read
8792
id-token: write
93+
# Job-level permissions replace the workflow default, so re-grant the read checkout needs.
94+
contents: read
8895
steps:
8996
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
9097
with:

.github/workflows/release-drafter.yml

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,32 @@ on:
55
branches:
66
- main
77

8-
permissions:
9-
contents: write
10-
pull-requests: read
8+
# Release automation routes every repo operation (release-drafter, checkout, and the version-bump
9+
# push) through the GitHub App token generated below, so the default GITHUB_TOKEN is unused here.
10+
# Grant it no permissions at all. GitHub always mints the token; the empty set just strips its powers.
11+
permissions: {}
1112

1213
jobs:
1314
release-drafter:
1415
name: Release Drafter
1516
environment: release-drafter
1617
runs-on: ubuntu-latest
1718
steps:
19+
- name: Generate a token
20+
id: generate-token
21+
uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0
22+
with:
23+
app-id: ${{ secrets.ESPHOME_GITHUB_APP_ID }}
24+
private-key: ${{ secrets.ESPHOME_GITHUB_APP_PRIVATE_KEY }}
25+
1826
- uses: release-drafter/release-drafter@34d80673e067bdc0c24568d3af899c216adcfaa9 # v7.7.0
1927
id: drafter
2028
env:
21-
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
29+
GITHUB_TOKEN: ${{ steps.generate-token.outputs.token }}
2230

2331
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
2432
with:
25-
ssh-key: ${{ secrets.DEPLOY_KEY }}
33+
token: ${{ steps.generate-token.outputs.token }}
2634

2735
- name: Update version files
2836
run: |

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ build/
88
.libs/
99
.deps/
1010

11+
# Local marker recording which internal conventions version is used for this project.
12+
.conventions-version
13+
1114
# ESP-IDF specific
1215
sdkconfig
1316
sdkconfig.old

CMakeLists.txt

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,26 @@ if(ESP_IDF_BUILD)
9595
# Apply ESP-IDF configuration
9696
opus_configure_esp_idf(${COMPONENT_LIB} ${COMPONENT_DIR} ${OPUS_STAGED_DIR})
9797

98+
# Strict warnings for our own wrapper sources on the ESP-IDF build too - this is the build that
99+
# ships to hardware and the only place ESP-only code paths (#ifdef ESP_PLATFORM branches, Xtensa
100+
# sources) ever compile, so local warnings are most useful here. Scoped per-source to src/ via the
101+
# same helper as the host build; the bundled upstream Opus C (registered in the same component) is
102+
# never put through them. -Werror is gated behind ENABLE_WERROR exactly as on host (default off, so
103+
# a consumer building the component against an arbitrary future IDF toolchain gets warnings only);
104+
# CI's pinned ESP build passes -DENABLE_WERROR=ON, the only place ESP-only wrapper warnings actually
105+
# become errors (the host -Werror build never compiles these branches).
106+
option(ENABLE_WERROR "Treat warnings as errors" OFF)
107+
opus_wrapper_warning_flags(MICRO_OPUS_ESP_WRAPPER_WARNINGS)
108+
if(ENABLE_WERROR)
109+
list(APPEND MICRO_OPUS_ESP_WRAPPER_WARNINGS -Werror)
110+
# These sources build at -O2, where GCC's -Wmaybe-uninitialized is false-positive-prone, so
111+
# keep it non-fatal (a definite -Wuninitialized still errors), matching the host block and
112+
# opus_set_optimization_flags. The ESP toolchain is always GCC, so no compiler guard is needed.
113+
list(APPEND MICRO_OPUS_ESP_WRAPPER_WARNINGS -Wno-error=maybe-uninitialized)
114+
endif()
115+
set_source_files_properties(${OGG_OPUS_SOURCES} PROPERTIES
116+
COMPILE_OPTIONS "${MICRO_OPUS_ESP_WRAPPER_WARNINGS}")
117+
98118
# ==============================================================================
99119
# Host Build
100120
# ==============================================================================
@@ -148,10 +168,9 @@ else()
148168
# Strict warnings for our own wrapper sources only. The bundled upstream Opus C is not clean
149169
# under this set and is never edited here, so it keeps the relaxed flags from
150170
# opus_set_optimization_flags(); scoping these per-source confines them to src/. ENABLE_WERROR
151-
# makes them fatal in CI. Mirrors the warning set used across the micro-* libraries.
152-
set(MICRO_OPUS_WRAPPER_WARNINGS
153-
-Wall -Wextra -Wpedantic -Wshadow -Wconversion -Wsign-conversion -Wdouble-promotion
154-
-Wformat=2 -Wimplicit-fallthrough)
171+
# makes them fatal in CI. Mirrors the warning set used across the micro-* libraries (shared with
172+
# the ESP-IDF build via opus_wrapper_warning_flags()).
173+
opus_wrapper_warning_flags(MICRO_OPUS_WRAPPER_WARNINGS)
155174
if(ENABLE_WERROR)
156175
list(APPEND MICRO_OPUS_WRAPPER_WARNINGS -Werror)
157176
# These sources build at -O2 (above), where GCC's -Wmaybe-uninitialized is

cmake/functions.cmake

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,37 @@ function(opus_set_common_definitions TARGET)
2727
)
2828
endfunction()
2929

30+
# ==============================================================================
31+
# opus_wrapper_warning_flags
32+
# ==============================================================================
33+
# Returns (in OUT_VAR, in the caller's scope) the strict warning set applied to
34+
# our own first-party C++ wrapper sources (src/*.cpp) on BOTH the host and the
35+
# ESP-IDF component build. The bundled upstream Opus C is never put through these
36+
# (it is not clean under them by design), so callers scope the result to the
37+
# wrapper sources via set_source_files_properties().
38+
#
39+
# -Werror is deliberately NOT included here: each caller (host and ESP) appends it
40+
# under the ENABLE_WERROR guard, which defaults off. These sources are
41+
# source-distributed and consumers compile them with arbitrary future toolchains,
42+
# so with the guard off they get warnings only; CI's pinned host and ESP builds
43+
# pass -DENABLE_WERROR=ON, which is where the warnings actually become errors.
44+
#
45+
# Arguments:
46+
# OUT_VAR - Name of the variable to populate in the caller's scope
47+
# ==============================================================================
48+
function(opus_wrapper_warning_flags OUT_VAR)
49+
set(${OUT_VAR}
50+
-Wall -Wextra -Wpedantic -Wshadow -Wconversion -Wsign-conversion -Wdouble-promotion
51+
-Wformat=2 -Wimplicit-fallthrough
52+
# Any function not declared in a header must be static, so -Wunused-function can see it go
53+
# dead. Clang and GCC spell the C++ variant differently.
54+
$<$<CXX_COMPILER_ID:Clang,AppleClang>:-Wmissing-prototypes>
55+
$<$<CXX_COMPILER_ID:GNU>:-Wmissing-declarations>
56+
# Require static_cast/reinterpret_cast over C-style casts (the wrapper sources are all C++).
57+
$<$<COMPILE_LANGUAGE:CXX>:-Wold-style-cast>
58+
PARENT_SCOPE)
59+
endfunction()
60+
3061
# ==============================================================================
3162
# opus_set_optimization_flags
3263
# ==============================================================================

examples/decode_benchmark/platformio.ini

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,11 @@
22
platform = https://github.com/pioarduino/platform-espressif32/releases/download/54.03.20/platform-espressif32.zip
33
framework = espidf
44
monitor_speed = 115200
5+
; CI builds this example as the pinned ESP-IDF gate for the micro-opus component's first-party
6+
; wrapper warnings: -DENABLE_WERROR=ON turns them into errors here, the only place the ESP-only code
7+
; paths ever compile (the host -Werror build never sees them). Remove this line if you copy the
8+
; example and build against a newer/unpinned toolchain, where a new warning could hard-fail the build.
9+
board_build.cmake_extra_args = -DENABLE_WERROR=ON
510

611
[env:esp32-s3]
712
board = esp32-s3-devkitm-1

examples/decode_benchmark/src/decode_benchmark.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ static void log_decode_result(const char* prefix, DecodeResult* result) {
241241

242242
// FreeRTOS task function for concurrent decoding
243243
static void decode_task(void* params) {
244-
TaskParams* task_params = (TaskParams*)params;
244+
TaskParams* task_params = static_cast<TaskParams*>(params);
245245
const AudioConfig* config = task_params->audio_config;
246246

247247
ESP_LOGI(TAG, "Task %d starting %s decode...", task_params->task_id, config->name);

0 commit comments

Comments
 (0)