[bazel] Add llvm_zlib to system include path #121374
Conversation
LLVM codebase expects to find zlib using (angle brackets) `<zlib.h>`. This means llvm_zlib needs to be exposed on the system include path (`-isystem` not `-iquote`). Tell bazel to do that.
|
Reviewers: I don't have merge permissions - so please mash the merge button yourself when you feel appropriate. |
| includes = select({ | ||
| ":llvm_zlib_enabled": ["."], | ||
| "//conditions:default": [], | ||
| }), | ||
| # Clang includes zlib with angled instead of quoted includes, so we need |
There was a problem hiding this comment.
This comment suggests angled bracket includes should already be working, so I'm not sure why an additional change is needed. Looks like @aaronmondal set this up a long time ago; do you know what's going on here?
There was a problem hiding this comment.
Hmm IIIRC entire target should only be instantiated if "llvm_zlib_enabled" is set. Maybe some downstream target is missing an explicit dep on zlib? The strip_include_prefix should already be doing the include path shifting for the angled include. But this code is old and Bazel changed a bunch of internals around rule_cc so that might also be related here.
For reference, this was originally upstreamed from here: https://github.com/eomii/bazel-eomii-registry/blob/main/modules/llvm-project-overlay/17-init-bcr.3/patches/llvm_use_zlib-ng.patch
I have a newer version here which I originally wrote against llvm which works without custom includes or the strip prefix here: https://github.com/bazelbuild/bazel-central-registry/pull/1539/files. BCR CI is too slow moving to fix this for windows, but I can probably bump it in llvm and fix the windows support here.
There was a problem hiding this comment.
@anguslees After looking into this a bit more I believe some misconfiguration in your Bazel setup causes this issue.
The include attribute would add an additional path to all targets that depend (potentially transitively) on zlib. The current strip_include_prefix doesn't do that and instead modifies the generated -isystem path from the headers attribute which should already be the correct approach.
I've noticed that there is a comment https://bazel.build/reference/be/c-cpp#cc_library.strip_include_prefix mentioning that strip_include_prefix is only "legal" under "third_party". It could be possible that Bazel ignores the strip_include_prefix for some reason, but I'd expect there to be some sort of error around this (or at least a warning).
Could you let me know what your bazel version is and how you're importing llvm? Also, which version of rules_cc and what crosscompilation setup are you using? For instance, if you're using a workspace setup the rules_cc version from llvm (currently 0.0.17 which is the latest) might be overridden by some other version and causing issues.
LLVM codebase expects to find zlib using (angle brackets)
<zlib.h>.This means llvm_zlib needs to be exposed on the system include path
(
-isystemnot-iquote). Tell bazel to do that.