[PATCH] D143320: [bazel] Rework zlib dependency

[Aaron Siddhartha Mondal via Phabricator via llvm-commits]

aaronmondal added a comment.

@GMNGeoffrey I originally had a version with a string_flag, but abandoned that because I couldn't solve the following issues:

- 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?

- How should the difference to the CMake build be handled? The current diff mirrors the behavior of `LLVM_ENABLE_ZLIB` and the `select` logic is reasonably friendly to migrate to e.g. the CMake-to-Bazel scripts. The most precise option would be to use two config flags (enable/disable, hermetic/non-hermetic), but that seemed like an awful lot of spaghetti code only to get a single use case working. Since we can't "export" an `-lz` copt, We'd have to add selects to the LLVM Build file, which seems *very* hacky.

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.

If you know the systems that use the "system" strategy, you'll also be able to use a module `*_override`with bzlmod in the future. E.g. you could use a custom Bazel registry with a module pointing to specific local paths.

If these migration options and workarounds are not satisfactory I can bring the `string_flag` back, but please let's only do this if the "system" functionality is actually important enough to justify the additional complexity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143320