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

Tue May 12 00:30:16 PDT 2020

labath 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"
----------------
JDevlieghere wrote:
> 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.
I agree with Jonas, though I don't think the actual values (FORCE_ON/ON vs. ON/Auto) really matter here. The important part is not overwriting the cache variables specified by the user, as that can create various problems with reconfigurations (like the zlib becoming unavailable example).

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

https://reviews.llvm.org/D79219