Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable layering_check in lint_clang_tidy_aspect evaluation #472

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Databean
Copy link

layering_check is a toolchain feature that can track whether #include dependencies are properly represented in the bazel dependency graph. It is only supported with clang as the compiler. Turning on this feature affects the internals of the c++ build rules, breaking an interaction with lint_clang_tidy_aspect. This leads to a build error looking like:

ERROR: .../BUILD.bazel:16:10: in //:linters.bzl%clang_tidy aspect on cc_binary rule //:executable:
Traceback (most recent call last):
        File ".../external/aspect_rules_lint+/lint/clang_tidy.bzl", line 391, column 26, in _clang_tidy_aspect_impl
                clang_tidy_action(ctx, compilation_context, ctx.executable, files_to_lint, outputs.human.out, outputs.human.exit_code)
        File ".../external/aspect_rules_lint+/lint/clang_tidy.bzl", line 310, column 45, in clang_tidy_action
                compiler_args.add_all(_get_compiler_args(ctx, compilation_context, srcs))
        File ".../external/aspect_rules_lint+/lint/clang_tidy.bzl", line 263, column 54, in _get_compiler_args
                args.extend(_safe_flags(ctx, _toolchain_flags(ctx, user_flags, ACTION_NAMES.cpp_compile) + rule_flags) + ["-xc++"])
        File ".../external/aspect_rules_lint+/lint/clang_tidy.bzl", line 86, column 58, in _toolchain_flags
                flags = cc_common.get_memory_inefficient_command_line(
        File "/virtual_builtins_bzl/common/cc/cc_common.bzl", line 205, column 66, in _get_memory_inefficient_command_line
Error in get_memory_inefficient_command_line: Invalid toolchain configuration: Cannot find variable named 'module_name'.
ERROR: Analysis of target '//:executable' failed
ERROR: Analysis of target '//:executable_tidy' failed; build aborted

This commit disables the layering_check toolchain feature while running lint_clang_tidy_aspect without affecting the behavior of non-aspect builds, making it possible to use both in the same project.

References:

Small reproduction case:

BUILD.bazel:

load("@bazel_skylib//rules:native_binary.bzl", "native_binary")
load("//:linters.bzl", "clang_tidy_test")

native_binary(
    name = "clang_tidy",
    src = "@llvm_toolchain//:clang-tidy",
    out = "clang_tidy",
)

cc_library(
    name = "library",
    hdrs = ["direct.h"],
    srcs = ["indirect.h"],
)

cc_binary(
    name = "executable",
    srcs = ["executable.cc"],
    deps = [":library"],
)

clang_tidy_test(
    name = "executable_tidy",
    srcs = [":executable"],
)

exports_files([".clang-tidy"])

direct.h:


inline void DirectDependency() {
  std::cerr << "Direct\n";
}

executable.cc:


int main() {
  std::cerr << "hello\n";
  DirectDependency();
  IndirectDependency();
}

indirect.h:


inline void IndirectDependency() { std::cerr << "Indirect\n"; }

linters.bzl:

load("@aspect_rules_lint//lint:clang_tidy.bzl", "lint_clang_tidy_aspect")
load("@aspect_rules_lint//lint:lint_test.bzl", "lint_test")

clang_tidy = lint_clang_tidy_aspect(
    binary = "@@//:clang_tidy",
    configs = ["@@//:.clang-tidy"],
)

clang_tidy_test = lint_test(aspect = clang_tidy)

MODULE.bazel:

bazel_dep(name = "aspect_rules_lint", version = "1.0.8")
bazel_dep(name = "bazel_skylib", version = "1.7.1")
bazel_dep(name = "toolchains_llvm", version = "1.2.0")

llvm = use_extension("@toolchains_llvm//toolchain/extensions:llvm.bzl", "llvm")
llvm.toolchain(
   llvm_version = "18.1.8",
)
use_repo(llvm, "llvm_toolchain")
register_toolchains("@llvm_toolchain//:all")

REPO.bazel:

repo(features = ["layering_check", "parse_headers"])

Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: no
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes

"Adds workaround for incompatibility between lint_clang_tidy_aspect rule and layering_check toolchain feature"

Test plan

  • Manual testing; please provide instructions so we can reproduce:

[`layering_check`](https://bazel.build/docs/bazel-and-cpp#toolchain-features)
is a toolchain feature that can track whether `#include` dependencies
are properly represented in the bazel dependency graph. It is only
supported with `clang` as the compiler.  Turning on this feature affects
the internals of the c++ build rules, breaking an interaction with
`lint_clang_tidy_aspect`. This leads to a build error looking like:

```
ERROR: .../BUILD.bazel:16:10: in //:linters.bzl%clang_tidy aspect on cc_binary rule //:executable:
Traceback (most recent call last):
        File ".../external/aspect_rules_lint+/lint/clang_tidy.bzl", line 391, column 26, in _clang_tidy_aspect_impl
                clang_tidy_action(ctx, compilation_context, ctx.executable, files_to_lint, outputs.human.out, outputs.human.exit_code)
        File ".../external/aspect_rules_lint+/lint/clang_tidy.bzl", line 310, column 45, in clang_tidy_action
                compiler_args.add_all(_get_compiler_args(ctx, compilation_context, srcs))
        File ".../external/aspect_rules_lint+/lint/clang_tidy.bzl", line 263, column 54, in _get_compiler_args
                args.extend(_safe_flags(ctx, _toolchain_flags(ctx, user_flags, ACTION_NAMES.cpp_compile) + rule_flags) + ["-xc++"])
        File ".../external/aspect_rules_lint+/lint/clang_tidy.bzl", line 86, column 58, in _toolchain_flags
                flags = cc_common.get_memory_inefficient_command_line(
        File "/virtual_builtins_bzl/common/cc/cc_common.bzl", line 205, column 66, in _get_memory_inefficient_command_line
Error in get_memory_inefficient_command_line: Invalid toolchain configuration: Cannot find variable named 'module_name'.
ERROR: Analysis of target '//:executable' failed
ERROR: Analysis of target '//:executable_tidy' failed; build aborted
```

This commit disables the `layering_check` toolchain feature while
running `lint_clang_tidy_aspect` without affecting the behavior of
non-aspect builds, making it possible to use both in the same project.

References:

- bazel-contrib/rules_foreign_cc#630
- grailbio/bazel-compilation-database#101

Small reproduction case:

BUILD.bazel:
```
load("@bazel_skylib//rules:native_binary.bzl", "native_binary")
load("//:linters.bzl", "clang_tidy_test")

native_binary(
    name = "clang_tidy",
    src = "@llvm_toolchain//:clang-tidy",
    out = "clang_tidy",
)

cc_library(
    name = "library",
    hdrs = ["direct.h"],
    srcs = ["indirect.h"],
)

cc_binary(
    name = "executable",
    srcs = ["executable.cc"],
    deps = [":library"],
)

clang_tidy_test(
    name = "executable_tidy",
    srcs = [":executable"],
)

exports_files([".clang-tidy"])
```

direct.h:
```

inline void DirectDependency() {
  std::cerr << "Direct\n";
}
```

executable.cc:
```

int main() {
  std::cerr << "hello\n";
  DirectDependency();
  IndirectDependency();
}
```

indirect.h:
```

inline void IndirectDependency() { std::cerr << "Indirect\n"; }
```

linters.bzl:
```
load("@aspect_rules_lint//lint:clang_tidy.bzl", "lint_clang_tidy_aspect")
load("@aspect_rules_lint//lint:lint_test.bzl", "lint_test")

clang_tidy = lint_clang_tidy_aspect(
    binary = "@@//:clang_tidy",
    configs = ["@@//:.clang-tidy"],
)

clang_tidy_test = lint_test(aspect = clang_tidy)

```

MODULE.bazel:
```
bazel_dep(name = "aspect_rules_lint", version = "1.0.8")
bazel_dep(name = "bazel_skylib", version = "1.7.1")
bazel_dep(name = "toolchains_llvm", version = "1.2.0")

llvm = use_extension("@toolchains_llvm//toolchain/extensions:llvm.bzl", "llvm")
llvm.toolchain(
   llvm_version = "18.1.8",
)
use_repo(llvm, "llvm_toolchain")
register_toolchains("@llvm_toolchain//:all")
```

REPO.bazel:
```
repo(features = ["layering_check", "parse_headers"])
```
@CLAassistant
Copy link

CLAassistant commented Jan 31, 2025

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants