-
Notifications
You must be signed in to change notification settings - Fork 21
Draft - Add clang-tidy support in GHA #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5e5a41b
5e2268f
a584b27
918a2c0
e65e8ab
99081ab
36e9110
85caa49
cce7d8b
cccbc74
72c2fe4
8aa951d
65d923e
45b9e5b
357b85f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| Checks: > | ||
| -*, | ||
| bugprone-*, | ||
| performance-*, | ||
| modernize-*, | ||
| -readability-braces-around-statements, | ||
| readability-misleading-indentation, | ||
| readability-redundant-smartptr-get, | ||
| -modernize-use-trailing-return-type, | ||
| -modernize-avoid-c-arrays, | ||
| -modernize-use-nodiscard, | ||
| -bugprone-easily-swappable-parameters | ||
|
|
||
| # These warnings have determined to be critical and are as such treated as errors | ||
| WarningsAsErrors: > | ||
| clang-analyzer-*, | ||
| bugprone-use-after-move, | ||
| bugprone-dangling-handle, | ||
| bugprone-infinite-loop, | ||
| bugprone-narrowing-conversions, | ||
| bugprone-undefined-memory-manipulation, | ||
| bugprone-move-forwarding-reference, | ||
| bugprone-incorrect-roundings, | ||
| bugprone-sizeof-expression, | ||
| bugprone-string-literal-with-embedded-nul, | ||
| bugprone-suspicious-memset-usage, | ||
|
|
||
| HeaderFilterRegex: '(include/livekit|src|bridge/(include|src)|examples)' | ||
|
|
||
| FormatStyle: file | ||
|
|
||
| CheckOptions: | ||
| - key: modernize-use-nullptr.NullMacros | ||
| value: 'NULL' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,8 @@ on: | |
|
|
||
| permissions: | ||
| contents: read | ||
| actions: read | ||
| packages: read | ||
|
|
||
| env: | ||
| CARGO_TERM_COLOR: always | ||
|
|
@@ -53,22 +55,22 @@ jobs: | |
| name: linux-x64 | ||
| build_cmd: ./build.sh release-examples | ||
| build_dir: build-release | ||
| - os: ubuntu-24.04-arm | ||
| name: linux-arm64 | ||
| build_cmd: ./build.sh release-examples | ||
| build_dir: build-release | ||
| - os: macos-latest | ||
| name: macos-arm64 | ||
| build_cmd: ./build.sh release-examples | ||
| build_dir: build-release | ||
| - os: macos-latest | ||
| name: macos-x64 | ||
| build_cmd: ./build.sh release-examples --macos-arch x86_64 | ||
| build_dir: build-release | ||
| - os: windows-latest | ||
| name: windows-x64 | ||
| build_cmd: .\build.cmd release-examples | ||
| build_dir: build-release | ||
| # - os: ubuntu-24.04-arm | ||
| # name: linux-arm64 | ||
| # build_cmd: ./build.sh release-examples | ||
| # build_dir: build-release | ||
| # - os: macos-latest | ||
| # name: macos-arm64 | ||
| # build_cmd: ./build.sh release-examples | ||
| # build_dir: build-release | ||
| # - os: macos-latest | ||
| # name: macos-x64 | ||
| # build_cmd: ./build.sh release-examples --macos-arch x86_64 | ||
| # build_dir: build-release | ||
| # - os: windows-latest | ||
| # name: windows-x64 | ||
| # build_cmd: .\build.cmd release-examples | ||
| # build_dir: build-release | ||
|
|
||
| name: Build (${{ matrix.name }}) | ||
| runs-on: ${{ matrix.os }} | ||
|
|
@@ -244,6 +246,16 @@ jobs: | |
| ${{ matrix.build_dir }}/bin/ | ||
| retention-days: 7 | ||
|
|
||
| - name: Upload compile database (for clang-tidy) | ||
| if: matrix.name == 'linux-x64' | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: compile-database | ||
| path: | | ||
| ${{ matrix.build_dir }}/compile_commands.json | ||
| ${{ matrix.build_dir }}/generated/ | ||
| retention-days: 1 | ||
|
|
||
| # ---------- Cleanup ---------- | ||
| - name: Clean after build (best-effort) | ||
| if: always() | ||
|
|
@@ -382,3 +394,74 @@ jobs: | |
| cmake -S . -B build -DLIVEKIT_LOCAL_SDK_DIR=/opt/livekit-sdk | ||
| cmake --build build --parallel | ||
| ' | ||
|
|
||
| clang-tidy: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could i buy you a beer to put this clang tidy work into cpp-example-collection as well? 🙏🏼 🍻 |
||
| name: clang-tidy | ||
| needs: build | ||
| runs-on: ubuntu-latest | ||
| continue-on-error: false | ||
| if: ${{ !cancelled() }} | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
|
|
||
| steps: | ||
| - name: Checkout (with submodules) | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| submodules: recursive | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| set -eux | ||
| sudo apt-get update | ||
| sudo apt-get install -y \ | ||
| clang-tidy \ | ||
| llvm-dev libclang-dev clang \ | ||
| libssl-dev \ | ||
| libprotobuf-dev protobuf-compiler \ | ||
| libabsl-dev \ | ||
| libspdlog-dev \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we already have a lot of these deps in other stages -- should we just add clang-tidy to a stage(s) that already has deps of the lib? |
||
| libva-dev libdrm-dev libgbm-dev libx11-dev libgl1-mesa-dev \ | ||
| libxext-dev libxcomposite-dev libxdamage-dev libxfixes-dev \ | ||
| libxrandr-dev libxi-dev libxkbcommon-dev \ | ||
| libasound2-dev libpulse-dev \ | ||
| libwayland-dev libdecor-0-dev | ||
|
|
||
| - name: Download compile database | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: compile-database | ||
| path: build-release/ | ||
|
|
||
| - name: Run clang-tidy | ||
| uses: cpp-linter/cpp-linter-action@v2 | ||
| id: linter | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| with: | ||
| style: '' | ||
| tidy-checks: '' | ||
| database: build-release | ||
| files-changed-only: false | ||
| lines-changed-only: false | ||
| ignore: 'build-*|client-sdk-rust|vcpkg_installed' | ||
| file-annotations: true | ||
| thread-comments: update | ||
| step-summary: true | ||
| tidy-review: true | ||
| passive-reviews: true | ||
| no-lgtm: true | ||
|
|
||
| - name: Check warning thresholds | ||
| env: | ||
| TIDY_FINDINGS: ${{ steps.linter.outputs.clang-tidy-checks-failed }} | ||
| MAX_TIDY_FINDINGS: '396' | ||
| run: | | ||
| echo "clang-tidy findings: ${TIDY_FINDINGS}" | ||
| if [ "${TIDY_FINDINGS}" -gt "${MAX_TIDY_FINDINGS}" ]; then | ||
| echo "::warning::clang-tidy found ${TIDY_FINDINGS} issue(s), threshold is ${MAX_TIDY_FINDINGS}" | ||
| exit 1 | ||
| fi | ||
| echo "clang-tidy findings within threshold" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,11 @@ BridgeAudioTrack::BridgeAudioTrack( | |
| : name_(std::move(name)), sample_rate_(sample_rate), | ||
| num_channels_(num_channels), source_(std::move(source)), | ||
| track_(std::move(track)), publication_(std::move(publication)), | ||
| participant_(participant) {} | ||
| participant_(participant) { | ||
| std::cout << "Bad name use: " << name << "\n"; // Should invoke clang-tidy warning | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clang-tidy diagnosticbridge/src/bridge_audio_track.cpp:42:42: error: [bugprone-use-after-move]
42 | std::cout << "Bad name use: " << name << "\n"; // Should invoke clang-tidy warning
| ^
/home/runner/work/client-sdk-cpp/client-sdk-cpp/bridge/src/bridge_audio_track.cpp:38:7: note: move occurred here
38 | : name_(std::move(name)), sample_rate_(sample_rate),
| ^There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clang-tidy diagnosticbridge/src/bridge_audio_track.cpp:42:42: error: [bugprone-use-after-move]
42 | std::cout << "Bad name use: " << name << "\n"; // Should invoke clang-tidy warning
| ^
/home/runner/work/client-sdk-cpp/client-sdk-cpp/bridge/src/bridge_audio_track.cpp:38:7: note: move occurred here
38 | : name_(std::move(name)), sample_rate_(sample_rate),
| ^
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was induced just to show how this works, not an actual issue on this branch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clang-tidy diagnosticbridge/src/bridge_audio_track.cpp:42:42: error: [bugprone-use-after-move]
42 | std::cout << "Bad name use: " << name << "\n"; // Should invoke clang-tidy warning
| ^
/home/runner/work/client-sdk-cpp/client-sdk-cpp/bridge/src/bridge_audio_track.cpp:38:7: note: move occurred here
38 | : name_(std::move(name)), sample_rate_(sample_rate),
| ^ |
||
|
|
||
| while(true); // should invoke another warning | ||
| } | ||
|
|
||
| BridgeAudioTrack::~BridgeAudioTrack() { release(); } | ||
|
|
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now way we only had two files NOT clang tidy'd 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a note to remove