Move formatting to a separate workflow

[DolphinChips]

No description provided.

[convextriangle]

curious for the context - why was this change needed?

[DolphinChips]

curious for the context - why was this change needed?

@convextriangle build process doesn't depend on actual formatting, so makes sense to split it out and run in parallel

I saw build failing in #188 and wanted to see why it failed, turns out it wasn't even build-related ¯\_(ツ)_/¯ so that's where I got an idea for this change

[convextriangle]

@convextriangle build process doesn't depend on actual formatting, so makes sense to split it out and run in parallel

i kind of see the point but do we really need to do this? why waste time waiting for build completion when someone can't even format their code

I saw build failing in #188 and wanted to see why it failed, turns out it wasn't even build-related ¯\_(ツ)_/¯ so that's where I got an idea for this change

likely a better solution would be auto-formatting support for common editors and such, not sure

[DolphinChips]

i kind of see the point but do we really need to do this? why waste time waiting for build completion when someone can't even format their code

why not?

[convextriangle]

i kind of see the point but do we really need to do this? why waste time waiting for build completion when someone can't even format their code

why not?

my thought was to have some sanity checks which would rule out common setup mistakes (such as someone formatting only in one big commit and breaking reviews in the progress), and would probably save some CI time (fail fast instead of waiting for build) -- an indication to the dev that their env is sorta broken would be nice
but fwiw if this doesn't make the formatting optional and will be a mandatory check and etc. then this is fine

[ScottBrenner]

Would it make sense to refactor this as a separate job in the same workflow?

[uvcat7]

Having a separate build item might make the problem clearer, but it only takes a few seconds to diagnose as is.

That said, I've had formatting errors break builds twice while using the "correct" environment:
First when I merged code I wrote before the formatting change in that used tabs instead of spaces.
Second when I tried to use clang-tidy to fix errors in the build process, since it made lines >80 characters.

In both cases the CI broke but there were no warnings on my local build.
Clang-tidy also doesn't detect the same errors as the build CI--that should also be reconciled.

Long story short, I think breaking nonconformant compilation on CI is fine. It'd save me effort reviewing PRs. But it should do the same locally so I'm not playing whack-a-mole.

Does that sound like a better solution?

[DolphinChips]

Having a separate build item might make the problem clearer, but it only takes a few seconds to diagnose as is.

I see formatting and build process as two separate problems, broken code can pass formatting check and vice versa, so makes sense to decouple them in CI too, as I mentioned above.

Long story short, I think breaking nonconformant compilation on CI is fine. It'd save me effort reviewing PRs. But it should do the same locally so I'm not playing whack-a-mole.

Does that sound like a better solution?

Yeah, checking for formatting while compiling would be ideal, but CMake developers sorta disagree with us. Pre-commit/pre-push hooks get us close, but those require manual setup, which is better than nothing

[DolphinChips]

Would it make sense to refactor this as a separate job in the same workflow?

Sure, for some reason I thought build matrices apply to an entire workflow and not to a job

[uvcat7]

I see formatting and build process as two separate problems, broken code can pass formatting check and vice versa, so makes sense to decouple them in CI too, as I mentioned above.

Sure, go for it.

Yeah, checking for formatting while compiling would be ideal, but CMake developers sorta disagree with us. Pre-commit/pre-push hooks get us close, but those require manual setup, which is better than nothing

We don't need to automatically format the build files; just error out if clang-format fails when building locally. That has to be doable.

[uvcat7]

@DolphinChips can you update this per bren's suggestion to refactor this into the windows.yml file?

I've handled the formatting improvements in #200.

[ScottBrenner]

Regarding #189 (comment), you can simply move formatting out to a separate job under the top-level jobs key in the workflow - reference https://github.com/itgmania/itgmania/blob/release/.github/workflows/ci.yml#L68