Page MenuHomePhabricator

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

Authored by chandlerc on Oct 30 2021, 9:18 PM.

Details

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.

Diff Detail

Event Timeline

chandlerc created this revision.Oct 30 2021, 9:18 PM
chandlerc requested review of this revision.Oct 30 2021, 9:18 PM
rsmith accepted this revision.Nov 1 2021, 2:41 PM
rsmith added a subscriber: rsmith.

:( LGTM

This revision is now accepted and ready to land.Nov 1 2021, 2:41 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 5:31 PM

:-((((( 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

:-((((( 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.

:-((((( 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"

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?

SGTM

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

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.