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]

vscode@mathetake-Venus-series:/workspaces/envoy$ clang-tidy empty.hpp -extra-arg=-std=c++17
vscode@mathetake-Venus-series:/workspaces/envoy$ clang-tidy empty.hpp -extra-arg=-std=c++20 
3 warnings generated.
error: redefining builtin macro [clang-diagnostic-builtin-macro-redefined,-warnings-as-errors]
1 warning treated as error
vscode@mathetake-Venus-series:/workspaces/envoy$ cat empty.hpp 

created empty header file and manually run clang-tidy (with the compilation_commands.json) and the error disappears with c++17 but comes in c++20. So i think the problem is something to do with our -config=clang build settings regarding the standard library.

https://github.com/llvm/llvm-project/blob/d681e1030ffd71412294d3fadc7ef673f822b832/clang/lib/Lex/PPDirectives.cpp#L3234-L3237

This is the exact line where the error emitted - I suspect that some system headers somehow avoided being recognized as a system header.

The same error is not observable with the real clang (not clang-tidy) compilation, so I think this might be a bug of clang-tidy. Maybe we should remove macro-redefined for clang-tidy target for now.

[phlax]

i should have mentioned - we only saw this appear after updating to c++20

[mathetake]

#37408 finally found the fix!

[mathetake]

maybe we want to revive the ignored targets here

envoy/ci/run_clang_tidy.sh

Lines 40 to 94 in 22e7328

	# Do not run clang-tidy against win32 impl
	# TODO(scw00): We should run clang-tidy against win32 impl once we have clang-cl support for Windows
	function exclude_win32_impl() {
	grep -v source/common/filesystem/win32/ | grep -v source/common/common/win32 | grep -v source/exe/win32 | grep -v source/common/api/win32 | grep -v source/common/event/win32
	}
	# Do not run clang-tidy against macOS impl
	# TODO: We should run clang-tidy against macOS impl for completeness
	function exclude_macos_impl() {
	grep -v source/common/filesystem/kqueue/ | grep -v source/extensions/network/dns_resolver/apple/apple_dns_impl | grep -v test/extensions/network/dns_resolver/apple/apple_dns_impl_test
	}
	# Do not run incremental clang-tidy on check_format testdata files.
	function exclude_check_format_testdata() {
	grep -v tools/testdata/check_format/
	}
	# Exclude files in third_party which are temporary forks from other OSS projects.
	function exclude_third_party() {
	grep -v third_party/ | grep -v bazel/external/http_parser
	}
	# Exclude files which are part of the Wasm emscripten environment
	function exclude_wasm_emscripten() {
	grep -v source/extensions/common/wasm/ext
	}
	# Exclude files which are part of the Wasm SDK
	function exclude_wasm_sdk() {
	grep -v proxy_wasm_cpp_sdk
	}
	# Exclude files which are part of the Wasm Host environment
	function exclude_wasm_host() {
	grep -v proxy_wasm_cpp_host
	}
	# Exclude proxy-wasm test_data.
	function exclude_wasm_test_data() {
	grep -v wasm/test_data
	}
	# Exclude files which are part of the Wasm examples
	function exclude_wasm_examples() {
	grep -v examples/wasm
	}
	# Exclude envoy mobile.
	function exclude_envoy_mobile() {
	grep -v mobile/
	}
	function filter_excludes() {
	exclude_envoy_mobile | exclude_check_format_testdata | exclude_win32_impl | exclude_macos_impl | exclude_third_party | exclude_wasm_emscripten | exclude_wasm_sdk | exclude_wasm_host | exclude_wasm_test_data | exclude_wasm_examples
	}

[mathetake]

@phlax do you have any update

[phlax]

no - ive been busy with other things

[mathetake]

ERROR: /workspaces/envoy/bazel/foreign_cc/BUILD:180:15: Foreign Cc - Configure: Building unicode_icu_build failed: (Exit 2): bash failed: error executing CcConfigureMakeRule command (from target //bazel/foreign_cc:unicode_icu_build) 
  (cd /build/bazel_root/base/sandbox/processwrapper-sandbox/2684/execroot/envoy && \
  exec env - \
    BAZEL_COMPILER=clang \
    BAZEL_LINKLIBS=-l%:libstdc++.a \
    BAZEL_LINKOPTS=-lm \
    CC=clang \
    CXX=clang++ \
    LLVM_CONFIG=/opt/llvm/bin/llvm-config \
    PATH=/opt/llvm/bin:/opt/llvm/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin \
  /bin/bash -c bazel-out/k8-fastbuild/bin/bazel/foreign_cc/unicode_icu_build_foreign_cc/wrapper_build_script.sh)
# Configuration: 3acc140781edcb72c0227536a2a67000f32d5fa673f7821dfe96065478be7234
# Execution platform: @@internal_platforms_do_not_use//host:host

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
rules_foreign_cc: Build failed!
rules_foreign_cc: Printing build logs:
_____ BEGIN BUILD LOGS _____

been a while since my last run, but now I get this build error. Looks like foreign CC is trying to compile everything during the clang-tidy target?

[phlax]

foreign CC is trying to compile everything

thats odd - i think the aspect just pulls in the sources - nothing should be built iirc

[phlax]

fwiw im hitting the same issue when trying to run the aspect in staging

[phlax]

hmm - not 100% certain but i think its not building its configuring (ie make)

[phlax]

actually - not - the configure_make rule ...

After configuration, GNU Make is called.

so it seems it is building stuff

[mathetake]

we definitely should prioritize this as until then we end up dealing with random broken builds like this over and over again

[phlax]

i tried reverting recent update to foreign_cc but hit the same issue - confused as to what changed

[phlax]

it would appear that the breaking change is the bazel update - reverting bazel to 6.5.0 fixes

[phlax]

same failure on 7.0.0 - so it appears to be some change in 7.x that is causing this

checking further - and aspects should not, at least in ordinary circumstances build nodes/deps - however if the required file/s (ie headers) are declared as outputs of the rule but not declared independently then bazel will ~correctly conclude that the rule/target has to be built in order to get to the headers - i think that is what is happening here

if that is correct then i guess the foreign_cc targets probably were always getting built - the breaking bazel update is most likely that now when building the env/context/inputs are not being passed through correctly