[bazel] Re-introduce `copts` hacks for lib/AST includes.

[chandlerc]

Summary

Sadly, these are necessary AFAICT. There is a file lib/AST/CXXABI.h.
On case insensitive file systems like macOS this will collide with
cxxabi.h on the system if we use the includes trick to allow
file-relative #include of generated files.

I've tested this on both Linux and Windows to make sure it remains
reasonably portable.

[rsmith]

:( LGTM

[GMNGeoffrey]

:-((((( Did you try the separate library with strip_include_prefix?

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

711

This breaks anyone who calls the repository something other than "llvm-project". I think we have real users for whom that is the case

[chandlerc]

In D112883#3101645, @GMNGeoffrey wrote:

:-((((( Did you try the separate library with strip_include_prefix?

I can add a separate library to do that.

It would still expose all consumers to Opcodes.inc instead of only exposing *this* library to it. If we can afford to have the dependency on the repository name, I feel like the current solution is actually a cleaner workaround. But if we genuinely cannot, then we could revisit this.

However, I thought with Bazel having a consistent repo name was important to allow cross-repo dependencies to reliably resolve, and so maybe this isn't that bad of a thing to rely on?

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

711

This isn't the only place where we assume that in Clang's build at least. =[

Is this breaking actual users? I wouldn't expect it to break unless they depend on this target.

[GMNGeoffrey]

In D112883#3101665, @chandlerc wrote:

In D112883#3101645, @GMNGeoffrey wrote:

:-((((( Did you try the separate library with strip_include_prefix?

I can add a separate library to do that.

It would still expose all consumers to Opcodes.inc instead of only exposing *this* library to it. If we can afford to have the dependency on the repository name, I feel like the current solution is actually a cleaner workaround. But if we genuinely cannot, then we could revisit this.

Yeah there's really no great way. Bazel has helpfully classified https://github.com/bazelbuild/bazel/issues/13803 as P4.

However, I thought with Bazel having a consistent repo name was important to allow cross-repo dependencies to reliably resolve, and so maybe this isn't that bad of a thing to rely on?

I think cross-repo dependencies just tend to only work by chance. I haven't seen any specific guidance for how to name repository, so I don't think it can really be relied upon. Bazel has functionality in local_repository specifically to remap repository names (https://docs.bazel.build/versions/main/be/workspace.html#local_repository.repo_mapping), but of course it's only available in local_repository and there's no way to compose it.

Is this breaking actual users? I wouldn't expect it to break unless they depend on this target.

I don't know :-) I just know that people have sent me patches to remove the repository name dependence. I think https://github.com/llvm/llvm-project/blob/d1fdd745d510f40d8741d44ce39f5ae24ee7f91a/utils/bazel/llvm-project-overlay/clang/BUILD.bazel#L1460 is the only previous instance in clang. That gets used only in "frontend". I'm not sure if there are people using "ast" but not "frontend"

[chandlerc]

In D112883#3101685, @GMNGeoffrey wrote:

Is this breaking actual users? I wouldn't expect it to break unless they depend on this target.

I don't know :-) I just know that people have sent me patches to remove the repository name dependence. I think https://github.com/llvm/llvm-project/blob/d1fdd745d510f40d8741d44ce39f5ae24ee7f91a/utils/bazel/llvm-project-overlay/clang/BUILD.bazel#L1460 is the only previous instance in clang. That gets used only in "frontend". I'm not sure if there are people using "ast" but not "frontend"

I think users of ast that don't use frontend would be *extremely* rare. I'd like to see if this works for folks for now?

The other approach we could explore is removing the reliance on file-relative inclusion of generated code completely which I think is the real sustainable answer here. CMake only causes this to work for pretty questionable reasons as-is. But it's a lot of work, and so I'd like to understand if we have to before we do.

Does that seem reasonable to you?

[GMNGeoffrey]

SGTM

If you're coming here because it breaks your build please let us know :-)

[GMNGeoffrey]

In D112883#3101685, @GMNGeoffrey wrote:

In D112883#3101665, @chandlerc wrote:

However, I thought with Bazel having a consistent repo name was important to allow cross-repo dependencies to reliably resolve, and so maybe this isn't that bad of a thing to rely on?

I think cross-repo dependencies just tend to only work by chance. I haven't seen any specific guidance for how to name repository, so I don't think it can really be relied upon. Bazel has functionality in local_repository specifically to remap repository names (https://docs.bazel.build/versions/main/be/workspace.html#local_repository.repo_mapping), but of course it's only available in local_repository and there's no way to compose it.

I recently learned that apparently repo_mapping works for *all* repository rules. That's just not documented anywhere: https://github.com/bazelbuild/bazel/issues/10410. So yeah relying on repository naming is for sure a bug. Of course, so are all the other options.

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

 
 cc_library(
     name = "ast",
     srcs = glob([
         "lib/AST/*.cpp",
         "lib/AST/*.h",
         "lib/AST/Interp/*.cpp",
         "lib/AST/Interp/*.h",
-    ]),
+    ]) + [
+        "lib/AST/AttrDocTable.inc",
+        "lib/AST/Interp/Opcodes.inc",
+    ],
     hdrs = glob([
         "include/clang/AST/*.h",
     ]),
-    includes = [
-        "include",
-        "lib/AST",
-        "lib/AST/Interp",
+    copts = [
+        # FIXME: This is necessary to allow "file relative" include paths from
+        # non-generated `srcs` to find generated `srcs` above. Bazel should
+        # either make this work automatically by creating a unified tree of
+        # `srcs` or at least provide a `local_includes` that has the path
+        # translation logic of `includes` but is only used locally (similar to
+        # `local_defines` vs. `defines`). Until one of those lands, this is the
+        # least bad approach. Using `includes` is *specifically* problematic for
+        # this library because it contains files that collide easily with system
+        # headers such as `CXXABI.h`.
+        "-I$(GENDIR)/external/llvm-project/clang/lib/AST",
+        "-I$(GENDIR)/external/llvm-project/clang/lib/AST/Interp",
     ],
     textual_hdrs = [
         "include/clang/AST/AttrImpl.inc",
         "include/clang/AST/AttrNodeTraverse.inc",
         "include/clang/AST/Attrs.inc",
         "include/clang/AST/AttrTextNodeDump.inc",
         "include/clang/AST/AttrVisitor.inc",
         "include/clang/AST/CommentCommandInfo.inc",
         "include/clang/AST/CommentCommandList.inc",
         "include/clang/AST/CommentHTMLNamedCharacterReferences.inc",
         "include/clang/AST/CommentHTMLTags.inc",
         "include/clang/AST/CommentHTMLTagsProperties.inc",
         "include/clang/AST/CommentNodes.inc",
         "include/clang/AST/DeclNodes.inc",
         "include/clang/AST/StmtDataCollectors.inc",
         "include/clang/AST/StmtNodes.inc",
-        "lib/AST/AttrDocTable.inc",
-        "lib/AST/Interp/Opcodes.inc",
     ] + glob([
         "include/clang/AST/*.def",
     ]),
     deps = [
         ":ast_attr_gen",
         ":ast_comment_command_info_gen",
         ":ast_comment_html_named_character_references_gen",
         ":ast_comment_html_tags_gen",