Investigate differences in clang format in CI and Windows

[ann0see]

Describe the bug
@pljones and @pgScorpio experienced unexpected clang format failures: see #2550 and #2551
To Reproduce
?

Expected behavior
Clang format on Windows should format the same as on the CI. I expect some unknown incompatibility.

Screenshots

Operating system
Windows and Linux

Version of Jamulus
N/A
Additional context
@hoffie

[pljones]

I'm running it on Linux, not Windows.

$ clang-format --version
clang-format version 10.0.0-4ubuntu1

I don't remember if we pinned CI to an earlier or later version...

[ann0see]

Ok. Thought you were developing on Windows too.

jamulus/.github/workflows/coding-style-check.yml

Line 20 in 5609296

clangFormatVersion: 10

It’s clang format 10 probably? So something else must be wrong.

[pljones]

I just tried running the clang-format check workflow on my repo:
https://github.com/pljones/jamulus/runs/5702476683?check_suite_focus=true
run against
pljones@b995489

So it's weird...

[pgScorpio]

I'm using clang-format Version 13.0.1 on Windows and macOS and Version 10.0.0-4ubuntu1 on Linux.
Both give the same result locally, but often auto-build does not have the same result.

1. auto-build clang-format breaks long lines too soon (config says ColomnLimit 150, but auto-build clang-format breaks sooner (just try a line of 149 chars long.).
2. auto-build clang-format does sometimes not align braces with if correctly. (See: here)
3. auto-build clang-format sometimes changes type* variable or type *variable to type * variable, but not consistently. (See here)

[ann0see]

Thanks for your comment. If I find time for it I’ll check what my clang-format does (but it should also be clang-format 10)

@passing knows clang format well, so he might know what goes wrong.

Else it might be worth opening an issue on the clang format action repo

[dcorson-ticino-com]

I had the same problem on my Qt6 work.
clang-format all OK locally on Win10, had errors when run on github.

[hoffie]

I didn't have time to investigate all hints yet, but I mainly suspect discrepancies in clang-format versions to be the cause.

I've checked https://github.com/jamulussoftware/jamulus/runs/5704004785?check_suite_focus=true (e36d3d6):

• This fails in CI
• When using the same clang-format 10 binary as in CI, this fails locally for me as well.
• clang-format 13.0.1 feels that it is clean and should not be changed.

I don't have a good idea what to do about this though. We've decided against using a newer clang-format version in CI due to it not being widely available on Linux distribution packages. Maybe we should revisit this decision.

What still needs investigation the case where Ubuntu's clang 10 disagrees as well.

[pljones]

What is the output of clang-format --version for the CI clang-format?

[pgScorpio]

@hoffie

What still needs investigation the case where Ubuntu's clang 10 disagrees as well.

Indeed. As said I use Version 13.0.1 on Windows and macOS and Version 10.0.0-4ubuntu1 on Linux.

And locally they all agree.
I cross-checked all of them by running make clang_format on the exact same repo on a USB drive on all three OS'es.
Both macOS and Linux give no change at all on the Windows clang-formated repo, so they all give the same result.
I even wondered if clang-format was actually working, but after manually changing some files they where the same again after clang-formating, so clang-format does work ;=))
But still some files from that same repo failed in the auto-build...
So there seems to be playing something else than just clang-format binary versions.
Perhaps somehow using a different clang-format configuration for auto-build ?
But still then I really wonder how, since only the long line breaking seems consistent, the other abnormalities I can't explain either.

[hoffie]

@pljones:

What is the output of clang-format --version for the CI clang-format?

clang-format version 10.0.0 (https://github.com/muttleyxd/clang-format-static-binaries 5b56bb49b977b80cd18f44df4444db93e2d30f46)
Ubuntu 20.04's clang-format package (clang-format version 10.0.0-4ubuntu1) is consistent with CI's clang-format for me.

@pgScorpio:

I'm using clang-format Version 13.0.1 on Windows and macOS and Version 10.0.0-4ubuntu1 on Linux. Both give the same result locally, but often auto-build does not have the same result.

Getting the same output from 13.0.1 on both Windows and macOS sounds reasonable to me. Getting the same output with 10.0.0-4ubuntu1 in the mentioned cases sounds surprising to me based on my tests. Can you confirm that the binary which is invoked is indeed extactly the version you've mentioned? Maybe some editors include their own (more-recent) clang-format?

1. auto-build clang-format breaks long lines too soon (config says ColomnLimit 150, but auto-build clang-format breaks sooner (just try a line of 149 chars long.).

I just tried it as part of a variable assignment/function call and could not reproduce anything unexpected with 149 columns, either with clang-format 10 or 13. Maybe it requires specific circumstances to trigger. As far as I understand, your local formatting (= clang-format 13) provides the better/correct result, right?

2. auto-build clang-format does sometimes not align braces with if correctly. (See: here)

Yep. v13 output seems more logical.

3. auto-build clang-format sometimes changes type* variable or type *variable to type * variable, but not consistently. (See here)

llvm/llvm-project#27727 mentions formatting problems for pointer receivers when using AlignConsecutiveDeclarations (which we do). It's fixed by clang-format 13.

@pljones:

I just tried running the clang-format check workflow on my repo: https://github.com/pljones/jamulus/runs/5702476683?check_suite_focus=true run against pljones@b995489

The #1432 PR check ran on 69a2411 and failed.
Your manual check ran on b995489.
There are differences between these two trees. I'm not saying that they should be relevant, but that at least explains why "clang behavior differs between repos" (it doesn't -- it differs when the files differ). The behavior is consistent with both clang-format 10 and 13. Adding an empty line before // clang-format on works around this.

All in all I can confirm that all the mentioned strangeness is reproducible with clang-format 10 (both with the binary from CI, but not with clang-format 13).
I cannot confirm any discrepancies regarding different output with the same clang-format base version or with the same version in different contexts/repos.

There are huge numbers of Issues for clang-format.

I see two ways foward:

• Disable clang-format for the problematic places. I don't like that as we have to remember reverting that at some point.
• Update CI's clang-format to v13 (this should be trivial). The only downside would be that people on Linux distributions with older versions (e.g. Ubuntu) may have to get clang-format from somewhere else.

In any case, we should specify which clang-format version we require (and mayb where to get it).

[pljones]

Ubuntu 20.04's clang-format package (clang-format version 10.0.0-4ubuntu1) is consistent with CI's clang-format for me.

Have you tried it on my code? It's consistent except sometimes it isn't. One specific place it differs.

[pljones]

Ubuntu 20.04's clang-format package (clang-format version 10.0.0-4ubuntu1) is consistent with CI's clang-format for me.

Have you tried it on my code? It's consistent except sometimes it isn't. One specific place it differs.

Suddenly it's consistent. Nothing in that area of the code changed. But suddenly my local make clang_format complained, too. Just weird, which was my first reaction.

[pgScorpio]

@hoffie

@pgScorpio:

I'm using clang-format Version 13.0.1 on Windows and macOS and Version 10.0.0-4ubuntu1 on Linux. Both give the same result locally, but often auto-build does not have the same result.

Getting the same output from 13.0.1 on both Windows and macOS sounds reasonable to me. Getting the same output with 10.0.0-4ubuntu1 in the mentioned cases sounds surprising to me based on my tests. Can you confirm that the binary which is invoked is indeed extactly the version you've mentioned? Maybe some editors include their own (more-recent) clang-format?

Strange indeed!

On all tree systems I just used:
clang-format --version to see which clang-format version was used.
and then I did

qmake Jamulus.pro
make clang_format

To do the clang-format, but no files seemed to change when clang-formatting the same folder on different OS'es.

Just as strange is that if I do the same on a fresh clone of the jamulus master repo on Windows also nothing seems to change, though the files are validated with version 10.0.0 and I'm now checking with version 13.0.1 ???
( So I did change some files manually to confirm that clang-format was indeed working, and they indeed reverted back after the clang-format.)

As I said before, there seems to be playing more than just the clang-format version....

(Except for the pointer problem, in my sound-redesign repo, that one does give different results on 10.0.0 and 13.0.1, but also strange that this happens only at some positions in the file and other files with similar coding format also do not have the same problem.)

[dcorson-ticino-com]

Are there other tools that do something similar to clang-format that might work more reliably?
This doesn't look like there is a solution coming soon.

[pgScorpio]

@dcorson-ticino-com

Are there other tools that do something similar to clang-format that might work more reliably?
This doesn't look like there is a solution coming soon.

Just a quick google says there are:

• Uncrustify
• Artistic Style
• indent-js
• Code Beautifier
• NArrange

And probably there are more, but I doubt they would be any better...

[hoffie]

As I said before, there seems to be playing more than just the clang-format version....

Can you provide some kind of reproducer? I can then have a look at this again. Until now, I haven't encountered any non-determinism (and I'd still be surprised to find any, but I'm happy to be proven wrong ;)).

Are there other tools that do something similar to clang-format that might work more reliably?

I think clang-format is the solution to this. I don't see a reliability issue ither. The issues we are seeing are related to different behavior of different clang-format versions. Other tools are likely to suffer the same problem unless everyone uses the same version with the same features and the same bugs.

[pgScorpio]

Can you provide some kind of reproducer?

Well this seems to be hard but this one seems to give different results.
For me it gives the same result on 10.0.0 (linux) and on 13.0.0 (Windows and mac) but CI fails. (The alignment of braces with 'if' differs, and on CI it is not according rules.).
Also I encountered CI breaking long lines to soon (not according rules), which didn't happen on any version locally.
Until now the only difference between 10.0.0 and 13.0.1 I could reproduce locally is the "pointer" problem.

P.S.: Also see pljones's comment

Suddenly it's consistent. Nothing in that area of the code changed. But suddenly my local make clang_format complained, too. Just weird, which was my first reaction.

Some things seem to be inconsistant independant of the clang-format version...

P.S.2: Did CI already run a test on this commit ? It might "bypass" the problem and I don't see a failed check yet.

[hoffie]

Can you provide some kind of reproducer?

Well this seems to be hard but this one seems to give different results. For me it gives the same result on 10.0.0 (linux) and on 13.0.0 (Windows and mac) but CI fails. (The alignment of braces with 'if' differs, and on CI it is not according rules.).

According to the action, the commit was e36d3d6. I think I had already checked this in detail, but I've just done it again:

• clang-format 13.0.1 (Arch) does not show any changes
• clang-format 10.0.0 from the GIthub action (https://github.com/muttleyxd/clang-format-static-binaries 5b56bb49b977b80cd18f44df4444db93e2d30f46) does show changes (which look wrong to me)
• clang-format 13.0.0 from the Github action's repo does not show any changes
• clang-format 10.0.0-4ubuntu1 (Ubuntu 20.04) does show changes (which look wrong to me)

Conclusion: clang-format 10 consistenly makes improper changes. clang-format 13 consistenly fixes it.

Also I encountered CI breaking long lines to soon (not according rules), which didn't happen on any version locally.

Do you have a specific example where CI fails for this and local checks don't?

P.S.: Also see pljones's comment

Suddenly it's consistent. Nothing in that area of the code changed. But suddenly my local make clang_format complained, too. Just weird, which was my first reaction.

I saw that, but that's hard to investigate if it's no longer reproducible.

Some things seem to be inconsistant independant of the clang-format version...

I'm trying to get this "seem to" backed by facts which make it reproducible (and hopefully: fixable).

P.S.2: Did CI already run a test on this commit ? It might "bypass" the problem and I don't see a failed check yet.

The check was waiting for approval. I did that just now and it's green: https://github.com/jamulussoftware/jamulus/runs/5757957920?check_suite_focus=true

[pgScorpio]

@hoffie

According to the action, the commit was e36d3d6. I think I had already checked this in detail, but I've just done it again:

No this doesnt' seem to be the same commit to me, though they both include the same problem in settings.cpp. But my link is a later commit, trying to fix all the clang-format errors. And on that one I DID check on both 10.0.0 and 13.0.1 where both gave the same OK result, but on CI only this, single, specific problem with alignment persisted.

• clang-format 13.0.1 (Arch) does not show any changes
• clang-format 10.0.0 from the GIthub action (https://github.com/muttleyxd/clang-format-static-binaries 5b56bb49b977b80cd18f44df4444db93e2d30f46) does show changes (which look wrong to me)
• clang-format 13.0.0 from the Github action's repo does not show any changes
• clang-format 10.0.0-4ubuntu1 (Ubuntu 20.04) does show changes (which look wrong to me)
  Conclusion: clang-format 10 consistenly makes improper changes. clang-format 13 consistenly fixes it.

Again strange that this NOT according my test results. (Again, there seems to be playing more, but we still have to find out what...)

Also I encountered CI breaking long lines to soon (not according rules), which didn't happen on any version locally.

Do you have a specific example where CI fails for this and local checks don't?

Also see: https://github.com/jamulussoftware/jamulus/runs/5264767533?check_suite_focus=true [Line 70]

And I just did some thorough testing on that one with some logical, as well as remarkable, results:
I created 3 header files with just this specific line of 149 characters long. Keeping one as reference, one to clang-format on Windows with v13.0.1 and one to clang-format on Linux with v10.0.0.
On the first test both Linux and windows broke the lines! WTF! I'm sure this didn't happen before!
Than I realized that the folder these test files where in did not contain a clang-format configuration, so I copied those configuration files from my Jamulus folder to the folder with the test files and repeated the test...
Now Windows did NOT break the line, but Linux did. WTF! again not according my first results! So I looked for further differences and found out that my test files where ANSI format (since I created a "new text file" on windows), but the original file was UNIX format, So I created new test files in UNIX format.
And now YES! the same result as my very first result, Neither Windows nor Linux broke the line !

So
1: The current directory must contain the right clang-format configuration files to be consistent (Seems logical, but are we sure this is always the case on the CI test? and are we sure this is always the case when we manually check-clang format? or when we clang-format from within another development tool?)
2: It seems that with ANSI formatted files the Linux version of clang-format does count the extra \r as a character, the Windows version doesn't. (Maybe also logical, but not quite as expected.)
3: So what happens if someone changes a ANSI formatted file on Linux? Saving the file will probably change the format to UNIX and this might change clang-format behavior. Same might be the case with UNIX formated files on Windows, all depends on the used editing tools I guess.

P.S.: Also see pljones's comment

Suddenly it's consistent. Nothing in that area of the code changed. But suddenly my local make clang_format complained, too. Just weird, which was my first reaction.

I saw that, but that's hard to investigate if it's no longer reproducible.

Some things seem to be inconsistant independant of the clang-format version...
I'm trying to get this "seem to" backed by facts which make it reproducible (and hopefully: fixable).
(ede3242) commit ? It might "bypass" the problem and I don't see a failed check yet.

But it again might indicate there is more playing than just clang-format versions...

P.S.2: Did CI already run a test on [this]
The check was waiting for approval. I did that just now and it's green: https://github.com/jamulussoftware/jamulus/runs/5757957920?check_suite_focus=true

So this really seems to be a way to avoid at least "some" clang-format problems...

So, what I did was, do NOT use clang-format off/on around parts of statements, and use empty lines before and after clang-format off/on. (Though I'm not yet sure if the empty lines actually make a difference, they certainly make the clang-format controls themselves stand out more.)

[hoffie]

No this doesnt' seem to be the same commit to me, though they both include the same problem in settings.cpp. But my link is a later commit, trying to fix all the clang-format errors. And on that one I DID check on both 10.0.0 and 13.0.1 where both gave the same OK result, but on CI only this, single, specific problem with alignment persisted.

Can you help me pin-point the commit you mean?
You had linked to this action run:
https://github.com/jamulussoftware/jamulus/runs/5704004785?check_suite_focus=true
The checkout action shows a log of what it has done:
https://github.com/jamulussoftware/jamulus/runs/5704004785?check_suite_focus=true#step:3:474

That says e36d3d6, so that's what this Action run was about. Maybe you meant a different run?

1: The current directory must contain the right clang-format configuration files to be consistent (Seems logical, but are we sure this is always the case on the CI test?

Yes, there's only one CI job for that which always runs from the checkout root.

and are we sure this is always the case when we manually check-clang format

make clang_format only works when started from the checkout root, where both the Makefile and the clang config lives.

or when we clang-format from within another development tool?

We can't control that, but I hope that most IDEs/editors properly handle that case. It's not Jamulus-specific and I don't think it's appropriate to spray .clang-format files into each and every source dir.

2: It seems that with ANSI formatted files the Linux version of clang-format does count the extra \r as a character, the Windows version doesn't. (Maybe also logical, but not quite as expected.)

Ah, nice finding!

3: So what happens if someone changes a ANSI formatted file on Linux? Saving the file will probably change the format to UNIX and this might change clang-format behavior. Same might be the case with UNIX formated files on Windows, all depends on the used editing tools I guess.

Do you mean LF (\n) vs. CR-LF (\r\n)? All source files should be LF-only anyway. Current git seems clean in that way. We could consider adding/extending .editorconfig to help editors doing the right thing in the first place here. That's not directly related to clang-format though.

[pgScorpio]

Can you help me pin-point the commit you mean?
You had linked to this action run:
https://github.com/jamulussoftware/jamulus/runs/5704004785?check_suite_focus=true
The checkout action shows a log of what it has done:
https://github.com/jamulussoftware/jamulus/runs/5704004785?check_suite_focus=true#step:3:474

Yes correct, and this run still had a difference with the "alignment" problem. Which seems to have to do with the placing if the "clang-format off/on". (I have a later commit changing these clang-format off/on positions, and that one solved the problem.)

make clang_format only works when started from the checkout root, where both the Makefile and the clang config lives.

Yet but a manual clang-format -i <file> must be executed from root too! and this is not guaranteed, nor stated anywhere in the documentation.

I don't think it's appropriate to spray .clang-format files into each and every source dir

I totally agree, but it's worth nothing to verify if embedded tools use the correct clang-format config. Using make clang_format always seems the safest to me.

All source files should be LF-only anyway

Is there a way to guarantee LF-only using clang-format ? Since creating a new file on Windows will default to CR/LF !

[hoffie]

Yes correct, and this run still had a difference with the "alignment" problem. Which seems to have to do with the placing if the "clang-format off/on". (I have a later commit changing these clang-format off/on positions, and that one solved the problem.)

So... I'm a bit lost -- is there still some unexplainable thing to investigate in this context? If so, which?

Yet but a manual clang-format -i <file> must be executed from root too! and this is not guaranteed, nor stated anywhere in the documentation.

I agree. Can you add a suggestion or comment to @ann0see's #2561?

I don't think it's appropriate to spray .clang-format files into each and every source dir

I totally agree, but it's worth nothing to verify if embedded tools use the correct clang-format config. Using make clang_format always seems the safest to me.

I don't think we should advise against using tool-integrated formatting. It has to match what clang-format does, but the recent two PRs have been the only instances where we had actual confusions/problems around this area at all (and it isn't related to tool integrations, but to different clang-format versions, it seems).

Is there a way to guarantee LF-only using clang-format ?

Yes, it seems like the following config should work, but is only supported as of clang-format 11, so we could only do that after bumping the version, e.g. via #2559.

DeriveLineEnding: false  # LLVM format sets this to true: https://github.com/llvm/llvm-project/blob/b89b18e350e11efc599f6ce2bb55cbec89a0efe1/clang/lib/Format/Format.cpp#L1243
UseCRLF: false

I'm happy to open a PR for this, but it requires a decision on #2559 first.

Other than that:

• Git can help with line endings: https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings
• Editors/IDE's respecting .editorconfig can help with this: https://editorconfig.org/ (will submit a PR for that as well)

Since creating a new file on Windows will default to CR/LF !

I'd assume that this depends on the IDE/editor and its config.

[pgScorpio]

I'd assume that this depends on the IDE/editor and its config.

And, most important gitconfig !
If text is set to text=auto "Git will handle the files in whatever way it thinks is best", which seems to be "OS default format", so on Windows all files will have CR/LF and so clang-format results can differ.
Same occurs when text eol is left at default. So I think it is wise to set sourcefiles to text eol=lf

[ann0see]

Closing as I think this is fixed. #3020