[PATCH] D143320: [bazel] Rework zlib dependency

[Aaron Siddhartha Mondal via Phabricator via llvm-commits]

aaronmondal added inline comments.


================
Comment at: utils/bazel/WORKSPACE:38
 maybe(
     http_archive,
+    name = "llvm_zlib",
----------------
GMNGeoffrey wrote:
> 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`
It turned out that GitHub actually does guarantee sha stability, but only for release tag links. This *really* screwed all our workflows.

I'd love changing this (and all other http archives), but I think it should be handled in a different commit, since it may break CI environments without git.


================
Comment at: utils/bazel/third_party_build/zlib.BUILD:17-20
+config_setting(
+    name = "llvm_zlib_disabled",
+    flag_values = {":llvm_enable_zlib": "false"},
+)
----------------
GMNGeoffrey wrote:
> 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"
Yeah I think originally I did it like this so that it's clear that the `zlib.BUILD` file actually builds `zlib` source code by default, but looking at it again it indeed seems unnecessarily unintuitive 😆 Will change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143320