Skip to content
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

static_tests: Add test for Rust code formatting rules #20887

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Oct 2, 2024

Contribution description

It is common for projects with Rust code to just enforce the code style suggested by Rust.

This adds a check to the static tests, and a second commits changes all the Rust sources to actually follow the code style (that is automated, cargo fmt does it).

Testing procedure

This PR should fail with a sensible message on the first run, and be green when I push the second commit fixing all the formatting errors (through the provided command).

Issues/PRs references

Thanks @Teufelchen1 for spotting the formatting errors.

This is a do-over of #20886 which GitHub killed.

@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Oct 2, 2024
@chrysn
Copy link
Member Author

chrysn commented Oct 2, 2024

This is now based on #20888 to pass the other half of the static tests.

Static tests did fail as planned in job https://github.com/RIOT-OS/RIOT/actions/runs/11150914441/job/30993154634?pr=20887#step:6:87 when the old files were not reformatted.

The latest version now contains a commit whose content was generated by following the failing static check's suggestion, and should thus be ready for final review.

@chrysn chrysn marked this pull request as ready for review October 2, 2024 20:17
@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 2, 2024
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: sys Area: System Area: examples Area: Example Applications labels Oct 2, 2024
@chrysn chrysn removed Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: sys Area: System Area: examples Area: Example Applications labels Oct 2, 2024
@riot-ci
Copy link

riot-ci commented Oct 3, 2024

Murdock results

✔️ PASSED

d0299de static_tests/rust: Address shellcheck lints

Success Failures Total Runtime
10197 0 10197 16m:35s

Artifacts

@maribu
Copy link
Member

maribu commented Oct 3, 2024

The CI has some comments.

The shellcheck output IMO is always good to react on, even if this is just adding a comment explaining why a certain anti-pattern is fine in a given context and locally disabling the corresponding shellcheck test with that comment.

@chrysn
Copy link
Member Author

chrysn commented Oct 3, 2024

Good catch, how did you even find those outputs when they don't light up?

I had already commented on how the find is OK, but given that we have those checks, it's easier to just follow them than to put the appropriate overrides in place. (I think we're overlinting here -- chances are none of our systems are tested with non-GNU-compatible coreutils, and it's kind of inconsistent to accommodate old finds that don't do default path but not old shells that don't do $(), but meh, for here, I'm just fixing it, even if that means requiring bash to do it without a tempfile).

@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: sys Area: System Area: examples Area: Example Applications labels Oct 3, 2024
@maribu
Copy link
Member

maribu commented Oct 3, 2024

If you go for #!/bin/bash if you don't want to go for POSIX shell. Or we could tell the CI to assume at least the feature set of dash, which I believe is pretty much the botton end of features available as /bin/sh on typical dev machines.

For a script only run in the CI you can indeed just assume it is run on Ubuntu. I guess /bin/dash is want they use.

@chrysn
Copy link
Member Author

chrysn commented Oct 3, 2024

I did go for bash and it's still yammering about some https://www.shellcheck.net/wiki/SC2031, fixing which once more pushing readability down. Next check I'll just write in Python :-)

@chrysn chrysn added this pull request to the merge queue Oct 3, 2024
@maribu
Copy link
Member

maribu commented Oct 3, 2024

No squashing?

The pure POSIX way to do `for x in $(find ...)` right involves a
tempfile, thus limiting to bash.
@chrysn chrysn removed this pull request from the merge queue due to a manual request Oct 3, 2024
@chrysn chrysn enabled auto-merge October 3, 2024 18:43
@github-actions github-actions bot removed the Area: build system Area: Build system label Oct 3, 2024
@chrysn
Copy link
Member Author

chrysn commented Oct 3, 2024

Squashed the two lint-addressing ones, but I do consider the others sensible and important steps. In particular, having the linter fixes independent from the original (working and thus well bisectable) code means that should I have gotten things wrong there (and I'd place the error probability around 50-50 between original code and the lint fixes), whoever fixes it (eg. future me after I've forgotten about this PR) has a chance of seeing both the original intention and where things went wrong.

@chrysn chrysn added this pull request to the merge queue Oct 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 3, 2024
@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 4, 2024
@chrysn chrysn enabled auto-merge October 4, 2024 08:08
@chrysn chrysn added this pull request to the merge queue Oct 4, 2024
Merged via the queue into RIOT-OS:master with commit 13188e1 Oct 4, 2024
27 checks passed
@chrysn chrysn deleted the fmt branch October 9, 2024 08:11
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: examples Area: Example Applications Area: sys Area: System Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants