[Lldb-commits] [PATCH] D79219: [CMake] Simplify CMake handling for zlib

Mon May 11 15:07:56 PDT 2020

JDevlieghere added inline comments.

================
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"
----------------
phosek wrote:
> JDevlieghere wrote:
> > If I understand correctly, after running the configuration phase with LLVM_ENABLE_ZLIB to FORCE_ON, the cached value will be overwritten by ON? 
> Correct, we used `FORCE_ON` above when invoking `find_package` setting `REQUIRED` which makes the check fail if the library is missing. Recording this information is important for LLVM consumers because it'll be stored in `LLVMConfig.cmake` and AFAIU we don't want to propagate `FORCE_ON` there.
I get that. My worry is that if for whatever reason the library disappears (system upgrade, package removal, ...) this will silently disable ZLIB support because now LLVM_ENABLE_ZLIB just equals on. This might sound far fetched, but happens all the time with "internal installs". This is why I prefer the ON/OFF/Auto approach because it doesn't update the cache variable, but would set the internal variable to either ON or OFF.

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

https://reviews.llvm.org/D79219