clippy: GitHub inline annotation #405
Conversation
Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
jerensl
force-pushed
the
clippy-github-inline-annotation
branch
from
6484cc7 to
92a28c4
Compare
…and feature Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
jerensl
force-pushed
the
clippy-github-inline-annotation
branch
from
92a28c4 to
652f212
Compare
etc/ci/clippy-annotation.sh
Outdated
There was a problem hiding this comment.
Maintaining this in every repo is a chore, we should pack this script into github action for easier reuse across servo org and wider rust community.
There was a problem hiding this comment.
We can use github docker container actions or create the github-script version with javascript. Is there any repo intended only for github actions?
There was a problem hiding this comment.
We can use github docker container actions or create the github-script version with javascript
I think it would be simpler to just put it into composite action (with just one step): https://docs.github.com/en/actions/tutorials/create-actions/create-a-composite-action?platform=linux
Is there any repo intended only for github actions?
Not yet, but we should make one.
There was a problem hiding this comment.
Not yet, but we should make one.
I can create a repo if we are agreed on this plan. Do we have a name in mind? 'github-actions' or just actions?
There was a problem hiding this comment.
I've created https://github.com/servo/actions to host the shared CI actions @jerensl Could you create a PR to this repo with the code for the clippy composite action. I'm not sure if the permissions are setup correctly. Let me know on Zulip if you face issues.
Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
| GITHUB_ACTION_PATH: ${{ github.action_path }} | ||
|
|
||
| - name: clippy (with annotations) | ||
| run: cargo clippy --features "$FEATURES" --target "$PLATFORM" --message-format=json | clippy-annotation.sh |
There was a problem hiding this comment.
Looks like we currently don't fail the CI if there are clippy warnings. Should we change the current behaviour with the-- --deny warnings argument? If so, this script should also be updated so that the exit code of cargo clippy is not swallowed by the clippy-annotation.sh script.
There was a problem hiding this comment.
Since we only capture warnings and errors, how about checking the jq output directly and returning an error exit code if any are found as it’s more a simpler approach.
There was a problem hiding this comment.
I guess that depends on the complexity of the logic to check the jq output vs just returning the error code from cargo using the bash syntax. Also, failing the CI job should happen not just when clippy is run with annotations, but also for the other targets/features that currently run cargo clippy directly. For those, either we update the command directly or make them use the same action, but with annotations disabled. I think I prefer the latter so we don't have to maintain two cargo clippy invocations.
etc/ci/clippy-annotation.sh
Outdated
There was a problem hiding this comment.
Not yet, but we should make one.
I can create a repo if we are agreed on this plan. Do we have a name in mind? 'github-actions' or just actions?
Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
…ipc-channel into clippy-github-inline-annotation
Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
| - name: clippy (with annotations) | ||
| if: inputs.with-annotation == 'true' | ||
| run: | | ||
| cargo clippy --features "$FEATURES" --target "$PLATFORM" --message-format=json -- --deny warnings \ |
There was a problem hiding this comment.
I think --deny warnings unfortunately also causes clippy to exit early and doesn't output all the lint violations. If so, we can remove the the --deny warnings.
Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
Signed-off-by: Jerens Lensun <jerensslensun@gmail.com>
| #!/usr/bin/env bash | ||
|
|
||
| set -o errexit | ||
| set -o nounset | ||
| set -o pipefail | ||
|
|
||
| annotations=$(jq -r ' | ||
| select(.reason == "compiler-message") | ||
| | .message as $msg | ||
| | select($msg.level == "error" or $msg.level == "warning") | ||
| | $msg.spans[] | ||
| | select(.is_primary) | ||
| | "::\($msg.level) file=\(.file_name),line=\(.line_start),col=\(.column_start)\(if .column_end != .column_start then ",endColumn=\(.column_end)" else "" end)::\($msg.message)" | ||
| ') | ||
|
|
||
| # Print the annotations | ||
| if [[ -n "$annotations" ]]; then | ||
| echo "$annotations" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
I would move this to .github/actions/annotations/clippy-annotations.sh as there may be other annotations that we add at some point.
| - name: clippy (with annotations) | ||
| if: inputs.with-annotation == 'true' | ||
| run: | | ||
| cargo clippy --features "$FEATURES" --target "$PLATFORM" --message-format=json \ |
There was a problem hiding this comment.
How are these environment variables set?
There was a problem hiding this comment.
This is an old change. We decided to consolidate this file on servo/actions. After this pr, we will use the composite actions from there
This integrates Clippy errors directly into GitHub inline annotations.
Testing: It has been tested here.