opentitan icon indicating copy to clipboard operation
opentitan copied to clipboard

[ci, sival] Create a linter for the testplans

Open AlexJones0 opened this issue 1 year ago • 6 comments

This PR is an updated version of a previous PR by @engdoreis found at https://github.com/lowRISC/opentitan/pull/20265, with merge conflicts resolved and with support for the bazel property of a testplan to be conditionally missing if the si_stage property is set to NA, as discussed here. That PR can be closed when this is merged.

A JSON schema is used with the relevant python library to lint the testplans appropriately. This can be run using the below commands to verify that all testplans pass the linting.

./util/lint_testplan.py --schema hw/lint/sival_testplan_schema.json --dir hw/top_earlgrey/data/
./util/lint_testplan.py --schema hw/lint/sival_testplan_schema.json --dir hw/top_earlgrey/data/ip

Edit: The missing si_stage properties in hw/top_earlgrey/data/standalone_sw_testplan.hjson have been set to SV4 as suggested by @engdoreis, and support for SV4 and SV5 in the json schema has now been added.

I've also now added a Github actions CI Job to run the linting check on these two directories on any PR that is made to OpenTitan.

AlexJones0 avatar Jul 09 '24 10:07 AlexJones0

But I had to add bazel: [] tags to standalone_sw_testplan.hjson, and tags and si_stages to chip_testplan.hjson (as well as fix a si_stage which was SV4) to make it pass.

Thanks for pointing this out - it seems that we were only ever running the linting on hw/top_earlgrey/data/ip and did not realise there were testplans defined outside of the ip folder, so I'll fix this up along with the other stuff

AlexJones0 avatar Jul 09 '24 10:07 AlexJones0

But I had to add bazel: [] tags to standalone_sw_testplan.hjson, and tags and si_stages to chip_testplan.hjson (as well as fix a si_stage which was SV4) to make it pass.

Thanks - the linting errors in the hw/top_earlgrey/data directory have now also been fixed.

Maybe @engdoreis will know what si_stages to set the missing values.

After discussion we decided to set these as SV4 for now, and also add json schema support for both SV4 and SV5 to support future versioning.

I think we also want this script to run in CI (probably in quick_lints) to prevent regressions

After some further discussion with @engdoreis I've added a new commit to add a github actions job that is run on PR to perform the linting. I can then make a separate PR with the ability to run through Bazel. Do you still think it should be added to quick_lints or is the Github action sufficient for now?

AlexJones0 avatar Jul 09 '24 13:07 AlexJones0

Do you still think it should be added to quick_lints or is the Github action sufficient for now?

The goal is to eventually have all our CI (lints, builds, tests) in GitHub Actions and not Azure. I think for now though it would be better to have all lints running in the same place.

If a quick lint fails, we block CI and don't move onto the more expensive FPGA or simulator tests. We can't do that blocking if some things are in GitHub Actions and some are in Azure.

jwnrt avatar Jul 09 '24 13:07 jwnrt

If a quick lint fails, we block CI and don't move onto the more expensive FPGA or simulator tests. We can't do that blocking if some things are in GitHub Actions and some are in Azure.

Understood, I'll change that to a quick lint instead.

AlexJones0 avatar Jul 09 '24 13:07 AlexJones0

The most recent force push to this PR addresses the feedback gathered so far:

  • The PR has been rebased and linting has been applied to the most recent testplan changes.
  • Testplan linting has been incorporated as an Azure CI job, and added to quick-lint.sh, instead of using Github Actions.
  • The testplan schema has been converted to a hjson and given a comment and license header.

Again, you can test this PR using the following commands:

./util/lint_testplan.py --schema hw/lint/sival_testplan_schema.hjson --dir hw/top_earlgrey/data
./util/lint_testplan.py --schema hw/lint/sival_testplan_schema.hjson --dir hw/top_earlgrey/data/ip

or just by running

./ci/scripts/testplans-lint.sh

@jwnrt @engdoreis for visibility now that I've updated this

AlexJones0 avatar Sep 06 '24 11:09 AlexJones0

I've just pushed changes to replace all mentions of lint with validate, and have changed the display name and comments of the azure job step as suggested by feedback. There should have been no changes to functionality in this last force push, CI notwithstanding.

AlexJones0 avatar Sep 06 '24 17:09 AlexJones0

The CI failure is not related.

nbdd0121 avatar Sep 30 '24 14:09 nbdd0121