CI: Add git-clang-format #595

rettichschnidi · 2021-04-23T22:18:07Z

git-clang-format ensures that all new code being submitted adheres to the format dictated by clang-format.

The code style is based on LLVM (the creator of clang-format), but with 4 instead of 2 spaces indent width and a maximum of 120 instead of 80 characters per line in order to blend in well with existing code.

Please note:

The er-coap-13 code gets exempt, as at some point, we probably want to to interact with whatever upstream there might be. For those files, just do what existing code does (which varies every few lines).
Default is clang-format-10, which is available in the Ubuntu 20.04 GitHub runner.

Suggestions on how to test this functionality locally:

Add commit with well formatted code and you should see this:

$ tools/ci/run_ci.sh --run-clang-format --clang-format clang-format-10
No code formatting errors found

Add commit with badly formatted code and you should get a diff, telling you how to correct the code:

$ tools/ci/run_ci.sh --run-clang-format --clang-format clang-format-10
diff --git a/core/liblwm2m.c b/core/liblwm2m.c
index 0dba8a8..669e562 100644
--- a/core/liblwm2m.c
+++ b/core/liblwm2m.c
@@ -352,8 +352,7 @@ int lwm2m_add_object(lwm2m_context_t * contextP,
     return COAP_NO_ERROR;
 }
 
-int lwm2m_remove_object(lwm2m_context_t * contextP, uint16_t id)
-{
+int lwm2m_remove_object(lwm2m_context_t *contextP, uint16_t id) {
     lwm2m_object_t * targetP;
 
     LOG_ARG("ID: %d", id);

To automatically correct all code changed since master:
```
$ git clang-format-10 --commit master
changed files:
  core/liblwm2m.c
```
The result can then be used to fixup the faulty commits.

Avoid bash only "[[" and "]]", access variables with curly braces and quote them.

Checkpatch is not even used in this project.

tuve · 2021-04-24T07:26:23Z

I like that we get a detailed description on how to fix any problem, but shouldn't we add to the readme.md (and/or other documentation) how to run the check locally (or actually how to contribute). It is not a very nice experience if someone makes an effort to contribute, and then get the feedback, you have erred, for something that they possible can not now beforehand.

rettichschnidi · 2021-04-26T01:24:52Z

@tuve Just adapted the PR accordingly. While doing, I felt like having to add an example commit, causing the PR to grow quite a bit. You might want to check each commit separately.

tuve · 2021-04-26T05:21:06Z

Looks fine to me

rettichschnidi · 2021-04-26T10:49:39Z

Looks fine to me

Can you please approve the PR then?

@qleisan @mlasch @sbertin-telular Any objections/feedback now, before you will be ruled by clang-format once merged?

qleisan · 2021-04-26T12:46:36Z

Go ahead! We can always tweak settings later on if something is turns out to be "wrong".

sbertin-telular · 2021-04-26T12:51:58Z

I think we need improved documentation for running the CI tests in general and this specifically. At the very least we need a list of prerequisites. Trying to run it based on the instructions in README.md doesn't work for me:

sbertin@osboxes:~/workspace/wakaama$ git clang-format-10 --diff
git: 'clang-format-10' is not a git command. See 'git --help'.
sbertin@osboxes:~/workspace/wakaama$ tools/ci/run_ci.sh --all
tools/ci/run_ci.sh: line 114: gitlint: command not found
sbertin@osboxes:~/workspace/wakaama$ tools/ci/run_ci.sh --run-clang-format --clang-format clang-format-10
sbertin@osboxes:~/workspace/wakaama$ echo $?
127

I could probably get it to work with some effort, but right now it raises a barrier to contributing.

rettichschnidi · 2021-04-26T12:57:07Z

I think we need improved documentation for running the CI tests in general

+1

... and this specifically. At the very least we need a list of prerequisites.

I'd like to go with the minimum for this PR, adding this to the readme. Everything else then as part of #586.

mlasch

@sbertin-telular I had the same issue. I was missing clang-format-10 in order to get it working.
I agree, with the growing number, the prerequisites should be listed in the readme.

rettichschnidi · 2021-04-26T18:00:47Z

At the very least we need a list of prerequisites.

I just pushed changes to fulfill this need. What do you think about it?

sbertin-telular · 2021-04-26T18:24:12Z

I think we still need instruction for adding the git alias.

sbertin@osboxes:~/workspace/wakaama$ git clang-format-10 --diff
git: 'clang-format-10' is not a git command. See 'git --help'.
sbertin@osboxes:~/workspace/wakaama$ tools/ci/run_ci.sh --run-clang-format --clang-format clang-format-10
sbertin@osboxes:~/workspace/wakaama$ echo $?
127

I do get output from tools/ci/run_ci.sh --all now.

sbertin-telular · 2021-04-26T18:57:40Z

I needed to install one more package to get it working: clang-format.

rettichschnidi · 2021-04-26T20:18:39Z

I needed to install one more package to get it working: clang-format.

I added to the apt install part of the readme file.

sbertin-telular · 2021-04-26T20:24:09Z

@rettichschnidi I saw you added clang-format-10, but clang-format is also needed. This is on Ubuntu 20.04.

sbertin-telular · 2021-04-26T20:29:31Z

clang-format depends on clang-format-10, so clang-format is probably the dependency that should be installed instead of clang-format-10.

rettichschnidi · 2021-04-26T21:10:18Z

Looking at the content of clang-format-10 I can not see anything that would be missing:

root@dfee1e0d65cf:/# dpkg -L clang-format-10 | grep /usr/bin.*clang-format-10
/usr/bin/clang-format-10
/usr/bin/git-clang-format-10

But testing it within a Ubuntu 20.04 Docker container agrees with you - the package clang-format is needed.

I will add it to the readme. I'd like to leave clang-format-10 in there as it will produce a very visible error if version 10 of clang-format is not available.

git-clang-format ensures that all *new* code being submitted adheres to the format dictated by clang-format. The code style is based on LLVM (the creator of clang-format), but with 4 instead of 2 spaces indent width and a maximum of 120 instead of 80 characters per line in order to blend in well with existing code. Please note: - The er-coap-13 code gets exempt, as at some point, we probably want to to interact with whatever upstream there might be. For those files, just do what existing code does (which varies every few lines). - Default is clang-format-10, which is available in the Ubuntu 20.04 GitHub runner.

I used the following script: for f in $(find . -type f $ -name '*.c' -or -name '*.h' $ \ -not -path '*/examples/shared/tinydtls/*' \ -not -path '*/coap/er-coap-13/*'); do end="$(grep --line-number --extended-regexp "\\*{70}/" "${f}" | awk -F: '{print $1}')" echo "Formatting ${f}, line 1 to ${end}" clang-format-10 -i --lines="1:${end}" "${f}" done

The last commit was purely cosmetic. Therefore, adding it to .git-blame-ignore-revs.

Doing so gives the developer feedback early on, before the changes hit the GitHub CI infrastructure. This would be a good candidate for using a Git pre-hook. Unfortunately, gitlint is a bit tricky to be used in a pre-push hook: jorisroovers/gitlint#55

rettichschnidi · 2021-04-26T22:06:51Z

@sbertin-telular Done

qleisan

LGTM. Tried out latest greatest PR version and followed README.md instructions successfully

sbertin-telular

LGTM

rettichschnidi added 2 commits April 23, 2021 03:02

ci: Adjust to existing code

ec6fa57

Avoid bash only "[[" and "]]", access variables with curly braces and quote them.

ci: Fix typo

6e19254

Checkpatch is not even used in this project.

rettichschnidi mentioned this pull request Apr 23, 2021

Automatic code formatting #574

Open

rettichschnidi force-pushed the gardena/rs/git-clang-format branch 3 times, most recently from cd86c52 to 100c732 Compare April 23, 2021 22:31

rettichschnidi force-pushed the gardena/rs/git-clang-format branch from 100c732 to a5aa7a7 Compare April 26, 2021 01:23

rettichschnidi requested review from tuve and mlasch April 26, 2021 11:09

tuve approved these changes Apr 26, 2021

View reviewed changes

mlasch reviewed Apr 26, 2021

View reviewed changes

mlasch approved these changes Apr 26, 2021

View reviewed changes

rettichschnidi force-pushed the gardena/rs/git-clang-format branch from a5aa7a7 to ab4ceba Compare April 26, 2021 17:56

rettichschnidi force-pushed the gardena/rs/git-clang-format branch from ab4ceba to 90c5d28 Compare April 26, 2021 18:32

rettichschnidi added 2 commits April 26, 2021 23:15

[eclipse-wakaamaGH-574] blame: Declare Eclipse header cleanup

686f297

The last commit was purely cosmetic. Therefore, adding it to .git-blame-ignore-revs.

ci: Describe running locally

132e152

Doing so gives the developer feedback early on, before the changes hit the GitHub CI infrastructure. This would be a good candidate for using a Git pre-hook. Unfortunately, gitlint is a bit tricky to be used in a pre-push hook: jorisroovers/gitlint#55

rettichschnidi force-pushed the gardena/rs/git-clang-format branch from 90c5d28 to 132e152 Compare April 26, 2021 22:05

qleisan approved these changes Apr 27, 2021

View reviewed changes

sbertin-telular approved these changes Apr 27, 2021

View reviewed changes

rettichschnidi merged commit 4a8aa2b into eclipse-wakaama:master Apr 27, 2021

rettichschnidi deleted the gardena/rs/git-clang-format branch April 27, 2021 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: Add git-clang-format #595

CI: Add git-clang-format #595

rettichschnidi commented Apr 23, 2021 •

edited

tuve commented Apr 24, 2021

rettichschnidi commented Apr 26, 2021 •

edited

tuve commented Apr 26, 2021

rettichschnidi commented Apr 26, 2021

qleisan commented Apr 26, 2021

sbertin-telular commented Apr 26, 2021 •

edited

rettichschnidi commented Apr 26, 2021

mlasch left a comment

rettichschnidi commented Apr 26, 2021

sbertin-telular commented Apr 26, 2021

sbertin-telular commented Apr 26, 2021

rettichschnidi commented Apr 26, 2021 •

edited

sbertin-telular commented Apr 26, 2021

sbertin-telular commented Apr 26, 2021

rettichschnidi commented Apr 26, 2021 •

edited

rettichschnidi commented Apr 26, 2021

qleisan left a comment

sbertin-telular left a comment

CI: Add git-clang-format #595

CI: Add git-clang-format #595

Conversation

rettichschnidi commented Apr 23, 2021 • edited

tuve commented Apr 24, 2021

rettichschnidi commented Apr 26, 2021 • edited

tuve commented Apr 26, 2021

rettichschnidi commented Apr 26, 2021

qleisan commented Apr 26, 2021

sbertin-telular commented Apr 26, 2021 • edited

rettichschnidi commented Apr 26, 2021

mlasch left a comment

Choose a reason for hiding this comment

rettichschnidi commented Apr 26, 2021

sbertin-telular commented Apr 26, 2021

sbertin-telular commented Apr 26, 2021

rettichschnidi commented Apr 26, 2021 • edited

sbertin-telular commented Apr 26, 2021

sbertin-telular commented Apr 26, 2021

rettichschnidi commented Apr 26, 2021 • edited

rettichschnidi commented Apr 26, 2021

qleisan left a comment

Choose a reason for hiding this comment

sbertin-telular left a comment

Choose a reason for hiding this comment

rettichschnidi commented Apr 23, 2021 •

edited

rettichschnidi commented Apr 26, 2021 •

edited

sbertin-telular commented Apr 26, 2021 •

edited

rettichschnidi commented Apr 26, 2021 •

edited

rettichschnidi commented Apr 26, 2021 •

edited