Shift clang-tidy to github action

[phlax]

Currently clang-tidy is not working very well and need some attention

I am gradually picking off various bits of ci for shifting azp -> gh

This one seems like quite a good candidate to start on the checks with.

[phlax]

cc @wbpcode @yanavlasov

[phlax]

clang-tidy is currently disabled pending resolution here

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[mathetake]

can we prioritize this? I genuinely think this is really bad for the long term as it directly affects the code quality plus security risk

[phlax]

its half complete - atm you can run:

./ci/do_ci.sh clang-tidy

and it kinda works

the reason i didnt land in ci is that it is not 100% - i took someone elses (aspect-based) clang-tidy implementation and hacked a few things to make it work to my expectations

i havent had too much time to do it recently - but in the past i ran it and fixed any issues - probably that would be a good place to start - the tool could do with some non-me testing and feedback

[mathetake]

so yeah one thing I noticed is

xternal/com_google_absl/absl/log/absl_check.h:47:3: note: expanded from macro 'ABSL_DCHECK'
  ABSL_LOG_INTERNAL_DCHECK_IMPL((condition), #condition)
  ^
external/com_google_absl/absl/log/internal/check_impl.h:40:3: note: expanded from macro 'ABSL_LOG_INTERNAL_DCHECK_IMPL'
  ABSL_LOG_INTERNAL_CHECK_IMPL(condition, condition_text)
  ^
external/com_google_absl/absl/log/internal/check_impl.h:26:3: note: expanded from macro 'ABSL_LOG_INTERNAL_CHECK_IMPL'
  ABSL_LOG_INTERNAL_CONDITION_FATAL(STATELESS,                        \
  ^
external/com_google_absl/absl/log/internal/conditions.h:123:3: note: expanded from macro 'ABSL_LOG_INTERNAL_CONDITION_FATAL'
  ABSL_LOG_INTERNAL_##type##_CONDITION(                                    \

can we exclude external?

[mathetake]

I think that would save tons of CPU times in CI

[phlax]

good q - i never really got to the bottom of this

i think it does exclude it to the extent that it doesnt throw errors - at least using the do_ci.sh way - but i have always been convinced that its checking a lot more than it should

[mathetake]

meanwhile I will fix the errors and warning emitted currently little by little

[mathetake]

also noticed that

• ./ci/do_ci.sh clang_tidy always exists with status=0 regardless of the detected errors
• The files pointed by CLANG_TIDY_FIX_DIFF and FIX_YAML seems empty after the run

[mathetake]

Also noticed that clang-tidy runs multiple times each time included == if a file #included multiple places, the checks run multiple times. Can we simply run find . -name *.cc -name -name *.h instead of actually building for now? That would run instantly i think.

That's the reason why it's running on external/** as well. @phlax wdyt? If it sounds good, i can give it a shot

edited: maybe the find-and-run style won't work if the file include the thirparty stuff... maybe I am missing something though

[mathetake]

I found the root cause of why it's running multiple times on the same file:

INFO: From Run clang-tidy on source/common/common/callback_impl.cc:
error: redefining builtin macro [clang-diagnostic-builtin-macro-redefined,-warnings-as-errors]
INFO: From Run clang-tidy on source/common/common/regex.cc:
error: redefining builtin macro [clang-diagnostic-builtin-macro-redefined,-warnings-as-errors]

the newly introduced toolshed clang-tidy runs clang-tidy on each file but our .clang-tidy.yaml tells it to emit warning for all envoy repo headers. We definitely should invoke clang-tidy once, not on each file...

[mathetake]

can we revert #29848 and bring the compilation_db+manual clang-tidy invocation back? I think that's much simpiler

[mathetake]

basically using aspect (i assume that runs on each build target) for clang-tidy seems not the right way

[phlax]

no!!! really lets not - that way was awful and the general advice is to shift to aspects for this kind of job

what is the issue you are having? bazel certainly adds some overhead but it has its uses also

[phlax]

the point about using aspects - iiuc - is that you dont need to compile everything to run it

[mathetake]

ok, the issue is it's running clang-tidy on each header/source files hence whenever #include source_header is located, clang-tidy tries to analyze them. Basically doing the duplicates jobs

[phlax]

hmm, sounds like a job for (remote) persistent workers perhaps

i think this is the issue that i was saying about it running more than it should tbh - not sure - it was a while ago i worked on it

iiuc it does need to do this analysis even if its not testing those files?

[mathetake]

if you pass all files all at once to clang, then clang analyze once no matter how many times they are included elsewhere vs using aspect this runs on each time, so the context is missing. But anyways nvm not that important for now.

I think asap we should do is to bring this to CI - could you clarify what's the missing steps to close issue?

[mathetake]

so the thing is this is much more serious than we think - so many uncaught errors and it's increasing day by day. linter is fundamentally important CI job for any software nowadays, and I can't imagine how are we ok with this for such long time not enabling this

[phlax]

what's the missing steps to close issue?

someone other than me testing it - im not c++ - i can get up to speed on some things - but dont have immediate uses for the tooling personally

also - was concerned about performance of it - which i think is the what you are pointing out

how about we start off with running it on a schedule - we can get a measure of what its doing - and keep on top of any required fxes

re rerunning the same analysis - surely bazel copes with this - even if its (annoyingly) printing out the output from when it was actually built, i would hope/expect that bazel is using its cached artefacts (assuming the inputs for the file were the same)

its not super fast and the output needs work - but i think it was kinda functional - lets start using it

[mathetake]

scheduling or running on main branch sounds good to me

[mathetake]

#37378 can we merge this for now? this basically blocks everything

[phlax]

surely bazel copes with this

checked code - and it doesnt - its running with those files over and again

lets land it on a schedule - ill try and think of a better scheme - need to look at how the tool itself works

[mathetake]

fyi one thing the current toolshed / aspect based clang-tidy is missing is a way to ignore certain target - notidy is specified in .bazelrc's clang-tidy config but it is not respected by the tool - I think it's harder to do the tag based ignore as it can only be attached to the build target, and once the header is included in a source file in another target then exclusion doesn't work

[mathetake]

currently

INFO: From Run clang-tidy on contrib/kafka/filters/network/source/mesh/librdkafka_utils_impl.cc:
error: redefining builtin macro [clang-diagnostic-builtin-macro-redefined,-warnings-as-errors]
INFO: From Run clang-tidy on source/extensions/filters/http/file_system_buffer/config.cc:
error: redefining builtin macro [clang-diagnostic-builtin-macro-redefined,-warnings-as-errors]

this errors is produced for every single one of source file which sounds really weird

[phlax]

yeah - i didnt figure that issue out