This is an archive of the discontinued LLVM Phabricator instance.

[bazel] create a clang-tidy binary target
ClosedPublic

Authored by jathu on Feb 10 2023, 9:11 PM.

Details

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

Diff Detail

Event Timeline

jathu created this revision.Feb 10 2023, 9:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 9:11 PM
jathu requested review of this revision.Feb 10 2023, 9:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 9:11 PM
jathu added a reviewer: Restricted Project.Feb 10 2023, 9:13 PM

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

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 updated this revision to Diff 496695.EditedFeb 11 2023, 10:04 AM
jathu edited the summary of this revision. (Show Details)

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

aaronmondal requested changes to this revision.Feb 12 2023, 5:02 AM

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.

This revision now requires changes to proceed.Feb 12 2023, 5:02 AM
jathu updated this revision to Diff 496766.EditedFeb 12 2023, 9:18 AM

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 accepted this revision as: aaronmondal.Feb 12 2023, 10:23 AM
aaronmondal added subscribers: GMNGeoffrey, chapuni.

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

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

aaronmondal added inline comments.Feb 12 2023, 11:51 AM
utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel
6–9

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 updated this revision to Diff 496781.Feb 12 2023, 12:11 PM

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 accepted this revision.Feb 12 2023, 12:59 PM

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.

This revision is now accepted and ready to land.Feb 12 2023, 12:59 PM
jathu added a comment.Feb 12 2023, 1:08 PM

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

This revision was automatically updated to reflect the committed changes.
GMNGeoffrey added inline comments.Feb 16 2023, 2:41 PM
utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel
19

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

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

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

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 added inline comments.Feb 16 2023, 3:43 PM
utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel
19

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

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 added inline comments.Feb 16 2023, 3:53 PM
utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel
19

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.