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

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D79219#2029520 <https://reviews.llvm.org/D79219#2029520>, @JDevlieghere wrote:

> 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)


I'm fine either way, to me ON/OFF/AUTO and FORCE_ON/ON/OFF is just a different way to spell the same thing, my only concern would be breaking existing users who might be already relying on FORCE_ON.



================
Comment at: llvm/cmake/config-ix.cmake:506
 
-if (LLVM_ENABLE_ZLIB )
-  # Check if zlib is available in the system.
-  if ( NOT HAVE_ZLIB_H OR NOT HAVE_LIBZ )
-    set(LLVM_ENABLE_ZLIB 0)
+if(LLVM_ENABLE_ZLIB)
+  if(LLVM_ENABLE_ZLIB STREQUAL FORCE_ON)
----------------
smeenai wrote:
> How come you're moving this out of the MSan check block? Isn't that a behavior change?
This was done in the original change, I'm not quite sure what's the motivation, @compnerd do you know?


================
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:
> 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.


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

https://reviews.llvm.org/D79219