Rust::com enable the CI Job for rust lint check#523
Conversation
b6c1fc2 to
9d1d7a7
Compare
9d1d7a7 to
a7398da
Compare
LittleHuba
left a comment
There was a problem hiding this comment.
Hi,
I'll do some general feedback first.
Please change the trigger conditions. Avoid hiding this behind the rust-api label, since this puts load on maintainers to label PRs correctly. We only use the labeling approach for QNX since it helps with the security implications of pull-request-target triggers.
Better filter PRs for changes to specific path patterns. E.g. PRs that touch files with ending .rs. That would achieve the same but automatically.
@LittleHuba , I have updated pre-check condition now Job will run only when either rust files or workflow of lint is changed or updated on PR. |
237b0d2 to
ce4547e
Compare
LittleHuba
left a comment
There was a problem hiding this comment.
I see a lot of code duplication with the clang-tidy and the codeql workflows. We should try to reduce this.
One option is to work on #554
| #Clippy linting for Rust | ||
| build --aspects=@score_rust_policies//clippy:linters.bzl%clippy_strict | ||
| build --output_groups=+rules_lint_human | ||
| build:lint --@aspect_rules_lint//lint:fail_on_violation=true |
There was a problem hiding this comment.
Just calling the config lint invites future name clashes.
Why not call it clippy? this follows the concept we also use for clang-tidy.
Also scoping all three config lines to test:clippy makes more sense. We want to avoid worldwide flags as much as we can.
Also, this config ideally goes into our quality/static_analysis directory. Look for the clang-tidy config as an example.
There was a problem hiding this comment.
Moved this into static_analysis folder but i feel keeping build is fine for clippy as currently few of Rust target has manual bezel target tag.
And if no test target added by developer (may be initial Implementation PR kind of), then it may not give clippy warnings.
| bazel_dep(name = "rules_cc", version = "0.2.17") | ||
| bazel_dep(name = "rules_python", version = "1.8.5") | ||
| bazel_dep(name = "rules_rust", version = "0.68.1-score") | ||
| bazel_dep(name = "score_rust_policies", version = "0.0.5") |
There was a problem hiding this comment.
Is this really a non-dev-dependency?
You only need this for clippy. And clippy is only used by developers and not our users.
There was a problem hiding this comment.
It is a dev-dependency, i have updated flag.
| # For pull requests and merge groups, check for Rust file or workflow changes | ||
| if [[ "$EVENT_NAME" == "pull_request" || "$EVENT_NAME" == "merge_group" ]]; then | ||
| CHANGED_FILES=$(git diff --name-only "origin/${BASE_REF}...HEAD" 2>/dev/null || true) | ||
|
|
||
| CHANGED_RUST=$(echo "$CHANGED_FILES" | grep -E '\.rs$' || true) | ||
| CHANGED_WORKFLOW=$(echo "$CHANGED_FILES" | grep -E '^\.github/workflows/clippy_lint\.yml$' || true) | ||
|
|
||
| if [[ -n "$CHANGED_RUST" ]]; then | ||
| should_run='true' | ||
| echo "Found Rust file changes:" | ||
| echo "$CHANGED_RUST" | ||
| fi | ||
|
|
||
| if [[ -n "$CHANGED_WORKFLOW" ]]; then | ||
| should_run='true' | ||
| echo "Found workflow file changes:" | ||
| echo "$CHANGED_WORKFLOW" | ||
| fi | ||
|
|
||
| if [[ "$should_run" == "false" ]]; then | ||
| echo "No Rust or workflow files changed - skipping Clippy check" | ||
| fi | ||
| elif [[ "$EVENT_NAME" == "push" ]]; then | ||
| # For pushes to main, always run | ||
| should_run='true' | ||
| elif [[ "$EVENT_NAME" == "workflow_call" ]]; then | ||
| # For workflow_call, always run (manual or nightly) | ||
| should_run='true' | ||
| fi | ||
|
|
||
| echo "should-run=${should_run}" >> $GITHUB_OUTPUT | ||
| echo "Clippy check: should-run=${should_run}" |
There was a problem hiding this comment.
Please make it reusable and align it with the clang-format and clang-tidy workflows.
There was a problem hiding this comment.
I have created .github/actions/setup-bazel-lint/action.yml but only part we kept Bazel setup block which was common but other detail both workflow have different as per the processing.
|
|
||
| clippy: | ||
| needs: precheck | ||
| if: ${{ needs.precheck.outputs.should-run == 'true' }} |
There was a problem hiding this comment.
This will make this workflow impossible to make mandatory.
Once we make it mandatory, it must always run. So having an if condition here, would block any PR that does not touch a Rust file indefinitely blocked by the CI because the merge-queue would get a timeout.
Instead, run the action and return all good when no files were touched. If files are touched, do the actual check.
There was a problem hiding this comment.
Removed the precheck and changed as per your suggestion.
| id: run-clippy | ||
| continue-on-error: true | ||
| run: | | ||
| bazel build --config=lint //score/mw/com/... \ |
There was a problem hiding this comment.
Even though we do not have Rust targets outside //score/mw/com currently, people will likely forget to extend the scope when they add such code.
Since we use aspec_rules_lint for this, we anyway have a fast aspect-based way of finding out what targets to check. So just extend the scope to our complete relevant scope.
| bazel build --config=lint //score/mw/com/... \ | |
| bazel build --config=lint //score/... \ |
0bde4b4 to
692266c
Compare
b0b0c18 to
ce9966b
Compare
* Added workflow yml * Updated bzlrc file
* Auto enable CI Job when changes in rust file or workflow
ce9966b to
3bbef58
Compare
* Created common action for clang and clippy * Updated clippy config on static config
3bbef58 to
580388c
Compare
GitHub Actions workflow that runs Rust linter on pull requests to enforce code quality standards.
Key Features
.rsrust files or workflow of lint changedWhen It Runs
.rsrust files or workflow of lint changesFail Job Example available here - https://github.com/bharatGoswami8/communication/actions/runs/27337081555/job/80763973850?pr=1