[PATCH] D143320: [bazel] Rework zlib dependency

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey added a comment.

Aside: this patch appears to be missing a bunch of context in the diff. Could you try reuploading or something?

Good point about a dependent repo being able to override `llvm_zlib` if they want. Let's make sure that's actually ergonomic, but I think that should be sufficient to support the `system` case.

> Where is the system zlib? Some systems use /lib, some use /usr/lib, some use their 64 bit counterparts. Some use completely different layouts, e.g. /nix/store/<some_config_hash>-zlib-<version>/lib and /nix/store/<some_config_hash>-zlib-<version>-dev/include for zlib on NixOS. How could I ensure that the system is flexible enough for something like that without adding significant amounts of logic?

You'd add `-lz` as was here previously and let the system handle it. Definitely don't want to reinvent system package search here :-)

> Would patching WORKSPACE in the downstream project be a reasonable approach to get this kind of functionality? The proposed diff makes it very simple to patch the WORKSPACE and zlib.BUILD with local_repository logic.

Dependencies don't use the WORKSPACE file from the project they depend on. They can invoke Bazel macros or repo rules that it defines. It would be good to check that this actually makes that ergonomic. https://github.com/llvm/llvm-project/blob/main/utils/bazel/examples/http_archive/WORKSPACE and https://github.com/llvm/llvm-project/blob/main/utils/bazel/examples/submodule/WORKSPACE are examples of how to use the repository. Currently, it's easy to get things up and running with a couple of macros. I think this would break it and zlib would now need to be explicitly configured in the downstream WORKSPACE, repeating all the logic from LLVM's workspace. Can you factor the configuration into a macro so that we can preserve the easy configuration? I propose that we should offer the following macros: `llvm_configure`, the main repo only as we have it today; for each external dep, `llvm_configure_external_<dep_name>`; `llvm_configure_external_support_deps`, pulls in all of the deps as external repos initialized with `maybe`; `llvm_disable_external_support_deps`, initializes them all as empty with `maybe` and makes sure Bazel analysis doesn't get upset because of missing repos. The use of `maybe` here means that the downstream project can do its own configuration of select deps and then use a catch-all for the rest. If they want to be judicious about taking on dependencies, they can explicitly enable each one they want, but use the macros provided.



================
Comment at: utils/bazel/WORKSPACE:38
 maybe(
     http_archive,
+    name = "llvm_zlib",
----------------
Somewhat orthogonal to your change, but last week's GitHub sha256 blowup reminded me that `http_archive` is horrible and we should probably switch to using `git_repository`


================
Comment at: utils/bazel/third_party_build/zlib.BUILD:17-20
+config_setting(
+    name = "llvm_zlib_disabled",
+    flag_values = {":llvm_enable_zlib": "false"},
+)
----------------
Can we make this a positive option? It seems a bit confusing to have it negated like this. I think this works even if true is the default?

```
config_setting(
  name = "llvm_zlib_enabled",
  flag_values = {":llvm_enable_zlib": "true"},
)
```

And then the selects would be "if zlib is enabled add this thing, otherwise add nothing" rather than "if zlib is not enabled then don't add anything, otherwise add this thing"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143320/new/

https://reviews.llvm.org/D143320