Make clang-tidy feedback on GitHub much more responsive

[moxian]

Summary

Infrastructure "Make clang-tidy feedback on GitHub much more responsive"

Purpose of change

clang-tidy is a wonderful tool, and we should utilize it more.
Unfortunately, if your PR touches any mildly popular header, then clang-tidy would run on all .cpp files that transitvely include it and take hours. This effectively means that the diagnostics are only addressed next day. And if they are not addressed correctly on the first try - then one more day per each attempt.
This is unbearable, and makes people (both authors and maintainers) ignore diagnostics leading to situations such as #78616
This PR makes clang-tidy diagnostic only* (but see below) run on files directly affected, cutting down on latency massively, and increasing up development velocity.

Describe the solution

Currently the fileset for clang-tidy to analyze is split into two parts - affected files inside src/ and affected files outside of src/. Each having its own github action jobs.
This PR introduces a third job - files explicitly touched by the PR (and excludes those from the other two). This is usually a much much smaller set of files, so only takes 5 minutes or so to complete (depending on the PR of course), comparable to basic build.

I had a bit of a trouble coming up with a good name for that, so I ended up renaming existing jobs. The new set of names is

• directly-changed (the files explicitly changed in the pr)
• indirectly-changed-src (files in src/ that are transitively affected by the header changes, but not explicitly touched)
• indirectly-changed-other (files outside of src/, transitively affected by the header changes, but not explicitly touched)

You can see demo run of how it would look in my fork - moxian#2

Drive-by changes: silence log output of things we don't care about (generating the header map that's used to figure out which files are transitively affected or not), and loudening the log output of things we do (actual clang-tidy invocation)

Describe alternatives you've considered

Completely exclude the indirectly-changed files from analysis.
Optionally, move them to a daily/weekly job that would post the findings in a bug.

My original rationale for it was that it's very unlikely to make header changes that would break clang-tidy for the files including those without breaking the compilation of those files in th first place (and thus neccessiating changes there).
The only such examples I could think of would be const-qualifying a method in a header resulting in an opportunity to convert for ( auto thing : container) { thing.method() } into for( const auto thing& : container ) { thing.method() } , but those are rare and perhaps insignificant.

But thinking more, there's little harm to keep the indirectly-changed checks around. They might prevent the PR from being merged for extra hour or four, but they don't really negatively affect PR author. And sometimes they can catch neat stuff. So they can staty.

Testing

See the PR in my fork: moxian#2

• The base commit is the same as this PR.
• The demo commit is one where I do insignificant changes, and checks pass (modified to ignore the changes to clang-tidy-wrapper.sh itself, so as not to run on literally all the files) . Run link: https://github.com/moxian/Cataclysm-DDA/actions/runs/12521590012
• The break it commit - as the name implies - breaks things, checks fail. Run link: https://github.com/moxian/Cataclysm-DDA/actions/runs/12521740315

Notice the difference in runtime of the checks and how the one that's reporting the error is much faster than the one that's not. (And achievement.h is not even a very popular header)

Tested locally on WSL. Does work (analyzing all files), but the script is janky, so perhaps it is not supposed to be run locally (and I would hope it stays this way)

Additional context

It annoys me a little bit that clang-tidy is rebuilt (and re-tested) for each of the (now) three jobs, taking 5 minutes per. Would be nice to build it only once, and reuse it across those. This is somewhat easily doable, but out of scope.
Would also be nice to cache the binary across different PRs (or same PR, but different commits), but I'm not sure how to approach that one sanely.

[kevingranade]

So... the current job that builds every file containing a modified header is going to be mandatory for all builds Soon ™️ so this isn't going to accomplish anything. That will adress the issue of PRs getting merged is they fail clang-tidy.

The only hold up left is that there may be PRs that need rebasing to get them to pick up the updated build rules from #78339

Yes it takes a long time if you touch the wrong header file but it also catches real issues and best practices, so it needs to happen.

[moxian]

The purpose of this PR is to add a new check that only checks the modified files, so that the author can hear back sooner about the code they changed, because it's much more likely to contain clang-tidy errors than the code they didn't change.

The full set of files being linted isn't changing. I'm fine with the full-codebase scan being mandatory. It's completely orthogonal to that. This PR is fully compatible with you intentions.

Not being able to merge the PR while the full-scan is running is totally fine.
But if we can provide a likely-useful feedback in a likely-shorter timeframe - then that's means PRs would get into a mergeable state in fewer real-time days, which is valuable.

[Brambor]

Having clang tidy fail faster would be a big improvement for developers and possibly test performance.

If I fix clang after 15 minutes instead of 2 hours, GitHub spends less time running tests on my PR: complete run + 15 minutes instead of complete run + 2 hours.

That said every run takes the extra time to run the clang another time.

It annoys me a little bit that clang-tidy is rebuilt (and re-tested) for each of the (now) three jobs, taking 5 minutes per. Would be nice to build it only once, and reuse it across those. This is somewhat easily doable, but out of scope.

If it is easily doable and you want to do it. Why not do it now? It is very related to this PR, as this PR adds another job.

[moxian]

That said every run takes the extra time to run the clang another time.

Each file is only analyzed by clang-tidy once.
The only extra time is spent on building clang-tidy.

If it is easily doable and you want to do it. Why not do it now? It is very related to this PR, as this PR adds another job.

Because I prefer small changes that are easy to audit and test, and when i try to combine too many things into one PR, they tend to get irrepairably broken. So limiting PR scope is valuable for me.

I'll start working on this then (but it will neccessarily be a different PR)

[moxian]

Basic build consistently fails (3/3) due to #78833 😔

[kevingranade]

Just to clarify, I do seem to have misunderstood the intent and this seems ok, just makes the two way partition into a three way partition.

[akrieger]

This broke clang-tidy on master runs, and possibly elsewhere. There's a bunch of errors in repo now. Nevermind that's cause I wasn't running my local tidy with LOCALIZE=1 which apparently is needed now, but wasn't before in my bash history.

Subset to analyze: 'directly-changed'
grep: ./files_changed: No such file or directory
./build-scripts/clang-tidy-run.sh: line 93: printf: write error: Broken pipe
Error: Process completed with exit code 1.

[akrieger]

There's also errors with reusing the artifact Error: Unable to download artifact(s): Artifact not found for name: cata-analyzer-plugin - https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12838010130/job/35802803651#step:4:17, maybe on drafts where it doesn't get built at all?

[akrieger]

Ah no that was a json only change so skip-duplicates triggered but its only skipping some of the steps. So this is going to red-error clang-tidy on json changes.

[moxian]

These problems stem from #79100 which is techincally a different PR.
I acknowledge the issues, and will try and make a fix for both, but probably tomorrow (for a definition of "tomorrow", timezones and all)

Apologies for the trouble.

Edit: some (most??) json-only changes pass clang-tidy just fine. e.g.: https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12826245046/job/35766017697?pr=79204 . Anyway, will take a closer look a bit later.