[bazel] Include builtin headers with clang-tidy

[jathu]

Summary

Context: https://github.com/llvm/llvm-project/issues/63890

These builtin headers are required to exist alongside clang-tidy for it to work. Specifically, the tool looks for headers relatively in ../lib/clang/{major version}/include.

[jathu]

oops fixing bad merge

[jathu]

undo bazelrc lld comment

[aaronmondal]

I like this change. If something like this also works for the clang target and maybe a few others it would relieve downstream users from implementing logic around this resource path. (E.g. it could potentially reduce some of the hackiness in https://github.com/eomii/rules_ll/blob/37b5721a8083d36332eaca1f62fcc43b8aba7bd1/ll/args.bzl#L9-L47).

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

18

This seems to better fit into the clang directory in the overlay so that targets can consume //clang:builtin_headers.

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

372

I like this data attribute approach a lot. I wonder whether it's possible to do something similar for the clang binary target. This could reduce the complexity of using the clang binary target from the Bazel overlay.

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

1704

The name gen_dir could be misinterpreted as Bazel's gendir variable. If this contains everything in the location from -resource-dir, it might make sense to call it resource_dir.

utils/bazel/llvm-project-overlay/clang/defs.bzl

6

Would a string flag with a default value of "staging/include/" make sense here? How is this handled in the CMake build?

[keith]

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

8

I'm not sure if the LLVM repo wants this but there is a rule in the bazel-skylib for this https://github.com/bazelbuild/bazel-skylib/blob/main/docs/copy_file_doc.md#copy_file-allow_symlink

[jathu]

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

18

We can't do this because the tool looks for the path in its parent folder ../lib/clang/{major version}/include, where as your suggestion would move it within the clang folder

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

8

I don't think that does everything we want:

1. The source only takes a single file, so we'd have to write a macro to loop through the files and create the targets anyways
2. We are chopping the path after the staging/include/ path and preserving everything right of it (the subdirectories) — which we'd have to do still do if we use that rule. The only difference will be calling that rule instead of ctx.actions.symlink

utils/bazel/llvm-project-overlay/clang/defs.bzl

6

I don't know the motivation behind this path, it seems to have been introduced in the initial commit: https://github.com/llvm/llvm-project/commit/4aeb2e60df98a07e6a2a3cc16fc9ad1c1001d563. I should say, I'm not introducing anything new here — the logic is preserved, I'm only allowing the path to be static and shared

[jathu]

rename gen_dir to resource_dir

[jathu]

bump @aaronmondal @keith

[jathu]

bump for anyone in the @bazel_build group. cc @goncharov @gchatelet @hokein (based on git history for clang-tidy)

This will unblock clang-tidy builds and allow it to be used out of the box

[jathu]

This comment was removed by jathu.

[jathu]

bump @aaronmondal

[jathu]

Closing this diff to attempt to get another fresh pair of eyes on this change: D158942

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("//:vars.bzl", "LLVM_VERSION_MAJOR")
+load("//clang:defs.bzl", "BUILTIN_HEADERS_GEN_DIR")
+load("defs.bzl", "symlink")
+
+package(
+    default_visibility = ["//visibility:public"],
+    features = ["layering_check"],
+)
+
+licenses(["notice"])
+
+symlink(
+    name = "builtin_headers",
+    srcs = ["//clang:builtin_headers_gen"],
+    destination = "lib/clang/{0}/include".format(LLVM_VERSION_MAJOR),
+    partition = BUILTIN_HEADERS_GEN_DIR,
+)

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

deps = CHECKS + [
         "//clang:tooling",
         "//llvm:Support",
     ],
 )
 
 cc_binary(
     name = "clang-tidy",
     srcs = ["tool/ClangTidyToolMain.cpp"],
+    data = ["//clang-tools-extra:builtin_headers"],
     stamp = 0,
     deps = [":tool"],
 )
 
 native_binary(
     name = "run-clang-tidy",
     src = "tool/run-clang-tidy.py",
     out = "run-clang-tidy",
     data = [":clang-tidy"],
 )

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

+# 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//lib:paths.bzl", "paths")
+
+def _symlink_impl(ctx):
+    copied_files = []
+    for input_file in ctx.files.srcs:
+        (_, _, relative_filename) = input_file.path.rpartition(ctx.attr.partition)
+        output_file = ctx.actions.declare_file(paths.join(ctx.attr.destination, relative_filename))
+        ctx.actions.symlink(
+            target_file = input_file,
+            output = output_file,
+        )
+        copied_files.append(output_file)
+    return DefaultInfo(files = depset(copied_files))
+
+symlink = rule(
+    implementation = _symlink_impl,
+    attrs = {
+        "destination": attr.string(mandatory = True),
+        "partition": attr.string(mandatory = True),
+        "srcs": attr.label_list(allow_files = True),
+    },
+)

utils/bazel/llvm-project-overlay/clang/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")
-load("//:workspace_root.bzl", "workspace_root")
-load("//llvm:tblgen.bzl", "gentbl")
-load("//llvm:binary_alias.bzl", "binary_alias")
-load("//llvm:cc_plugin_library.bzl", "cc_plugin_library")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library")
+load("@rules_python//python:defs.bzl", "py_binary")
 load(
     "//:vars.bzl",
     "LLVM_VERSION",
     "LLVM_VERSION_MAJOR",
     "LLVM_VERSION_MINOR",
     "LLVM_VERSION_PATCH",
 )
+load("//:workspace_root.bzl", "workspace_root")
+load("//llvm:binary_alias.bzl", "binary_alias")
+load("//llvm:cc_plugin_library.bzl", "cc_plugin_library")
+load("//llvm:tblgen.bzl", "gentbl")
+load("defs.bzl", "BUILTIN_HEADERS_GEN_DIR")
 
 package(
     default_visibility = ["//visibility:public"],
     features = ["layering_check"],
 )
 
 licenses(["notice"])
 
) + [
     "lib/Headers/openmp_wrappers/cmath",
     "lib/Headers/openmp_wrappers/complex",
     "lib/Headers/openmp_wrappers/new",
 ]
 
 genrule(
     name = "builtin_headers_gen",
     srcs = builtin_headers,
-    outs = [hdr.replace("lib/Headers/", "staging/include/") for hdr in builtin_headers],
+    outs = [
+        header.replace("lib/Headers/", BUILTIN_HEADERS_GEN_DIR)
+        for header in builtin_headers
+    ],
     cmd = """
        for src in $(SRCS); do
-         relsrc=$${src#*"$(WORKSPACE_ROOT)"/clang/lib/Headers}
-         target=$(@D)/staging/include/$$relsrc
+         relsrc=$${{src#*"$(WORKSPACE_ROOT)"/clang/lib/Headers}}
+         target=$(@D)/{resource_dir}$$relsrc
          mkdir -p $$(dirname $$target)
          cp $$src $$target
-       done""",
+       done""".format(resource_dir = BUILTIN_HEADERS_GEN_DIR),
     output_to_bindir = 1,
     toolchains = [
         ":workspace_root",
     ],
 )
 
 cc_library(
     name = "frontend",

utils/bazel/llvm-project-overlay/clang/defs.bzl

+# 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
+
+BUILTIN_HEADERS_GEN_DIR = "staging/include/"