[ci] Add GHA workflows to review PRs using Verible #1427

wsipak · 2021-08-26T12:37:10Z

This PR adds CI scripts that use Verible Lint Action
to automatically review the changes in .v and .sv files.

You can see an example of how this works here:
antmicro/gha-playground#20

In order to post comments, the action needs a proper token.
Due to restrictions described in the docs It's not possible to create comments
in the context of pull_request from an external repository.
To tackle this problem we create two workflows:
The first workflow triggered by pull_request stores information
about the PR in artifact and triggers the main workflow.
The main workflow runs the Action using a token that allows creating review comments in any PR.

We can also add a configuration file for Verible using the config_file input.
Currently the default rule set is used.
These topics are also described in the Readme of the Action.

imphil

That's great, thanks for your work on this! Can you add a couple lines of comments to explain what the two files are doing? Effectively copy-pasting from your PR description here should do the trick.

On configuration: We currently run verible-lint through FuseSoC, which also provides our waiver file(s) and configuration. The configuration is at https://github.com/lowRISC/ibex/blob/master/vendor/lowrisc_ip/lint/tools/veriblelint/rules.vbl. Additionally, there are multiple waivers. As starting point, using the rules would probably be sufficient.

On testing: Is there a way to test this whole setup before enabling it for everybody? E.g. can we push a "known bad" RTL commit to this PR and see how it all works out?

.github/workflows/pr_lint_review.yml

tgorochowik · 2021-09-01T11:00:42Z

Thanks for the review!

We will work on adding the things you asked for.

As for testing: I think it would be easiest to do this in a fork - we could push everything that's needed to the main branch and then create PRs for making sure everything works as needed. Would that be ok?

rswarbrick · 2021-09-01T11:03:59Z

As for testing: I think it would be easiest to do this in a fork - we could push everything that's needed to the main branch and then create PRs for making sure everything works as needed. Would that be ok?

I think @imphil is suggesting you push an extra commit to this PR containing a "bug" that the CI is supposed to catch and flag. Assuming that works, you can force-push to get rid of the extra commit before merging. I think this is probably simpler than what you're suggesting.

tgorochowik · 2021-09-01T11:10:47Z

Unfortunately for this to work correctly, parts of the code has to land in the main branch first.

As you can see in the config there are two pipelines added, one that does all the checking (pr_trigger.yml ) and one that posts review comments (pr_lint_review.yml ).

The pr_trigger uploads a json file to artifacts which is then parsed and converted into proper comments.

The reason for this, and the reason for the requirement of the "comment poster" to be merged first is that it needs a GITHUB_TOKEN to have the permissions to post comments. This token is not available in pull requests.

Testing only the first step is possible with the approach you're suggesting, but we will only see the output in the logs (no comments will be added).

Setting it up in a fork might be less convenient but will allow us to test the whole flow without pushing anything to your repository (we did test everything of course in the gha-playground repo mentioned above, but we could replicate it in a fork)

By the way - I think that github actions are disabled in this repository (?) - I think that at least the first step should already be starting here

imphil · 2021-09-01T13:17:33Z

GitHub Actions are enabled for this repository, but I've seen it in the past that they don't run for the first time if there's a PR from a fork.

I'm OK with just merging this PR and then take it from there, and back it out if it doesn't work. Commit history beauty isn't the most important goal in life. Let us know when you feel it's ready, and then we can simply give it a try.

wsipak · 2021-09-01T14:52:16Z

Thank you. The requested changes have been applied.
The configuration file is specified here:
https://github.com/antmicro/ibex/blob/verible_ci/.github/workflows/pr_lint_review.yml#L48

Here are the steps I've taken to test this PR:

Recreated and merged the same PR here:
[ci] Add GHA workflows to review PRs using Verible antmicro/gha-playground#32
Created a new PR adding a source file to trigger the automatic review:
test new workflows antmicro/gha-playground#33
Additionally I removed the rules file in order to forcibly get some errors.

I think it's ready!

Any activity regarding a Pull Request will trigger workflows that create automatic code review using outputs from Verible linter

imphil

Looks good, let's give this a try!

Running the verible linter and adding review comments to the pull request previously had to be done in two stages: 1. Triggered on the pull request - prepare config and waiver files as artifacts. 2. Running on the repo's HEAD - run Verible and add review comments. This was required because Actions running in the context of the pull request did not have write permissions to add comments to pull requests. This is now possible with the `pull_request_target` event, which triggers when pull requests change, but runs in the context of the repo's HEAD and has the permissions to create comments. See lowRISC/ibex#1427 and chipsalliance/verible-linter-action#31 for details. Signed-off-by: James Wainwright <[email protected]>

Running the verible linter and adding review comments to the pull request previously had to be done in two stages: 1. Triggered on the pull request - prepare config and waiver files as artifacts. 2. Running on the repo's HEAD - run Verible and add review comments. This was required because Actions running in the context of the pull request did not have write permissions to add comments to pull requests. This is now possible with the `pull_request_target` event, which triggers when pull requests change, but runs in the context of the repo's HEAD and has the permissions to create comments. See lowRISC/ibex#1427 and chipsalliance/verible-linter-action#31 for details. Signed-off-by: James Wainwright <[email protected]> Signed-off-by: Dan McArdle <[email protected]>

Running the verible linter and adding review comments to the pull request previously had to be done in two stages: 1. Triggered on the pull request - prepare config and waiver files as artifacts. 2. Running on the repo's HEAD - run Verible and add review comments. This was required because Actions running in the context of the pull request did not have write permissions to add comments to pull requests. This is now possible with the `pull_request_target` event, which triggers when pull requests change, but runs in the context of the repo's HEAD and has the permissions to create comments. See lowRISC#1427 and chipsalliance/verible-linter-action#31 for details. Signed-off-by: James Wainwright <[email protected]>

Running the verible linter and adding review comments to the pull request previously had to be done in two stages: 1. Triggered on the pull request - prepare config and waiver files as artifacts. 2. Running on the repo's HEAD - run Verible and add review comments. This was required because Actions running in the context of the pull request did not have write permissions to add comments to pull requests. This is now possible with the `pull_request_target` event, which triggers when pull requests change, but runs in the context of the repo's HEAD and has the permissions to create comments. See #1427 and chipsalliance/verible-linter-action#31 for details. Signed-off-by: James Wainwright <[email protected]>

imphil reviewed Sep 1, 2021

View reviewed changes

.github/workflows/pr_lint_review.yml Show resolved Hide resolved

wsipak force-pushed the verible_ci branch from ec75c27 to 53501db Compare September 1, 2021 14:22

[ci] Add GHA workflows to review PRs using Verible

e058690

Any activity regarding a Pull Request will trigger workflows that create automatic code review using outputs from Verible linter

wsipak force-pushed the verible_ci branch from 53501db to e058690 Compare September 2, 2021 14:34

imphil approved these changes Sep 16, 2021

View reviewed changes

imphil merged commit 31c5b5e into lowRISC:master Sep 16, 2021

jwnrt mentioned this pull request Jun 14, 2023

[ci] Update EOL GitHub Actions lowRISC/opentitan#18900

Merged

jwnrt mentioned this pull request Jun 14, 2023

[ci] Consolidate Verible linting workflow into one stage jwnrt/opentitan#6

Merged

This was referenced Jun 14, 2023

[ci] Consolidate Verible linting into one workflow lowRISC/opentitan#18909

Closed

[ci] Consolidate Verible linting workflow into one stage lowRISC/opentitan#18910

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ci] Add GHA workflows to review PRs using Verible #1427

[ci] Add GHA workflows to review PRs using Verible #1427

wsipak commented Aug 26, 2021

imphil left a comment

tgorochowik commented Sep 1, 2021

rswarbrick commented Sep 1, 2021

tgorochowik commented Sep 1, 2021 •

edited

Loading

imphil commented Sep 1, 2021

wsipak commented Sep 1, 2021

imphil left a comment

[ci] Add GHA workflows to review PRs using Verible #1427

[ci] Add GHA workflows to review PRs using Verible #1427

Conversation

wsipak commented Aug 26, 2021

imphil left a comment

Choose a reason for hiding this comment

tgorochowik commented Sep 1, 2021

rswarbrick commented Sep 1, 2021

tgorochowik commented Sep 1, 2021 • edited Loading

imphil commented Sep 1, 2021

wsipak commented Sep 1, 2021

imphil left a comment

Choose a reason for hiding this comment

tgorochowik commented Sep 1, 2021 •

edited

Loading