[PATCH] D79219: [CMake] Simplify CMake handling for zlib

Mon May 11 09:07:30 PDT 2020

JDevlieghere added a comment.

I'm in favor of this change. I'm not too happy with how this works in CMake, I've expressed similar concerns when the FORCE_ON approach was suggested in D71306 <https://reviews.llvm.org/D71306>.

I really like what we ended up with in LLDB. The TL;DR is that we have 3 values: ON, OFF and Auto. The later will resolve to either ON or OFF depending on whether the dependency was found. The "clever" part is that we use shadowing to avoid having another CMake variable without overwriting the cache.

Here's what it looks like in LLDB: https://github.com/llvm/llvm-project/blob/3df40007e6317955176dff0fa9e0b029dc4a11d1/lldb/cmake/modules/LLDBConfig.cmake#L26

Maybe we can consider something similar here? I'd be more than happy to hoist the corresponding CMake into llvm.

(edit: added myself as a reviewer so this shows up in my review queue)

================
Comment at: llvm/cmake/config-ix.cmake:514
+  if(ZLIB_FOUND)
+    set(LLVM_ENABLE_ZLIB "YES" CACHE STRING
+      "Use zlib for compression/decompression if available. Can be ON, OFF, or FORCE_ON"
----------------
If I understand correctly, after running the configuration phase with LLVM_ENABLE_ZLIB to FORCE_ON, the cached value will be overwritten by ON? 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79219