[bazel] create a clang-tidy binary target

[jathu]

Summary

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

[aaronmondal]

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

[aaronmondal]

I'm a big fan of the expand_template for that questionable one-define header. Very clean 👍
I do think it'd be better to set CLANG_STATIC_ANALYZER 1 though as we build the static analyzer targets anyways.

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.

[jathu]

Addressing comments: split to individual targets, enable static analyser, and create run-clang-tidy target

[aaronmondal]

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.

[jathu]

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

[aaronmondal]

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

[jathu]

@aaronmondal thanks for the reviews! These files were run through buildifier — any specific file/line I should look at?

[aaronmondal]

utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel

5–8 ↗

(On Diff #496766)

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.

[jathu]

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=all

[aaronmondal]

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.

[jathu]

@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)

[GMNGeoffrey]

utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel

19 ↗

(On Diff #497641)

Note that we can also add a flag alias in the project bazelrc so that in-repo builds have a short way to spell this.

67 ↗

(On Diff #497641)

I think this unnecessary, right? That's the default. The headers should already be available at this path

utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/defs.bzl

19 ↗

(On Diff #497641)

This saves a few keystrokes, but it adds a weird level of indirection here and a bunch of implicit behavior. Unfortunately, a lot of the literature on Bazel best practices I can only find internally, but generally it is recommend that you prioritize being DAMP rather than DRY. As a concerete example, does every one of these rules really depend on every one of these deps or is this adding unnecessary dependencies? I would actually recommend getting rid of this macro entirely and just expanding out its contents. In normal cases I'd even avoid globs for source files (outside of globbing tests), but given that Bazel is an unsupported build system for LLVM, we've compromised on using globs to reduce the frequency with which the BUILD.bazel files need to be updated.

See also, https://bazel.build/rules/bzl-style#macros

24 ↗

(On Diff #497641)

Oof does it really have to be alwayslink? alwayslink creates a whole world of pain where things can fail do to changes far away from them in the build graph. What neds this behavior?

[aaronmondal]

utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel

19 ↗

(On Diff #497641)

Yes, although I think we should try to not depend on the .bazelrc too much. If someone imports the project, the bazelrc is ignored (btw this also can cause builds to break already since we set the C++ standard in the .bazelrc and a naive workspace or module will try to build LLVM with an older, incompatible C++ standard).

We probably need "standardized" way to handle these config flags in general. If we move the entire LLVM config to build settings we'll have to handle like 50 config options. Improving the platforms may reduce that complexity a bit, but we'd probably still end up with quite a few manually tunable knobs.

67 ↗

(On Diff #497641)

I believe this case is different since we are handling a generated header. I remember that Bazel would raise an error if this was pointing to the workspace root, but I'm not sure whether the behavior has changed (or whether it is different for a non-root package).

[GMNGeoffrey]

utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel

19 ↗

(On Diff #497641)

Agreed we wouldn't want to *rely* on flag aliases, but it's also an advantage because it means we can choose an aliases that makes sense in the context of llvm, but would maybe be ambiguous in other projects. The alias is just a convenience, so I don't think that adding it is relying on it.

I never really found anyone who claimed that they could set up a Bazel toolchain though, which I think is the "proper" way to do this rather than the bazelrc configs. If someone is trying to build LLVM with an unsupported C++ version version though, that seems like on them? They need to have configured their C++ toolchain some way for the project they have that is presumably depending on LLVM, so will have chosen how to do that. We wouldn't want to mandate that LLVM has to be built with the One True Toolchain. It would be nice if the error message were friendlier though: just a quick failure saying "LLVM requires C++17" (are we on 17 as the minimum yet?). Not sure how to do that with Bazel.

utils/bazel/llvm-project-overlay/clang-tools-extra/BUILD.bazel

+# This file is licensed under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+load("@bazel_skylib//rules:expand_template.bzl", "expand_template")
+
+licenses(["notice"])
+
+expand_template(
+    name = "clang-tidy-config",
+    out = "include/clang-tidy-config.h",
+    substitutions = {
+        "#cmakedefine01 CLANG_TIDY_ENABLE_STATIC_ANALYZER": "#define CLANG_TIDY_ENABLE_STATIC_ANALYZER 0",
+    },
+    template = "clang-tidy/clang-tidy-config.h.cmake",
+)
+
+cc_binary(
+    name = "clang-tidy-confusable-table-builder",
+    srcs = ["clang-tidy/misc/ConfusableTable/BuildConfusableTable.cpp"],
+    deps = ["//clang:support"],
+)
+
+genrule(
+    name = "clang-tidy-confusables",
+    srcs = ["clang-tidy/misc/ConfusableTable/confusables.txt"],
+    outs = ["include/Confusables.inc"],
+    cmd = "$(location :clang-tidy-confusable-table-builder) $(SRCS) $(OUTS)",
+    exec_tools = [":clang-tidy-confusable-table-builder"],
+)
+
+cc_binary(
+    name = "clang-tidy",
+    srcs = glob(
+        [
+            "clang-tidy/**/*.cpp",
+            "clang-tidy/**/*.h",
+        ],
+        exclude = ["clang-tidy/misc/ConfusableTable/**/*"],
+    ) + [
+        ":clang-tidy-config",
+        ":clang-tidy-confusables",
+    ],
+    includes = ["include"],
+    stamp = 0,
+    visibility = ["//visibility:public"],
+    deps = [
+        "//clang:analysis",
+        "//clang:ast",
+        "//clang:ast_matchers",
+        "//clang:basic",
+        "//clang:format",
+        "//clang:frontend",
+        "//clang:frontend_rewrite",
+        "//clang:lex",
+        "//clang:rewrite",
+        "//clang:sema",
+        "//clang:serialization",
+        "//clang:static_analyzer_checkers",
+        "//clang:support",
+        "//clang:tooling",
+        "//clang:tooling_core",
+        "//clang:transformer",
+    ],
+)