[PATCH] D156433: [bazel] Include builtin headers with clang-tidy

[Aaron Siddhartha Mondal via Phabricator via llvm-commits]

aaronmondal requested changes to this revision.
aaronmondal added a comment.
This revision now requires changes to proceed.

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



================
Comment at: utils/bazel/llvm-project-overlay/clang-tools-extra/BUILD.bazel:17
+symlink(
+    name = "builtin_headers",
+    srcs = ["//clang:builtin_headers_gen"],
----------------
This seems to better fit into the `clang` directory in the overlay so that targets can consume `//clang:builtin_headers`.


================
Comment at: utils/bazel/llvm-project-overlay/clang-tools-extra/clang-tidy/BUILD.bazel:372
     srcs = ["tool/ClangTidyToolMain.cpp"],
+    data = ["//clang-tools-extra:builtin_headers"],
     stamp = 0,
----------------
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.


================
Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:1704
          cp $$src $$target
-       done""",
+       done""".format(gen_dir = BUILTIN_HEADERS_GEN_DIR),
     output_to_bindir = 1,
----------------
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`.


================
Comment at: utils/bazel/llvm-project-overlay/clang/defs.bzl:5
+
+BUILTIN_HEADERS_GEN_DIR = "staging/include/"
----------------
Would a string flag with a default value of `"staging/include/"` make sense here? How is this handled in the CMake build?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156433/new/

https://reviews.llvm.org/D156433