[ci, sival] Create a linter for the testplans
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.
But I had to add
bazel: []tags tostandalone_sw_testplan.hjson, and tags andsi_stages tochip_testplan.hjson(as well as fix asi_stagewhich wasSV4) 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
But I had to add
bazel: []tags tostandalone_sw_testplan.hjson, and tags andsi_stages tochip_testplan.hjson(as well as fix asi_stagewhich wasSV4) 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_stagesto 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?
Do you still think it should be added to
quick_lintsor 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.
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.
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
hjsonand 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
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.
The CI failure is not related.