Differential D143804
[bazel] create a clang-tidy binary target Authored by jathu on Feb 10 2023, 9:11 PM.
Details
Create a binary target for clang-tidy. Tested by running: $ bazel build --config=generic_clang @llvm-project//clang-tools-extra/... $ bazel test --config=generic_clang @llvm-project//clang-tools-extra/... $ bazel run --config=generic_clang @llvm-project//clang-tools-extra/clang-tidy -- --help
Diff Detail
Event TimelineComment Actions Very cool! I actually have a finer grained implementation of this if this would be interesting: https://github.com/eomii/rules_ll/blob/main/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel Comment Actions I'm a big fan of the expand_template for that questionable one-define header. Very clean 👍 I think it makes much sense to have a finer grained buildfile: If the coarse grained target fails to build, Bazel will rebuild ALL sources of this. Clang Tidy is a large target. Caching would really suffer if we have to rebuild everything if someone changes a single check ever so slightly. Also, I'd like to propose adding the multithreaded clang-tidy-runner: load("@bazel_skylib//rules:native_binary.bzl", "native_binary")
...
native_binary(
name = "run-clang-tidy",
src = "tool/run-clang-tidy.py",
out = "run-clang-tidy",
data = [":clang-tidy"],
visibility = ["//visibility:public"],
)as well, since running clang tidy single threaded will be rather limited in application. Comment Actions Addressing comments: split to individual targets, enable static analyser, and create run-clang-tidy target Comment Actions Thanks for addressing. Also, really appreciate the tests 😊 One more thing we'll probably want is configurability of CLANG_TIDY_ENABLE_STATIC_ANALYZER, i.e. something along these lines: load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")
...
bool_flag(
name = "clang_tidy_enable_static_analyzer",
build_setting_default = True,
)
config_setting(
name = "clang_tidy_static_analyzer_enabled",
flag_values = {":clang_tidy_enable_static_analyzer": True},
)
expand_template(
name = "config",
...
substitutions = select({
"clang_tidy_static_analyzer_enabled": {
"#cmakedefine01 CLANG_TIDY_ENABLE_STATIC_ANALYZER": "#define CLANG_TIDY_ENABLE_STATIC_ANALYZER 1",
},
"//conditions:default": {
"#cmakedefine01 CLANG_TIDY_ENABLE_STATIC_ANALYZER": "#define CLANG_TIDY_ENABLE_STATIC_ANALYZER 0",
}),
},
...
)The CMake build file at https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/CMakeLists.txt conditionally uses the static analyzer flag to disable linking of clangStaticAnalyzerCore and clangStaticAnalyzerFrontend, and to disable the mpi module. We could use a select like above to mirror that behavior in Bazel. This way someone who disables the static analyzer doesn't need to build unnecessary targets. Comment Actions Addressing comment: allow static analyser config to be controlled via bazel. Tested by running: $ bazel build --config=generic_clang @llvm-project//clang-tools-extra/clang-tidy:config && cat bazel-bin/external/llvm-project/clang-tools-extra/clang-tidy/clang-tidy-config.h $ bazel build --@llvm-project//clang-tools-extra/clang-tidy:enable_static_analyzer=false --config=generic_clang @llvm-project//clang-tools-extra/clang-tidy:config && cat bazel-bin/external/llvm-project/clang-tools-extra/clang-tidy/clang-tidy-config.h $ bazel build --@llvm-project//clang-tools-extra/clang-tidy:enable_static_analyzer=true --config=generic_clang @llvm-project//clang-tools-extra/clang-tidy:config && cat bazel-bin/external/llvm-project/clang-tools-extra/clang-tidy/clang-tidy-config.h Comment Actions Looking at the include order in BUILD.bazel I think you need to run buildifier over this once, but other than that I'm very happy with this state of the diff. Yielding to @GMNGeoffrey and/or @chapuni Comment Actions @aaronmondal thanks for the reviews! These files were run through buildifier — any specific file/line I should look at?
Comment Actions Run buildifier with lint fix and all warnings: $ find llvm-project-overlay/clang-tools-extra -name '*.bzl' -o -name '*.bazel' | xargs buildifier --lint=fix --warnings=allComment Actions Cool, thanks a lot for your efforts! Since this is a larger change, let's wait a few days to see if anyone else would like to add anything to the revision. I'll also test this with rules_ll to see whether any issues would arise from using it with bzlmod (sometimes code that works for WORKSPACE breaks with bzlmod). If you'd like me to commit on your behalf let me know your email address so that I can add your attribution. Make sure that this email address is registered on your GitHub profile. Otherwise GitHub won't link your GithHub profile to the commit. Comment Actions @aaronmondal sounds good! Feel free to commit on my behalf — my email/username is in the git log (avoiding typing it here incase of spam)
| |||||||||||||||||||||||||||||||||||||||||||||||||||
These includes look out of order.
Ah I remember having issues like this as well where buildifier wouldn't apply some warnings/fixes like sorting.
It's probably because some warnings/fixes are only raises/applied if -lint is set to warn/fix. Confusingly, the default for that flag is off. Maybe -warnings=all needs to be set as well.