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

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/lib/Support/CMakeLists.txt:3
+if ( HAVE_LIBZ )
   set(system_libs ${system_libs} ${ZLIB_LIBRARIES})
 endif()
----------------
phosek wrote:
> labath wrote:
> > delcypher wrote:
> > > 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.
> > Yes, a better test case would be useful certainly (but don't worry you will get test failures, they will just be in compiler-rt, or other projects which use llvm-config to invoke the linker :)).
> > 
> > Anyway, I am not asking for anything. I don't have a horse in this game; as long as the main cmake project builds I am happy. I'm just giving you a heads up that you are likely to run into issues down this road.
> Isn't this actually more correct? In our case, I'd like to use our own version of zlib and libxml2 rather than the system ones using the `CMAKE_FIND_ROOT_PATH` option. If `llvm-config --system-libs` prints `-lz -lxml2`, it's ambiguous which zlib and libxml2 it's referring to. Furthermore, in our case we want to link zlib and libxml2 statically, so presumably `llvm-config --system-libs` shouldn't print `-lz -lxml2` at all but that's not currently the case.
> Isn't this actually more correct? 

You could argue it's more correct if you also fixed --system-libs to output the library as `/usr/lib/libz.so` or something else that the linker can consume. As it stands now it will print `-l/usr/lib/libz.so` which will just blow up. I don't know much about the scenarios in which llvm-config is used (I just know that when I tried something like this I found a dozen angry emails waiting for me the next day), but this change might actually be enough to appease people relying on it.

> Furthermore, in our case we want to link zlib and libxml2 statically, so presumably llvm-config --system-libs shouldn't print -lz -lxml2 at all but that's not currently the case.

The intended audience of llvm-config is people who want to link against llvm. For that the modality (shared/static) of libz doesn't matter. What matters is the modality of llvm. If you built llvm as a bunch of static libraries, when using them, you will need to pass libz to the linker (static or otherwise). If you built llvm as a single shared library, then you don't need to pass anything as shared libraries can track their dependencies (and I believe in this case the output of --system-libs will already be empty).


Repository:
  rL LLVM

https://reviews.llvm.org/D47560