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

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/cmake/config-ix.cmake:112
+      include_directories(${ZLIB_INCLUDE_DIR})
+    endif()
   endif()
----------------
delcypher wrote:
> 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.
I agree that the approach you're following in KLEE is better, but in that case we'll probably have to change all the code to check `LLVM_ENABLE_ZLIB` rather than `HAVE_LIBZ` because even if `HAVE_LIBZ` is set to `TRUE` and we use it to se the default value of `LLVM_ENABLE_ZLIB`, user may still override that and set `LLVM_ENABLE_ZLIB` to `FALSE`. Does that sound fine to you?


Repository:
  rL LLVM

https://reviews.llvm.org/D47560