ci: Improved clang-format CI task #4647
Conversation
lgritz
force-pushed
the
lg-clangformat
branch
from
1ad7bbd to
6c8f370
Compare
|
Any objections? |
|
Maybe not an objection but a suggestion, perhaps we could run clang-tidy first before any other builds trigger to avoid the whole CI running when that check already fails? Especially now that clang-tidy only takes 45s |
I'm not sure. When I do a push or submit a PR, I want to know if the formatting is ok, but also I want to know if the build succeeds and the testsuite works (across a bunch of different platforms). I'm not sure I want one of those failing to prevent me from knowing the status of the other, if you know what I mean. But I'm interested in hearing others' opinions on this. Maybe it would work if combined with some actions trickery that will post the fixes as a PR comment with a diff that can be accepted to amend the PR with a single click? (I think this is possible.) |
100% agree with this. Formatted code should not be a blocker for functional validation when iterating on changesets. |
|
My original intention for the comment was twofold, of which I now realize one might be irrelevant: 1: if the CI will fail anyways without a valid clang-tidy, it is wasteful to run the rest of the suite only to the have to rerun it. Although, since oiio uses github hosted runners I suppose this is not an issue I do understand your concerns though and it is a fair point that the clang-tidy blocking the rest of the checks is a bit unfortunate. As a compromise solution I could come up with, especially for first time contributors, is that clang-tidy always runs and does not need explicit approval from a collaborator to kick off. That way, if theyre part of the first group they could iterate and chip away at those errors faster. |
|
There are some actions in the GHA marketplace (or maybe we can roll our own) that do the clang-format operation and then can post a PR comment, which the original poster can accept and have the PR amended. I think that can be an effective way to remove the burden from first-time contributors not knowing that/how they should format the code before submission. |
|
I think that would be a good addition (albeit as a different MR) |
Old: Do a build with target 'clang-format', relying on our build system. But that means that we need to do the cmake config step, and need a bunch of dependencies set up so that the config doesn't fail. New: Make a new run-clang-format.bash utility to issue the commands for the right files. No "build" is requred, and the only dependencies we need are the minimum it takes to run clang-format itself. Also, I noticed that we never had been clang-format testing the .cpp and .h files in the testsuite itself. So enable that now, and commit the small amount of reformatting that it identified. These changes reduce the approximate total elapsed time of a CI clang-format job from nearly 7:00 to 0:35. Signed-off-by: Larry Gritz <[email protected]>
lgritz
force-pushed
the
lg-clangformat
branch
from
6c8f370 to
e6de240
Compare
|
No objections voiced, merging. The CI failure in the Windows test is happening in all branches and is unrelated to this PR, so I'm not going to let that hold me up. (It looks like the GH Windows 2022 runner updated its MSVS version yesterday.) |
lgritz
merged commit a9c3dd6
into
AcademySoftwareFoundation:main
Old: Do a build with target 'clang-format', relying on our build system. But that means that we need to do the cmake config step, and need a bunch of dependencies set up so that the config doesn't fail. New: Make a new run-clang-format.bash utility to issue the commands for the right files. No "build" is requred, and the only dependencies we need are the minimum it takes to run clang-format itself. These changes reduce the approximate total elapsed time of a CI clang-format job from nearly 7:00 to around 0:45. Also, I noticed that we never had been clang-format testing the .cpp and .h files in the testsuite itself. So enable that now, and commit the small amount of reformatting that it identified. Signed-off-by: Larry Gritz <[email protected]> Signed-off-by: Scott Wilson <[email protected]>
Old: Do a build with target 'clang-format', relying on our build system. But that means that we need to do the cmake config step, and need a bunch of dependencies set up so that the config doesn't fail. New: Make a new run-clang-format.bash utility to issue the commands for the right files. No "build" is requred, and the only dependencies we need are the minimum it takes to run clang-format itself. These changes reduce the approximate total elapsed time of a CI clang-format job from nearly 7:00 to around 0:45. Also, I noticed that we never had been clang-format testing the .cpp and .h files in the testsuite itself. So enable that now, and commit the small amount of reformatting that it identified. Signed-off-by: Larry Gritz <[email protected]> Signed-off-by: Scott Wilson <[email protected]>
Old: Do a build with target 'clang-format', relying on our build system. But that means that we need to do the cmake config step, and need a bunch of dependencies set up so that the config doesn't fail. New: Make a new run-clang-format.bash utility to issue the commands for the right files. No "build" is requred, and the only dependencies we need are the minimum it takes to run clang-format itself. These changes reduce the approximate total elapsed time of a CI clang-format job from nearly 7:00 to around 0:45. Also, I noticed that we never had been clang-format testing the .cpp and .h files in the testsuite itself. So enable that now, and commit the small amount of reformatting that it identified. Signed-off-by: Larry Gritz <[email protected]>
3 participants
Old: Do a build with target 'clang-format', relying on our build system. But that means that we need to do the cmake config step, and need a bunch of dependencies set up so that the config doesn't fail.
New: Make a new run-clang-format.bash utility to issue the commands for the right files. No "build" is requred, and the only dependencies we need are the minimum it takes to run clang-format itself.
These changes reduce the approximate total elapsed time of a CI clang-format job from nearly 7:00 to around 0:45.
Also, I noticed that we never had been clang-format testing the .cpp and .h files in the testsuite itself. So enable that now, and commit the small amount of reformatting that it identified.