-
Notifications
You must be signed in to change notification settings - Fork 8k
CI: devicetree: Add compliance and formatting check #92334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
CI: devicetree: Add compliance and formatting check #92334
Conversation
fab15a8
to
985009f
Compare
8f97ce7
to
fd78f84
Compare
I have updated the library version alpha20 to have better Output formatting |
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) |
4f8e346
to
7e6c87d
Compare
I has a look at other big repos and packages and the also move dependencies to devDependencies see: https://github.com/colinhacks/zod/blob/main/package.json |
I watched the TSC meeting and have some thoughts worth considering IMO, The best case scenario IMO is some tool that can optionally be used in a development environment and the same tool can also be used to in CI. hence all requirements are always aligned. Users who opt to have such LSP in the IDE will format as they work (similar to the point made to get users to have properly linted code before committing), and this should be optional but possible! Currently if one looks at the adoption rate for the dts-lsp more than 3.5K users are already using this in VSCode like IDEs and form npm downloads (https://npm-stat.com/charts.html?package=devicetree-language-server&from=2024-12-01&to=2025-09-20) I would argue that at least 1K more are using it in VIM once you subtract some server caching downloads. And from feedback I have most are zephyr users. The LSP does way more then just linting, IMO linting is the first step and if Zephyr opts make use of this tool we can also do more static checking, Running this tool with --diagnosticsFulll and passing the --include and --binding can I was already able find a large number of other issues that the linter format check is not doing in this PR. Below is a sample report of issue I could detect on zephyr/boards/arm Below is a sample report of issue I could detect on zephyr/boards/microchip
IMO having these tools available and accepted by the community is also a big plus for the developer experience! I hope you can see how all of this can stack nicely together by giving users tools they can use in their Editors and at the same time reused the same tools in CI. |
doc/contribute/style/index.rst
Outdated
npx dts-linter --formatFixAll | ||
If you want to format file while developing and is using a VS Code like IDEs you can install the extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to format file while developing and is using a VS Code like IDEs you can install the extension | |
Editor Integration | |
~~~~~~~~~~~~~~~~~~ |
.github/workflows/compliance.yml
Outdated
name: compliance.xml | ||
path: compliance.xml | ||
|
||
- name: upload-dts-linter-patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are normally human readable titles, guess it'd be "Upload dts linter patch" or something on that line.
What's the idea with it btw? That one can download the patch and apply it locally? (sorry I've been on and off this PR conversation, I imagine it's been discussed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the idea is that a user can get all fixes and not need to install tool locally to mitigate the concern that users need to install the tool locally.
another question, does the linter tell you what is wrong with your formatting beside generating diffs? |
#92334 (comment) TLDR, I opted to annotate the changes needed to the whole file
No the linter does generates changes (that may in some cases lead to the same text) and applies them to the current document. Then a diff is generated. Explanation: I had given a try with annotating each edit (by annotating the diff changes). But given the limitation I faced with GIT showing only a limited number of annotations (as can be seen in the comments above) and technical limitations we will run into as we enforce more rules, I opted to give just provide a diff. This is also very common e.g. black a python formatter, also only provides diff, prettier - another linter - also cannot annotate each change and this doesn't even give a diff but rather just changes the files. The reason why this becomes unmanageable is that text edits must be applied in reverse order. i.e from last line to the top. Also you cannot have overlapping text edit regions. Therefore to solve certain problems and formatting rules you have to do the formatting in layers. example I have reached this limitation already. Currently I am working on adding sorting of properties and nodes as per the spec (properties on top and nodes underneath); in most cases this will lead to overlapping text edits when changing tabs and spaces within the node and property it self. This can be solved by first sorting and applying the edits internally to get the new code and then run the second layer to fix tabs, spaces etc... Similarly to wrap lines so as to not exceed 100 characters, another layer will be needed. This is still WIP on my end that will be added later. Given all the above technical limitations, the only feasible solution I reached is to provide a diff. Having a tool which annotates each specific error would require to keep track of the position of each text component in the original text source, the more layers the harder and more costly this gets. Note that syntax issues that stop the linter from formatting are annotated as can be see here https://github.com/kylebonnici/zephyr/pull/6/files#annotation_39253140434 EDIT to give a practical example how you one go about annotating this case from a diff output? Initial PR to address sorting of properties and node (i.e. prop on top of nodes and sorting of properties as per my understanding of https://docs.kernel.org/devicetree/bindings/dts-coding-style.html) |
a7ace50
to
68b676c
Compare
96478e1
to
826cf35
Compare
Use dts-linter to check each touched file in PR Signed-off-by: Kyle Micallef Bonnici <[email protected]>
826cf35
to
a775a39
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting work! The DT docs changes seem fine to me. I'm going to steer clear of the discussion on npm :)
Some point to address from today's TSC: I know this is long but I am trying to get as much details as possible. Do user Have to install it on their machines?Short answer is no. Longs answer:
Q: But if a user does not want to install this tool, won't this CI check make it more difficult to comply as PR is blocked and user has to manually fix the issues? A: No, actually this will make things faster. This linter is respecting the rules as described in the coding guidelines so In theory all the changes this linter suggests should be done and flagged non the less. With this in mind the PR should still be blocked! The only difference is that now these infractions will be reported constantly every time. In addition the reporting will be consistent and not up to a humans to explain "add new line", "remove line", "Add space here" Also the Concerns with adding Node (yet another scripting language):
Why Node:
Why reusing tools between IDEs and CI make logical sense to me:
To date if one looks at the adoption rate for the dts-lsp more than 4.1K users are already using this in VSCode like IDEs and form npm downloads (https://npm-stat.com/charts.html?package=devicetree-language-server&from=2024-12-01&to=2025-10-08) At least 1K+ more are using it in VIM/neoVIM once you subtract server caching downloads and some CIs downloads from when testing this PRs. From the feedback I have, most are zephyr users. Also if one looks at the dts-linter around 1K+ users are using this CLI tool see https://npm-stat.com/charts.html?package=dts-linter&from=2024-12-01&to=2025-10-08. This number considers server caching downloads and some CIs when testing these PRs Concerns with dependencies:There are two types of dependencies:
There are two ways to ship a project
First important point is that any dependency needed for development are never needed at runtime and hence these dependencies will never be bundled nor imported at runtime. E.g. in c++ gcc would be a dev dependency Dependencies of dts-linterDevelopment:esbuild @types/node devicetree-language-server-types typescript vscode-languageserver-protocol-types license-checker Runtime:dts-lsp diff glob zod vscode-jsonrpc Dependencies of dts-lspDevelopment:@jest/globals @tsconfig/node20 @types/jest @types/node jestts-jest typescript vscode-languageserver-types license-checker Runtime:@hyperjump/browser @hyperjump/json-schema ajv glob vscode-languageserver vscode-languageserver-textdocument yaml Note The development dependencies must be be in the devDependencies in the package.json of the dts-linter and dts-lsp Let focus on the Runtime dependencies. Case 1: Getting runtime dependencies at runtime with no bundling
Note One can see this as the equivalent of a DLL concept where the DLL is loaded at runtime Important notes
Currently the build process of both these tools ( dts-linter and dts-lsp ) is to bundle, hence if this above options is preferred changes to the build process will be needed. Case 2: Bundled (current case)
Note One can see bundling as the equivalent of a static linking in c/c++ context LicensingWhen considering the bundled/runtime dependencies of the dts-lsp: When considering the bundled/runtime dependencies of the dts-linter: I have added to dts-linter and dts-lsp and automatically generated file with all the License of the bundled dependencies Hope this address the concerns on licensing. If not let me know so I can address. AnnotationsAs explain here #92334 (comment), it will be very difficult, if at all possible, to be able to provide meaningful annotations along side code fixes such as diffs; (read the comment to get full understanding of the difficulties). This is also a limitation in other formatters such as the python formater (black)[https://github.com/psf/black] Also from the LSP perspective, annotations will bring no benefits to the end user. However if the committee opts to use this tool in CI, then I can dedicate time on this challenge. Alternative tools
On a separate note, Zephyr has been looking at providing tools for VS Code (see #74005), The zephyr-ide already integrates with the dts-lsp used by the dts-linter |
Minor addition: I have been made aware of |
One can see an example run here
https://github.com/kylebonnici/zephyr/actions/runs/15936563443/job/44957546032