uncrustify: add auto uncrustify with blacklist by jia200x · Pull Request #8519 · RIOT-OS/RIOT

jia200x · 2018-02-05T17:20:01Z

Contribution description

Here is a proposal for the discussion in #4363.

This PR adds a tool in dist/tools that applies the current uncrustify-riot.cfg config to all tracked files that don't match an entry in a blacklist.txt file.
It also includes a check.sh that could be used from CI. Checks if files in a branch have been uncrustified (close to a automatic style check).
Last but not least, provides a blacklist.txt with core and (most) vendor files blacklisted

To use this tool, simply run either uncrustify.sh for applying the configuration or check.sh to check if files have been uncrustified.

Does it make sense? Please feel free to give feedback.

Issues/PRs references

#4363

jia200x · 2018-02-05T17:22:24Z

we might need to tune again the cfg file though...

jnohlgard · 2018-02-05T17:25:01Z

Does this have any kind of safety check so that uncrustify does not overwrite local working tree changes?

kaspar030 · 2018-02-05T20:29:03Z

dist/tools/uncrustify/check.sh

+RIOTBASE=$(git rev-parse --show-toplevel)
+CURDIR=$(cd $(dirname $0) && pwd)
+BLACKLIST=$CURDIR/blacklist.txt
+FILES=$(git ls-tree -r HEAD --name-only --full-tree | grep -E '.*\.(c$|cpp$|h$|hpp$)' | grep -vf $BLACKLIST)


if possible, please re-use dist/tools/ci/changed_files.sh

ah, sure! Didn't know such file existed

dist/tools/uncrustify/uncrustify.sh

kaspar030 · 2018-02-05T20:31:34Z

we might need to tune again the cfg file though...

that will be the hard part. ;)

jia200x · 2018-02-06T09:46:32Z

Does this have any kind of safety check so that uncrustify does not overwrite local working tree changes?

Maybe I could add something that prevents the execution if there are changes (e.g output with git diff)

aabadie · 2018-02-06T09:50:25Z

dist/tools/uncrustify/check.sh

+FILES=$(git ls-tree -r HEAD --name-only --full-tree | grep -E '.*\.(c$|cpp$|h$|hpp$)' | grep -vf $BLACKLIST)
+for F in $FILES
+do
+	OUT=$(diff $RIOTBASE/$F <(uncrustify -c $CURDIR/uncrustify-riot.cfg -f $RIOTBASE/$F 2>/dev/null))


aabadie · 2018-02-06T09:51:40Z

dist/tools/uncrustify/uncrustify.sh

+FILES=$(git ls-tree -r HEAD --name-only --full-tree | grep -E '.*\.(c$|cpp$|h$|hpp$)' | grep -vf $BLACKLIST)
+for F in $FILES
+do
+	uncrustify -c $CURDIR/uncrustify-riot.cfg --no-backup $RIOTBASE/$F


Indentation should be 4 spaces (here and above)

jia200x · 2018-02-06T09:55:30Z

@aabadie solved

bergzand · 2018-02-06T09:58:37Z

@jia200x Could you please run your shell scripts through shellcheck?
It checks for simple issues such as missing quotes around variables (can cause issues with spaces in paths).

I can enumerate the issues as a review for you if you have trouble using shellcheck.

jia200x · 2018-02-06T10:01:28Z

@bergzand cool! Ran OK. I'm fixing those issues.

jia200x · 2018-02-06T10:48:19Z

@bergzand done. Thanks for the tip!

kaspar030 · 2018-02-06T11:09:37Z

dist/tools/uncrustify/uncrustify.sh

+BLACKLIST=$CURDIR/blacklist.txt
+
+. "$RIOTBASE"/dist/tools/ci/changed_files.sh
+FILES=$(changed_files | grep -vf "$BLACKLIST")


changed files allows it's own blacklist. Sth like this should be equivalent:

FILES=$(EXCLUDE="(/vendor/|/core/)" changed_files)

edit: fixed regex

I proposed to provide a separate blacklist.txt file because I think it's easier to add/remove entries from there. Since there might be several blacklisted entries, I thought it was a good idea. What do you think?

What do you think?

Ok! Then I'd suggest removing "vendor", as that should be filtered by changed_files' defaults.
Anyone reading this, next one who needs a blacklist file needs to add generic support for that to changed_files.sh! ;)

Ok, perfect! ;)

jia200x · 2018-02-06T12:01:41Z

@kaspar030 @haukepetersen anything else to blackilist besides core files?

jia200x · 2018-02-06T12:04:24Z

(If this get merged, the uncrustify process should come in a further PR)

kaspar030 · 2018-02-06T12:16:42Z

@kaspar030 @haukepetersen anything else to blackilist besides core files?

Well, as is uncrustify would change ~24k lines, with many of them unwanted changes. That IMO shows that the config file needs some tuning... I think it'll take some time util we can actually force uncrustify, even for new files. Thus whether core is blacklisted or not is pretty much irrelevant...

jia200x · 2018-02-06T12:20:11Z

Well, as is uncrustify would change ~24k lines, with many of them unwanted changes. That IMO shows that the config file needs some tuning... I think it'll take some time util we can actually force uncrustify, even for new files. Thus whether core is blacklisted or not is pretty much irrelevant...

Could you point to some unwanted changes? ;) I can do my best to address them.

EDIT: It seems there are still tabs being inserted by the cfg file, I will check

jia200x · 2018-02-06T12:27:42Z

@kaspar030 well, assuming files in upstream are in good shape, I wil try to find the config that minimizes the number of changed files

kaspar030 · 2018-02-06T12:28:44Z

This is from the first diff uncrustify.sh does on this PR's branch:

         .name = "Button 1",
-        .pin  = BTN0_PIN,
+        .pin = BTN0_PIN,
         .mode = BTN0_MODE,
         .flags = SAUL_GPIO_INVERTED,
     }

The alignment is removed here.

jia200x · 2018-02-06T12:29:54Z

The alignment is removed here.

I see.. I will play a little bit with the cfg file

tcschmidt · 2018-05-24T14:59:45Z

This tool seems a proper way to automate the burden of code style review. Can we advance to make this production ready for

sanitizing old code
adding this to the automated test system so that new PRs authors get automatic feedback prior to review?

haukepetersen · 2018-05-24T15:12:32Z

good luck :-) All the previous attempts to make this work over the last years have failed - but it would be nice to get somewhere this time!

I'd say, if applying uncrustify to our codebase yields somwhere < 1000 changes, we are on a good path...

tcschmidt · 2018-05-24T15:25:21Z

I'm sure @jia200x and others will have energy to make this work: efforts most likely will be honored by a huge burden of labor taken off reviewers shoulders.
Btw @haukepetersen : How is the status of your attempt for structuring review work? You started out with a Wiki page a while ago ?

jia200x · 2018-06-01T11:27:39Z

Check this gist to see the how the whole RIOT repo (without core files) get's uncrustified.

kaspar030 · 2018-06-01T13:04:26Z

As discussed offline, let's go with an iterative approach here.

One way would be:

add the result of "uncrustify -c /dev/null --update-config > uncrustify-force.cfg" somewhere. Best make a commit of it's own, stating the used uncrustify version
try to disable as much as possible in that config, set defaults for the rest, in order to get the minimal amount of lines when applied to the code base
those lines left, either accept the changes, or exclude from uncrustify (using uncrustify's macros)
merge this PR, with the enforcment enabled

Then, successively, add more options from uncrustify-riot.cfg. Probably best would be one per PR to make the discussions both manageable and parallelizable.

tcschmidt · 2018-06-11T08:01:21Z

@jia200x ready for the first deployment step as outlined by @kaspar030 ?

jia200x · 2020-03-10T11:39:27Z

@kaspar030 are you still interested in this PR?

kaspar030 · 2020-03-10T11:42:59Z

@kaspar030 are you still interested in this PR?

yes! I want this to enforce uncrustify once #10867 is in.

kaspar030 · 2020-03-30T19:26:19Z

#10867 is merged.
Could you rebase, move the check to dist/tools/ci/static_tests.sh and add core/*.c and core/include/*.h?

jia200x · 2020-04-02T10:27:06Z

Could you rebase, move the check to dist/tools/ci/static_tests.sh and add core/.c and core/include/.h?

Sure!

kaspar030 · 2020-04-27T14:35:13Z

ping @jia200x, I have review time tomorrow!

jia200x · 2020-04-27T15:03:20Z

done!

jia200x · 2020-04-27T15:06:38Z

Side note: In another open source project I'm working as a volunteer we are using a linter and a code beautifier. Both are for python code (black and pylint) but the experience has been quite nice.

The CI complains if the developer didn't run "black" (beautifier) but also if the linter detect issues (pylint). So I think this can live in harmony with Vera++

kaspar030 · 2020-04-27T15:08:07Z

LGTM, please squash!

jia200x · 2020-04-27T15:25:19Z

squashed!

kaspar030 · 2020-04-28T09:45:26Z

CI passed on compilation but failed on (still set) "needs squashing". re-triggering.

kaspar030 · 2020-04-28T09:57:59Z

@jia200x I found an issue: the whitelist contains paths, but should contain regexps. Also, each line should match a whole path, not a substring.
So as is the whitelist entry only matches by chance.
And when the regexp is fixed to e.g., core/.*\.c, it would also match foo/.../tests-core/foo.c.

This patch should fix the issue (adds "-x" for line based patterns to the filter, and fixes the core regexps):

https://gist.github.com/e28bea3fd46a16651ea955f5abf0b2b7

Co-authored-by: Kaspar Schleiser <[email protected]>

jia200x · 2020-04-28T10:35:54Z

fixed, and squashed directly. Thanks for the patch!

kaspar030

ACK.

jia200x · 2020-04-28T10:46:11Z

does this require Murdock? Can't we add the CI: skip compile tests label?

jia200x requested review from haukepetersen, kaspar030 and miri64 February 5, 2018 17:20

kaspar030 requested changes Feb 5, 2018

View reviewed changes

aabadie reviewed Feb 6, 2018

View reviewed changes

kaspar030 previously requested changes Feb 6, 2018

View reviewed changes

miri64 removed their request for review February 27, 2018 17:57

jia200x force-pushed the uncrustify branch from ff9822e to bdb14f2 Compare May 29, 2018 09:07

miri64 mentioned this pull request Sep 10, 2019

Uncrustify all files for release? #4363

Closed

jia200x force-pushed the uncrustify branch from b7f44bf to 209988c Compare April 27, 2020 15:02

jia200x force-pushed the uncrustify branch from 209988c to d2b841f Compare April 27, 2020 15:24

kaspar030 mentioned this pull request Apr 28, 2020

core/msg: add message bus #13947

Merged

kaspar030 removed the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Apr 28, 2020

jia200x and others added 3 commits April 28, 2020 12:35

uncrustify: add CI check

9f1b2f2

Co-authored-by: Kaspar Schleiser <[email protected]>

ci/static_tests: call uncrustify check

bf1e243

uncrustify: add core to whitelist

bb7c7a7

jia200x force-pushed the uncrustify branch from d2b841f to bb7c7a7 Compare April 28, 2020 10:35

kaspar030 approved these changes Apr 28, 2020

View reviewed changes

kaspar030 added the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Apr 28, 2020

kaspar030 merged commit e531bdb into RIOT-OS:master Apr 29, 2020

miri64 added this to the Release 2020.07 milestone Jun 24, 2020

Conversation

jia200x commented Feb 5, 2018

Contribution description

Issues/PRs references

Uh oh!

jia200x commented Feb 5, 2018

Uh oh!

jnohlgard commented Feb 5, 2018

Uh oh!

kaspar030 Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

jia200x Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaspar030 commented Feb 5, 2018

Uh oh!

jia200x commented Feb 6, 2018

Uh oh!

aabadie Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

jia200x commented Feb 6, 2018

Uh oh!

bergzand commented Feb 6, 2018

Uh oh!

jia200x commented Feb 6, 2018

Uh oh!

jia200x commented Feb 6, 2018

Uh oh!

kaspar030 Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jia200x Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

kaspar030 Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

jia200x Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

jia200x commented Feb 6, 2018

Uh oh!

jia200x commented Feb 6, 2018

Uh oh!

kaspar030 commented Feb 6, 2018

Uh oh!

jia200x commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jia200x commented Feb 6, 2018

Uh oh!

kaspar030 commented Feb 6, 2018

Uh oh!

jia200x commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcschmidt commented May 24, 2018

Uh oh!

haukepetersen commented May 24, 2018

Uh oh!

tcschmidt commented May 24, 2018

Uh oh!

jia200x commented Jun 1, 2018

Uh oh!

kaspar030 commented Jun 1, 2018

Uh oh!

tcschmidt commented Jun 11, 2018

Uh oh!

jia200x commented Mar 10, 2020

Uh oh!

kaspar030 commented Mar 10, 2020

Uh oh!

kaspar030 commented Mar 30, 2020

kaspar030 Feb 6, 2018 •

edited

Loading

jia200x commented Feb 6, 2018 •

edited

Loading

jia200x commented Feb 6, 2018 •

edited

Loading