[PATCH] D47560: [CMake] Use find_package to find zlib

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: llvm/cmake/config-ix.cmake:112
+      include_directories(${ZLIB_INCLUDE_DIR})
+    endif()
   endif()
----------------
phosek wrote:
> delcypher wrote:
> > If `LLVM_ENABLE_ZLIB` but `ZLIB_FOUND` is `FALSE` we should emit an error telling the user that Zlib couldn't be found and that should either tell CMake how to find it or set `LLVM_ENABLE_ZLIB` to `FALSE`. You also use the `REQUIRED` argument to `find_package()` as an alternative.
> I'm fine doing that, but it'd would be a different behavior from the current one where we silently ignore the missing library?
Yes that is different behaviour. I'm pretty convinced that the current behaviour is wrong.

In KLEE we use a slightly different pattern. We initialize the default option for enabling ZLIB depending on whether or not the library is available.

See https://github.com/klee/klee/blob/843e9be8fc10c6ffb30218c5a826aab192a31955/CMakeLists.txt#L344

The advantage of this approach is that if the system has the library we use it by default, but if the user explicitly sets the option to use zlib and its not available then we emit an error.

Given that it  appears the existing behaviour of LLVM right now is to silently ignore missing libraries, you don't have to fix this right now if you don't want to. This could be addressed in a separate patch. Although I'd personally like a warning at the very least instead of LLVM's build system silently ignoring the request to use zlib.


Repository:
  rL LLVM

https://reviews.llvm.org/D47560