[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:
> > 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?
Sounds good to me. You may have to refactor the code a bit more because it looks like the declaration for `LLVM_ENABLE_ZLIB` is nowhere near where it's used. In fact it's not even in the same file.


================
Comment at: llvm/lib/Support/CMakeLists.txt:3
+if ( HAVE_LIBZ )
   set(system_libs ${system_libs} ${ZLIB_LIBRARIES})
 endif()
----------------
labath wrote:
> What would be the new value of `ZLIB_LIBRARIES` here? One of my previous attempts to refactor this failed because we have code which assumes `system_libs` is a list bare library names `z dl ...` that can then be prefixed with `-l` and passed to the linker. This fails horribly if you specify a full path to a library (which cmake likes to to).
> Please make sure that `llvm-config --system-libs` produces something reasonable after this patch.
Sounds like we should have a better test case then. It looks like `test/tools/llvm-config/system-libs.test` is too weak to check what you asking for.


Repository:
  rL LLVM

https://reviews.llvm.org/D47560