This is an archive of the discontinued LLVM Phabricator instance.

[bazel] Include builtin headers with clang-tidy
AbandonedPublic

Authored by jathu on Jul 27 2023, 7:11 AM.

Details

Reviewers
njames93
GMNGeoffrey
keith
aaronmondal
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

jathu created this revision.Jul 27 2023, 7:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
jathu requested review of this revision.Jul 27 2023, 7:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 7:11 AM
jathu added reviewers: aaronmondal, GMNGeoffrey, keith, Restricted Project.Jul 27 2023, 7:12 AM
jathu updated this revision to Diff 544764.Jul 27 2023, 7:18 AM

oops fixing bad merge

jathu updated this revision to Diff 544765.Jul 27 2023, 7:20 AM

undo bazelrc lld comment

jathu edited the summary of this revision. (Show Details)Jul 27 2023, 7:22 AM
aaronmondal requested changes to this revision.Jul 27 2023, 8:49 AM

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
17

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?

This revision now requires changes to proceed.Jul 27 2023, 8:49 AM
keith added inline comments.Jul 27 2023, 9:33 AM
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 marked 3 inline comments as done.Jul 27 2023, 6:14 PM
jathu added inline comments.
utils/bazel/llvm-project-overlay/clang-tools-extra/BUILD.bazel
17

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 updated this revision to Diff 544995.Jul 27 2023, 6:15 PM
jathu marked 2 inline comments as done.

rename gen_dir to resource_dir

jathu marked 3 inline comments as not done.Jul 27 2023, 6:16 PM
matt-sm added a subscriber: matt-sm.Aug 7 2023, 2:44 PM

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 added a comment.Aug 19 2023, 3:15 PM
This comment was removed by jathu.
jathu added a reviewer: aaronmondal.
jathu added a subscriber: aaronmondal.
jathu marked an inline comment as done.Aug 22 2023, 6:54 PM
jathu abandoned this revision.Aug 26 2023, 12:10 PM

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