| Message ID | 20240713134518.773053-9-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clang-format: add more rules and enable on CI | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm > +echo "RemoveBracesLLVM: true" >> .clang-format > + > git clang-format --style file --diff --extensions c,h "$baseCommit" Style: lose the SP between the redirection operator and its operand. I know this is well intentioned, but will there be any downsides for running the check always against a dirty working tree? I know during a CI run, the working tree will not be completely clean, as we create and leave build artifacts, but this is as far as I know the first instance of us munging a tracked path, changing the working tree in a way that "git describe --dirty" would notice. This is done as the last (and only) step of check-style job and the ci/run-style-check.sh script may not do anything like "git describe --dirty" right now, but things can change. Perhaps I am worried about this a bit too much. I unfortunately couldn't find an option to "git clang-format" to tell it to read from an extra file in addition to the usual ".clang-format" file---if such an option existed, we obviously could use an untracked/ignored file to prepare the custom format file and use it without making the working tree dirty. So perhaps the posted change, while making us feel dirty, is the best we could do for this step. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm >> +echo "RemoveBracesLLVM: true" >> .clang-format >> + >> git clang-format --style file --diff --extensions c,h "$baseCommit" > > Style: lose the SP between the redirection operator and its operand. > Will change. > I know this is well intentioned, but will there be any downsides for > running the check always against a dirty working tree? > > I know during a CI run, the working tree will not be completely > clean, as we create and leave build artifacts, but this is as far as > I know the first instance of us munging a tracked path, changing the > working tree in a way that "git describe --dirty" would notice. > > This is done as the last (and only) step of check-style job and the > ci/run-style-check.sh script may not do anything like "git describe > --dirty" right now, but things can change. Perhaps I am worried > about this a bit too much. > Exactly my thoughts too. I did test on GitLab's CI and all other jobs were unaffected. So I think we're good here. --- After reading your mail, I figured I should also check GitHub's CI for this particular change and realized there is a slight issue with my current series. GitLab's CI highlights style check jobs which failed with a yellow warning symbol [1], even with the 'ignore failing check-style' directive. But GitHub's actions, simply marks the job as successful even if the job failed [1]. This was an oversight on my side, since I expected similar behavior. Seems like the required dependency wasn't even installed on GitHub causing 'git clang-format' to fail. Unfortunately this means all GitHub workflows for style-check will be green even if there were style issues found. I couldn't find a way to fix this from reading the documentation. This will not be an issue once we enforce, but till then users cannot rely on the job's outcome for the job's status in GitHub. They will have to see the logs to know if style check failed. I will re-roll with a fix, but will wait a day or so, to avoid spamming. > I unfortunately couldn't find an option to "git clang-format" to > tell it to read from an extra file in addition to the usual > ".clang-format" file---if such an option existed, we obviously could > use an untracked/ignored file to prepare the custom format file and > use it without making the working tree dirty. > This was also something I looked for, but couldn't find. I should have added that to the commit message. Will do so in the reroll. > So perhaps the posted change, while making us feel dirty, is the > best we could do for this step. > > Thanks. Yes, I think it is okay. I'm hoping we can move it in-tree someday. [1]: https://gitlab.com/gitlab-org/git/-/pipelines/1372326813 [2]: https://github.com/KarthikNayak/git/actions/runs/9921272597/job/27409047062
On 13/07/2024 17:46, Karthik Nayak wrote: > Junio C Hamano <gitster@pobox.com> writes: [snip] >> I unfortunately couldn't find an option to "git clang-format" to >> tell it to read from an extra file in addition to the usual >> ".clang-format" file---if such an option existed, we obviously could >> use an untracked/ignored file to prepare the custom format file and >> use it without making the working tree dirty. >> > > This was also something I looked for, but couldn't find. I should have > added that to the commit message. Will do so in the reroll. > I had a need recently to try applying the git '.clang-format' file to a different project: $ pwd /home/ramsay/sparse $ clang-format --style=file:/home/ramsay/git/.clang-format sparse.c >xxx.c $ meld sparse.c xxx.c # oh my lord :) Note that I had to specify '/home/ramsay/' rather than just '~', since it does not get recognized/expanded in that position: $ clang-format --style=file:~/git/.clang-format sparse.c >xxx.c Error reading ~/git/.clang-format: No such file or directory $ rm xxx.c Also, as you can see, this was 'clang-format' not 'git-clang-format' (which is what would actually be used in this situation), but the '--help' text claims that: $ git-clang-format --help | grep style clangFormat.style --style STYLE passed to clang-format $ .. so it should work (but I have not actually tried it, so YMMV ;) ). [So, munging the .clang-format file with sed (say) to a temp file and using the --style=file:tmp-file syntax should (hopefully) work] ATB, Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > On 13/07/2024 17:46, Karthik Nayak wrote: >> Junio C Hamano <gitster@pobox.com> writes: > [snip] > >>> I unfortunately couldn't find an option to "git clang-format" to >>> tell it to read from an extra file in addition to the usual >>> ".clang-format" file---if such an option existed, we obviously could >>> use an untracked/ignored file to prepare the custom format file and >>> use it without making the working tree dirty. >>> >> >> This was also something I looked for, but couldn't find. I should have >> added that to the commit message. Will do so in the reroll. >> > > I had a need recently to try applying the git '.clang-format' file to a > different project: > > $ pwd > /home/ramsay/sparse > $ clang-format --style=file:/home/ramsay/git/.clang-format sparse.c >xxx.c > $ meld sparse.c xxx.c # oh my lord :) > > Note that I had to specify '/home/ramsay/' rather than just '~', since it > does not get recognized/expanded in that position: > > $ clang-format --style=file:~/git/.clang-format sparse.c >xxx.c > Error reading ~/git/.clang-format: No such file or directory > $ rm xxx.c > > Also, as you can see, this was 'clang-format' not 'git-clang-format' (which > is what would actually be used in this situation), but the '--help' text > claims that: > > $ git-clang-format --help | grep style > clangFormat.style > --style STYLE passed to clang-format > $ > > .. so it should work (but I have not actually tried it, so YMMV ;) ). > > [So, munging the .clang-format file with sed (say) to a temp file and > using the --style=file:tmp-file syntax should (hopefully) work] > > ATB, > Ramsay Jones > > Hello, Providing a path does work indeed. But we were discussing the option to provide an additional path apart from the default '.clang-format'. The option you mentioned `--style=<file path>` will set the config to the contents of <file path>, but we want to add on top of that. So that we could hypothetically do something like $ git clang-format --style file --style-append '.ci-clang-format' --diff --extensions c,h But seems like this is currently not possible.
Karthik Nayak <karthik.188@gmail.com> writes: > Providing a path does work indeed. But we were discussing the option to > provide an additional path apart from the default '.clang-format'. But that is not a requirement. The requirement is to allow us to use what we have in .clang-format plus one other rule. And if that requirement is met by copying the entire contents in .clang-format to a temporary file, adding the other one to the same temporary file, and then using the temporary file instead of .clang-format, that is fine, isn't it?
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> Providing a path does work indeed. But we were discussing the option to >> provide an additional path apart from the default '.clang-format'. > > But that is not a requirement. The requirement is to allow us to > use what we have in .clang-format plus one other rule. > > And if that requirement is met by copying the entire contents in > .clang-format to a temporary file, adding the other one to the same > temporary file, and then using the temporary file instead of > .clang-format, that is fine, isn't it? Ah right. Let me summarise: - Method 1: Inject the extra config to '.clang-format' in the CI's job. This is the current method. - Method 2: Create '.clang-format-ci' to use in the CI - Method 2.a: The new file contains '.clang-format' + CI specific rules. - Method 2.b: The new file simply contains the new rules and we inject the rest in the CI's job. I'd say methods '1' and '2.b' are similar, since they modify the tree on the CI. So no real benefit of one over the other, no? The issue with method '2.a' is that we have two sources of truth and we need to ensure both files are updates always. Since the former methods don't have any cons of maintenance apart from seeming a bit hacky, I'd prefer that. But happy to go the other way too!
Karthik Nayak <karthik.188@gmail.com> writes: > Ah right. Let me summarise: > - Method 1: Inject the extra config to '.clang-format' in the CI's job. > This is the current method. > - Method 2: Create '.clang-format-ci' to use in the CI > - Method 2.a: The new file contains '.clang-format' + CI specific > rules. > - Method 2.b: The new file simply contains the new rules and we inject > the rest in the CI's job. > > I'd say methods '1' and '2.b' are similar, since they modify the tree on > the CI. So no real benefit of one over the other, no? Sorry, but I am not sure what you are trying to say, especially with 2.a and 2.b, your assumption on "the new file". Is it tracked? Try running "git describe --dirty" and see if the command can tell the difference. If you smudge .clang-format, which is a tracked file, it will be noticed. But you can use a temporary file and use --style=file:/... to point at it. The temporary file can be an untracked and ignored file, just like any *.o files we would create during a build. Then "git describe --dirty" would not complain that you are making the working tree dirty. The temporary file does not even have to be inside our working tree. If we know we can write into /tmp/clang-format-rules file, then the CI script can do something like { cat .clang-format echo echo "RemoveBracesLLVM: true" } >/tmp/clang-format-rules git clang-format --style=file:/tmp/clang-format-rules \ --diff --extensions c,h "$baseCommit" right? Then "git status" would even say "there is no untracked cruft" (although I do not know we *need* to keep the working tree that clean, without untracked cruft).
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> Ah right. Let me summarise: >> - Method 1: Inject the extra config to '.clang-format' in the CI's job. >> This is the current method. >> - Method 2: Create '.clang-format-ci' to use in the CI >> - Method 2.a: The new file contains '.clang-format' + CI specific >> rules. >> - Method 2.b: The new file simply contains the new rules and we inject >> the rest in the CI's job. >> >> I'd say methods '1' and '2.b' are similar, since they modify the tree on >> the CI. So no real benefit of one over the other, no? > > Sorry, but I am not sure what you are trying to say, especially with > 2.a and 2.b, your assumption on "the new file". Is it tracked? > Yes. I was referring to a tracked file. > Try running "git describe --dirty" and see if the command can tell > the difference. If you smudge .clang-format, which is a tracked > file, it will be noticed. > > But you can use a temporary file and use --style=file:/... to point > at it. The temporary file can be an untracked and ignored file, > just like any *.o files we would create during a build. Then "git > describe --dirty" would not complain that you are making the working > tree dirty. > > The temporary file does not even have to be inside our working tree. I feel stupid now. I completely didn't think of this and this solves everything. Thanks for being explicit here. The temporary file absolutely doesn't have to be in our working tree. > If we know we can write into /tmp/clang-format-rules file, then the > CI script can do something like > > { > cat .clang-format > echo echo "RemoveBracesLLVM: true" > } >/tmp/clang-format-rules > git clang-format --style=file:/tmp/clang-format-rules \ > --diff --extensions c,h "$baseCommit" > > right? Then "git status" would even say "there is no untracked > cruft" (although I do not know we *need* to keep the working tree > that clean, without untracked cruft). Yes this is the best solution.
Karthik Nayak <karthik.188@gmail.com> writes: >> If we know we can write into /tmp/clang-format-rules file, then the >> CI script can do something like >> >> { >> cat .clang-format >> echo echo "RemoveBracesLLVM: true" >> } >/tmp/clang-format-rules >> git clang-format --style=file:/tmp/clang-format-rules \ >> --diff --extensions c,h "$baseCommit" >> >> right? Then "git status" would even say "there is no untracked >> cruft" (although I do not know we *need* to keep the working tree >> that clean, without untracked cruft). > > Yes this is the best solution. FWIW, I think an in-tree throw-away file is a better option, simply because we _know_ that the working tree can be written (by the fact that we can do "make" and have compilers write *.o and other cruft), but we do not know if the CI environment allows us to write to /tmp or /var/tmp or /usr/local/tmp and you do not want to run around and see if there is a suitable temporary directory you can use.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >>> If we know we can write into /tmp/clang-format-rules file, then the >>> CI script can do something like >>> >>> { >>> cat .clang-format >>> echo echo "RemoveBracesLLVM: true" >>> } >/tmp/clang-format-rules >>> git clang-format --style=file:/tmp/clang-format-rules \ >>> --diff --extensions c,h "$baseCommit" >>> >>> right? Then "git status" would even say "there is no untracked >>> cruft" (although I do not know we *need* to keep the working tree >>> that clean, without untracked cruft). >> >> Yes this is the best solution. > > FWIW, I think an in-tree throw-away file is a better option, simply > because we _know_ that the working tree can be written (by the fact > that we can do "make" and have compilers write *.o and other cruft), > but we do not know if the CI environment allows us to write to /tmp > or /var/tmp or /usr/local/tmp and you do not want to run around and > see if there is a suitable temporary directory you can use. Sorry I got a bit busy with work. But I did end up testing out writing to a temp file and it works on both GitHub and GitLab CIs. Also I found some of the rules are too new for the clang-format in GitHub, so I removed some of them. Overall, I've sent a new version with these changes.
Karthik Nayak <karthik.188@gmail.com> writes: > Sorry I got a bit busy with work. But I did end up testing out writing > to a temp file and it works on both GitHub and GitLab CIs. Also I found > some of the rules are too new for the clang-format in GitHub, so I > removed some of them. Thanks, both are really appreciated, especially the latter about checking the features that are available.
diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh index 76dd37d22b..1dce50a0ac 100755 --- a/ci/run-style-check.sh +++ b/ci/run-style-check.sh @@ -5,4 +5,17 @@ baseCommit=$1 +# Remove optional braces of control statements (if, else, for, and while) +# according to the LLVM coding style. This avoids braces on simple +# single-statement bodies of statements but keeps braces if one side of +# if/else if/.../else cascade has multi-statement body. +# +# As this rule comes with a warning [1], we want to experiment with it +# before adding it in-tree. since the CI job for the style check is allowed +# to fail, appending the rule here allows us to validate its efficacy. +# While also ensuring that end-users are not affected directly. +# +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm +echo "RemoveBracesLLVM: true" >> .clang-format + git clang-format --style file --diff --extensions c,h "$baseCommit"
For 'clang-format' setting 'RemoveBracesLLVM' to 'true', adds a check to ensure we avoid curly braces for single-statement bodies in conditional blocks. However, the option does come with two warnings [1]: This option will be renamed and expanded to support other styles. and Setting this option to true could lead to incorrect code formatting due to clang-format’s lack of complete semantic information. As such, extra care should be taken to review code changes made by this option. The latter seems to be of concern. While we want to experiment with the rule, adding it to the in-tree '.clang-format' could affect end-users. Let's only add it to the CI jobs for now. With time, we can evaluate its efficacy and decide if we want to add it to '.clang-format' or retract it entirely. [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- ci/run-style-check.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)