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

[Jathu Satkunarajah via Phabricator via llvm-commits]

jathu marked 3 inline comments as done.
jathu added inline comments.


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


================
Comment at: utils/bazel/llvm-project-overlay/clang-tools-extra/defs.bzl:7
+
+def _symlink_impl(ctx):
+    copied_files = []
----------------
keith wrote:
> 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
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`


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156433