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

[Dan Liew via Phabricator via llvm-commits]

delcypher requested changes to this revision.
delcypher added a comment.
This revision now requires changes to proceed.

The idea behind this change is fine. The main problem in the implementation is not considering the scenario where `LLVM_ENABLE_ZLIB` is initially `TRUE` but is then changed at a later date. The current implementation would ignore the value of `LLVM_ENABLE_ZLIB` because the `HAVE_LIBZ` cache variable is still present from a previous configure run.



================
Comment at: llvm/cmake/config-ix.cmake:112
+      include_directories(${ZLIB_INCLUDE_DIR})
+    endif()
   endif()
----------------
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.


================
Comment at: llvm/cmake/config-ix.cmake:113
+    endif()
   endif()
 
----------------
We need to handle the case that Zlib was found previously but `LLVM_ENABLE_ZLIB` is now `FALSE`. You can either do it here so that `HAVE_LIBZ` is set to `FALSE` or you can update all uses of `HAVE_LIBZ` to also check if `LLVM_ENABLE_ZLIB` is set to `TRUE`.


================
Comment at: llvm/lib/Support/CMakeLists.txt:2
 set(system_libs)
-if ( LLVM_ENABLE_ZLIB AND HAVE_LIBZ )
+if ( HAVE_LIBZ )
   set(system_libs ${system_libs} ${ZLIB_LIBRARIES})
----------------
You shouldn't drop `LLVM_ENABLE_ZLIB` in the condition here, at least in the current implementation anyway. AFAIK both `LLVM_ENABLE_ZLIB` and `HAVE_LIBZ` are cache variables.

What could happen is that the user configures with  `LLVM_ENABLE_ZLIB` and ZLIB is found and used.
Then at a later date with an existing build directory the user decides to set `LLVM_ENABLE_ZLIB` to `FALSE`.
If that happens `HAVE_LIBZ` will still be `TRUE` causing the code to continue to use Zlib despite `LLVM_ENABLE_ZLIB` being
set to `FALSE`.


================
Comment at: llvm/lib/Support/Compression.cpp:21
 #include "llvm/Support/ErrorHandling.h"
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_ZLIB_H
+#if HAVE_ZLIB
 #include <zlib.h>
----------------
Shouldn't this be `HAVE_LIBZ`?


================
Comment at: llvm/lib/Support/Compression.cpp:27
 
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_LIBZ
+#if HAVE_ZLIB
 static Error createError(StringRef Err) {
----------------
Shouldn't this be `HAVE_LIBZ`?


Repository:
  rL LLVM

https://reviews.llvm.org/D47560