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

Mon Jun 29 15:18:26 PDT 2020

phosek marked an inline comment as done.
phosek 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"
----------------
labath wrote:
> 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).
I've updated the change to shadow the variable as suggested by Jonas.

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

https://reviews.llvm.org/D79219