Skip to content

[bazel] Add llvm_zlib to system include path #121374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[bazel] Add llvm_zlib to system include path #121374

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions utils/bazel/third_party_build/zlib-ng.BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ cc_library(
],
"//conditions:default": [],
}),
includes = select({
":llvm_zlib_enabled": ["."],
"//conditions:default": [],
}),
# Clang includes zlib with angled instead of quoted includes, so we need
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# strip_include_prefix here.
strip_include_prefix = ".",
Expand Down
Loading