CI: devicetree: Add compliance and formatting check #92334
Conversation
github-actions
bot
requested review from
fabiobaltieri,
kartben,
nashif and
stephanosio
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
kylebonnici
force-pushed
the
feat/add-dts-compliance-check
branch
from
fab15a8 to
985009f
Compare
kylebonnici
force-pushed
the
feat/add-dts-compliance-check
branch
4 times, most recently
from
8f97ce7 to
fd78f84
Compare
|
I have updated the library version alpha20 to have better Output formatting |
kylebonnici
force-pushed
the
feat/add-dts-compliance-check
branch
from
fd78f84 to
e0431e7
Compare
|
@kylebonnici hey, initial question on the basic design, this would check every file touched by a PR right? so if a PR changes only one line in the file and the styling issue is unrelated, this would fail and block it right? |
|
Yes it checks all of the file. It also parses any includes and check these files too. If it is a dts file it will also check for diagnostic issues. We can disable formatting for included files even if I think it brings lots of value IMO We can disable diagnostics when it is a dts file even if I think it brings lots of value IMO |
|
Ok so few practical comments:
|
|
+1 on doing the global cleanup first, and I think it's something that can be done automatically AFAICT. And +1 on making it part of check_compliance.py. Having to pull in node.js is a bit of a concern for me as well, but I think it makes sense to bite the bullet and go for it ; this might actually unlock some interesting stuff for documentation as well, by allowing to start using mermaid.js which I've wanted to do for a long time. |
|
To give a bit more context: it's common practice for commit and pull request to be limited in scope, having this change as is would often require a contributor that is, for example, adding a driver, to also refactor the dts files (potentially a lot of them) in the same PR, For node, this would be the first thing that requires it, I'd say add the check to compliance and have it skip automatically if node is not detected for now. |
|
Few answers and questions:
|
|
Follow up question:
|
|
Who is the intended user for |
|
this needs to be usable from just the command line, I can't get this tool to work at all, it specifies the --files is needed then throws some node exception when I supply that, it also throws some node exception when I even try to run it with just and |
|
Yes I am working on improving the CLI but first I want to understand what it will look at the end... I have also found some issue with formatting some specific files hopefully I can get these fixed in LSP today evening. Plan is to upload a diff of all of the formatting changes here to get feedback on weather the formatter is doing any changes the should be changed. |
Whatever the final linter configuration can detect should be fixed I suppose, then we can argue whether it's right or wrong but this sounds reasonable
No I would not make check_compliance install stuff, right now I'd expect the user to install all the python requirement, run the script and it should work, adding another framework is a different conversation, I'd skip if node is not present
Sure but right now it's not a requirement for the project so it's a bit of a hard sell, if it was python it'd be way easier since we already have the infrastructure to manage python libraries but here we are talking about taking in yet another framework.
It's a good argument, @kartben for it.
The script takes a git commit range, it should only work on files in that range, but there are some checks that work globally, so there is an expectation that the whole code base as committed is compliant.
Mainly the CI but it should be possible to run it for a contributor without major setups. |
|
for any linters or code scanners, at least the following applies:
Having a DTS linter is a great thing, this will remove the need for folks to go hunt for white space violations and trivial stuff a tool can catch, so thank you for looking into this. |
|
... and having this in current check_compliance script is preferred, but I would not say no to a linter that does the above on their own, if integration in check_compliance is not possible (we need to refactor check_compliance script and make it more modular, just looking at this thing now gives me the creeps) |
|
I will look into adding something like clang-format off clang-format on
I will prepare PR for the folders as you recommended
I already have a PR to support negative numbers in expressions
I still need to look at
Re the below am I to support this or let linter whine about it as syntax error? Re below I have found the bug on my end
|
Next I will look into |
kylebonnici
force-pushed
the
feat/add-dts-compliance-check
branch
from
8f316bb to
ddf725b
Compare
That works for me. |
|
I have added some changes to the |
|
From the Linux kernel coding style: "Tabs are 8 characters, and thus indentations are also 8 characters. There are heretic movements that try to make indentations 4 (or even 2!) characters deep, and that is akin to trying to define the value of PI to be 3."
Why not? We already have the .NET runtime in the CI Docker image for Renode -- might as well include Node.js too while we are at it. On a serious note, it would be nice if we could stick to only Python when possible; not because it is better than Node.js, but because it has been the main scripting language of choice by Zephyr. However, in this case, it would be unreasonable to ask
TBH, I think modern TypeScript is a much better choice for complex scripting than Python (or rather, Python with dynamic typing and weird OOP syntax is a truly horrible language to work with in general); not to mention, it is a much more natural choice for Zephyr developers given that most of us are already familiar with C/C++. Maybe, this could be a starting point for more TypeScript stuff in Zephyr ...
I agree that it would be good to have it be part of Also, in general, we should try to minimise the number of CI workflows we have, for maintainability reasons. |
It is, yeah so the dedicated workflow can be dropped right? I see you had the workflow dispatch logic but I think it's something we can happily leave without. |
|
Yes the my workflow allowed user to manually call the linter on files without a PR and allow us to change log level .... we can live without both IMO and user can run manually locally on files if they need this to be run without a PR/commit. I will drop that commit of my workflow... and I guess also the diff it self given it is own the other PRs right? |
|
Yeah all other linter check right now runs automatically on all changed files based on a PR so I think we should stick to it for this as well. The for the diff: linter errors ideally should be annotated by file and line, if possible, so that they show up correctly on the UI, there's other checks doing it. And yeah you could attach a diff and a description on how to have it fixed automatically, that'd be cool. |
|
@fabiobaltieri by annotated you mean something like this Or report the Text Edits e.g Start Line 5 col: 10 End: line 6 co:1 replace text = '...'? if you can point me to the other changes that will be very helpful I will extend to generate the diff as well not a problem and gut the user to download this and apply with git....locally user can also run the same command with fixAll.... I can also recommend that command ? |
GitHub has some known format for workflow outputs so that the annotations show up on the UI together with stuff like code review comments, this is documented here apparently. The check_compliance script uses the zephyr/scripts/ci/check_compliance.py Lines 1760 to 1761 in 0f8b7b7 Is there a way of getting the lint error from your script ingested in the check compliance script in a structured way so that they could produce these kind of formatted failure messages? In a similar way as how the yaml linter was integrated in the example above. Clang and Pylint do it as well.
I suppose it's possible but asking the user to download a patch file and apply locally sounds like a very crude approach, none of the other liners do anything like that, the expectations would be that either the comment is clear enough for the user to fix the issue manually (most of these) or the user can run the command locally (clang, ruff I think both support that). I think if you add a message on the error on how to have the tool fix it for you it'd be plenty good enough. |
Use dts-linter to check each touched file in PR Signed-off-by: Kyle Micallef Bonnici <kylebonnici@hotmail.com>
kylebonnici
force-pushed
the
feat/add-dts-compliance-check
branch
from
ddf725b to
038c803
Compare
github-actions
bot
requested review from
carlescufi,
jfischer-no,
keith-zephyr and
simhein
|
|
@fabiobaltieri I have added annotation (still to update this PR) but I have found some strange case where not all annotation show... have you ever had such an issue? look at the bellow two runs ... |
|
Look like there are limits on how many annotation are shown https://github.com/orgs/community/discussions/26680 For this reason I am considering notation the whole diff to the file .... I would assume all users can read diffs See example here https://github.com/kylebonnici/zephyr/actions/runs/16274081237 |
Add guidelines for how to split long property values across multiple lines. This documents the style that is enforced by the devicetree linter. Link: zephyrproject-rtos#92334 Signed-off-by: Keith Short <keithshort@google.com>
Add guidelines for how to split long property values across multiple lines. This documents the style that is enforced by the devicetree linter. Link: zephyrproject-rtos#92334 Signed-off-by: Keith Short <keithshort@google.com>
Add guidelines for how to split long property values across multiple lines. This documents the style that is enforced by the devicetree linter. Link: #92334 Signed-off-by: Keith Short <keithshort@google.com>
Add guidelines for how to split long property values across multiple lines. This documents the style that is enforced by the devicetree linter. (cherry picked from commit b634ede) Original-Link: zephyrproject-rtos#92334 Original-Signed-off-by: Keith Short <keithshort@google.com> GitOrigin-RevId: b634ede Cr-Build-Id: 8708896397616562065 Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8708896397616562065 Copybot-Job-Name: zephyr-main-copybot-downstream Change-Id: Ia04c700c669571d814a8f8d0f8fbca239f832116 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/6772896 Bot-Commit: ChromeOS Copybot <copybot@chops-service-accounts.iam.gserviceaccount.com> Commit-Queue: ChromeOS Copybot <copybot@chops-service-accounts.iam.gserviceaccount.com> Tested-by: ChromeOS Copybot <copybot@chops-service-accounts.iam.gserviceaccount.com>
One can see an example run here
https://github.com/kylebonnici/zephyr/actions/runs/15936563443/job/44957546032