[LLVMdev] proposal to avoid zlib dependency.

[Dan Liew]

On 17 September 2014 17:26, Mueller-Roemer, Johannes Sebastian
<Johannes.Sebastian.Mueller-Roemer at igd.fraunhofer.de> wrote:
> Well, I just had a quick look again and there were two issues:
>
> You added ${ZLIB_LIBRARIES} to CMAKE_REQUIRED_LIBRARIES not ${ZLIB_LIBRARY}
> That was covered up under Linux/MacOSX by

Do we have CMake version incompatibility? My version of FindZLIB.cmake
(shipped with CMake 3.0.1) says in the comments to use
${ZLIB_LIBRARIES} (although looking at the implementation
${ZLIB_LIBRARY} is also set but marked as advanced). My version looks
like [1]. I also checked FindZLIB.cmake shipped with CMake 2.8.7 and
it also says to use ${ZLIB_LIBRARIES}


> if ( LLVM_ENABLE_ZLIB AND HAVE_LIBZ )
>   set(system_libs ${system_libs} z)
> endif()

Oops. Well spotted.

> in Support's CMakeLists.txt
>
> I corrected the first point and removed the second. Now, HAVE_LIBZ and HAVE_ZLIB_H aren't used at all anymore so I removed those as well.

That's not true. They are used in include/llvm/config.h.cmake to
generated #defines which are then used by

* ``lib/Support/Compression.cpp``
* ``unittests/Support/CompressionTest.cpp``
* ``test/lit.site.cfg.in``

This probably needs cleaning up. A single ``LLVM_ENABLE_ZLIB`` macro
should be enough.

> PS: I wouldn't really consider CMAKE_REQUIRED_LIBRARIES/CMAKE_REQUIRED_INCLUDES a clean solution. target_include_directories and target_link_libraries offer much finer control.

Yeah my solution isn't clean, it was a hack because I didn't know
which libraries depended on zlib. If its just libLLVMSupport then we
should use ``target_include_directories()`` and
``target_link_libraries()``. It turns out we can't use
``target_include_directories()`` because that was included in CMake
2.8.11 and the minimum CMake version is 2.8.7 :(

I started trying to remove HAVE_LIBZ and HAVE_ZLIB_H but the changes
go deeper than I thought because the Autoconf/Makefile system needs to
be kept in sync as well and my system isn't set up to regenerate the
configure script right now. We certainly can and should remove
HAVE_LIBZ and HAVE_ZLIB_H but I think we should do it as a separate
patch.

So here's a revised version patch of my original patch. Note also I've
continued to use ${ZLIB_LIBRARIES} rather than ${ZLIB_LIBRARY} until
we resolve which we should be using.


[1] https://github.com/Kitware/CMake/blob/f051814ed0e63badbfd68049354f36259dbf4b49/Modules/FindZLIB.cmake
-- 
Dan Liew
PhD Student - Imperial College London
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Switch-detection-of-zlib-in-CMake-build-systemto-usi.patch
Type: text/x-patch
Size: 2698 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140917/77f60bc9/attachment.bin>