Conversation
|
Can clang-tidy also be included in the pre-commit config? |
|
@mvadari, clang-tidy requires to run conan, cmake and generate c++ code from proto files. Pre-commit checks don't require any extra files. Also clang-tidy check is relatively heavy (comparable to compilation or even slower depending on enabled checks). Running it locally on each commit will significantly slow down development process. |
There was a problem hiding this comment.
Pull request overview
This PR adds clang-tidy static analysis checks to the CI pipeline with three different triggers: on PRs (checking only changed files), on merges to develop (checking all files), and on a nightly schedule (checking all files and creating issues on failure). The PR includes a fix for argument comments in View.cpp that were flagged by the newly enabled bugprone-argument-comment check. The clang-tidy configuration is based on Clio's setup but starts with almost all checks commented out, enabling only the argument-comment check initially.
Changes:
- Added reusable GitHub Actions workflows for clang-tidy checks
- Enabled clang-tidy on PR, merge-to-develop, and scheduled runs
- Fixed argument comments in trustCreate function call to match actual parameter names
- Added
.clang-tidyconfiguration file with bugprone-argument-comment check enabled
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/reusable-clang-tidy.yml |
Orchestrates clang-tidy execution, determining which files to check based on PR context |
.github/workflows/reusable-clang-tidy-files.yml |
Executes clang-tidy on specified files, builds dependencies, and optionally creates issues on failure |
.github/workflows/on-trigger.yml |
Integrates clang-tidy check into merge/scheduled workflow |
.github/workflows/on-pr.yml |
Integrates clang-tidy check into PR workflow |
src/libxrpl/ledger/View.cpp |
Updates argument comments to match function parameter names (though still contains errors) |
.clang-tidy |
Adds clang-tidy configuration with bugprone-argument-comment check enabled |
.gitignore |
Adds .cache directory (used by clang-tidy) to ignore list |
cspell.config.yaml |
Excludes .clang-tidy from spell checking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run-clang-tidy: | ||
| needs: determine-files | ||
| if: ${{ always() && (inputs.check_all_files || needs.determine-files.outputs.any_changed == 'true') }} |
There was a problem hiding this comment.
The run-clang-tidy job declares needs: determine-files, but the determine-files job only runs when check_all_files is false (line 22). When check_all_files is true, the determine-files job is skipped, which means run-clang-tidy will also be skipped due to the dependency, even though the condition on line 49 uses always(). You should either make the needs conditional or restructure the job dependencies to avoid this issue.
| cat > issue.md <<'EOF' | ||
| ## Clang-tidy Check Failed | ||
|
|
||
| **Workflow:** ${{ github.workflow }} | ||
| **Run ID:** ${{ github.run_id }} | ||
| **Commit:** ${{ github.sha }} | ||
| **Branch/Ref:** ${{ github.ref }} | ||
| **Triggered by:** ${{ github.actor }} | ||
|
|
||
| ### Clang-tidy Output: | ||
| ``` | ||
| EOF | ||
|
|
||
| # Append clang-tidy output (filter for errors and code snippets) | ||
| if [ -f clang-tidy-output.txt ]; then | ||
| # Extract lines containing 'error:' or '|' (code snippet markers) | ||
| grep -E '(error:|\|)' clang-tidy-output.txt > filtered-output.txt || true | ||
|
|
||
| # If filtered output is empty, use original (might be a different error format) | ||
| if [ ! -s filtered-output.txt ]; then | ||
| cp clang-tidy-output.txt filtered-output.txt | ||
| fi | ||
|
|
||
| # Truncate if too large | ||
| head -c 60000 filtered-output.txt >> issue.md | ||
| if [ $(wc -c < filtered-output.txt) -gt 60000 ]; then | ||
| echo "" >> issue.md | ||
| echo "... (output truncated, see artifacts for full output)" >> issue.md | ||
| fi | ||
|
|
||
| rm filtered-output.txt | ||
| else | ||
| echo "No output file found" >> issue.md | ||
| fi | ||
|
|
||
| cat >> issue.md <<'EOF' | ||
| ``` | ||
|
|
||
| **Workflow run:** ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} | ||
|
|
||
| --- | ||
| *This issue was automatically created by the clang-tidy workflow.* | ||
| EOF |
There was a problem hiding this comment.
The heredoc delimiter uses single quotes (<<'EOF'), which prevents variable expansion in bash. This means the GitHub Actions expressions like ${{ github.workflow }}, ${{ github.run_id }}, etc. will be treated as literal text and not expanded. To fix this, either use <<EOF (without quotes) and escape any literal dollar signs, or construct the issue body differently using echo statements with proper quoting.
| /*uSrcQualityIn=*/0, | ||
| /*uSrcQualityOut=*/0, |
There was a problem hiding this comment.
The argument comment uses uSrcQualityIn and uSrcQualityOut, but the actual parameter names in the trustCreate function are uQualityIn and uQualityOut (without the "Src" prefix). The comments should match the exact parameter names to satisfy the bugprone-argument-comment check.
| /*uSrcQualityIn=*/0, | |
| /*uSrcQualityOut=*/0, | |
| /*uQualityIn=*/0, | |
| /*uQualityOut=*/0, |
| # Set the verbosity to "quiet" for Windows to avoid an excessive | ||
| # amount of logs. For other OSes, the "verbose" logs are more useful. | ||
| log_verbosity: ${{ runner.os == 'Windows' && 'quiet' || 'verbose' }} |
There was a problem hiding this comment.
The comment references Windows behavior, but this workflow only runs on Linux (as specified by the runs-on and container configuration). This appears to be a copy-paste artifact from another workflow. The comment should be removed or updated to reflect the actual behavior for this Linux-only workflow.
| # Set the verbosity to "quiet" for Windows to avoid an excessive | |
| # amount of logs. For other OSes, the "verbose" logs are more useful. | |
| log_verbosity: ${{ runner.os == 'Windows' && 'quiet' || 'verbose' }} | |
| # Use verbose logs for this Linux-only workflow. | |
| log_verbosity: verbose |
| uses: XRPLF/actions/get-nproc@cf0433aa74563aead044a1e395610c96d65a37cf | ||
| id: nproc | ||
| with: | ||
| subtract: ${{ inputs.nproc_subtract }} |
There was a problem hiding this comment.
The workflow references inputs.nproc_subtract but this input is not declared in the workflow_call inputs section. This will cause the workflow to fail when executed. You should either add nproc_subtract as an input parameter or remove the subtract parameter from the get-nproc action call.
| run-clang-tidy: | ||
| name: Run clang tidy | ||
| runs-on: ["self-hosted", "Linux", "X64", "heavy"] | ||
| container: "ghcr.io/xrplf/ci/debian-trixie:clang-21-sha-53033a2" |
There was a problem hiding this comment.
The workflow attempts to create GitHub issues using gh issue create but does not declare the necessary permissions. The workflow needs permissions: issues: write at the job level to create issues. Without this, the issue creation step will fail with a permissions error.
| container: "ghcr.io/xrplf/ci/debian-trixie:clang-21-sha-53033a2" | |
| container: "ghcr.io/xrplf/ci/debian-trixie:clang-21-sha-53033a2" | |
| permissions: | |
| issues: write |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6369 +/- ##
=========================================
- Coverage 79.9% 79.9% -0.0%
=========================================
Files 842 842
Lines 65538 65538
Branches 7253 7256 +3
=========================================
- Hits 52379 52373 -6
- Misses 13159 13165 +6
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
2 participants
High Level Overview of Change
This PR adds clang-tidy check to CI. It also includes fixes of the issues clang-tidy found with the enabled checks.
Context of Change
Automation added:
Clang-tidy configuration is taken from Clio but almost all the checks are commented for now.
I didn't test issue creation, but the rest should be fine.
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)