[LLVMdev] proposal to avoid zlib dependency.

Thu Sep 18 00:18:31 PDT 2014

Although my FindZLIB mentions ZLIB_LIBRARIES, it doesn't work... But now that I changed it back to ZLIB_LIBRARY that doesn't work either... maybe CMAKE_REQUIRED_LIBRARIES isn't supported properly with ninja (which I use as mingw32-make doesn't work properly in parallel). The new version using taget_link_libraries works fine though.

I must have somhow overlooked that configure file...  guess my file ending search filter must have skipped that one although I did include *.cmake

Now that we don't fail silently anymore, there really should be no need for those HAVE_* flags. From my search results:

>Internal search for "HAVE_ZLIB_H" in "CMakeLists.txt *.cmake *.in"
D:\projects\promo_code\external\llvm\cmake\config-ix.cmake:451:  set(HAVE_ZLIB_H 1)
D:\projects\promo_code\external\llvm\cmake\config-ix.cmake:455:  set(HAVE_ZLIB_H 0)
D:\projects\promo_code\external\llvm\include\llvm\Config\config.h.cmake:406:#cmakedefine HAVE_ZLIB_H ${HAVE_ZLIB_H}
D:\projects\promo_code\external\llvm\include\llvm\Config\config.h.in:403:#undef HAVE_ZLIB_H

>Internal search for "HAVE_LIBZ" in "CMakeLists.txt *.cmake *.in"
D:\projects\promo_code\external\llvm\cmake\config-ix.cmake:452:  set(HAVE_LIBZ 1)
D:\projects\promo_code\external\llvm\cmake\config-ix.cmake:456:  set(HAVE_LIBZ 0)

So it would appear that HAVE_LIBZ really isn't used anymore (must have searched in the wrong subdirectory yesterday). So we could at least remove one of those two variables. (See attached patch)

However I noticed another issue that I hadn't seen before and that is that generated exports (in INSTALL_DIR/share/llvm/cmake/LLVMExports.cmake) now contain an absolute path to zlib :(
I'm no expert on the whole Config/Export thing, as we haven't used that in any of our projects so far, but it would be much better if it actually used find_package for zlib as well... 

Re CMake 2.8.11:
Any chances of this change being made in the near future? Target_include_directories is very, very helpful for cleaning up CMakeLists

--
Johannes S. Mueller-Roemer, MSc
Wiss. Mitarbeiter - Interactive Engineering Technologies (IET)

Fraunhofer-Institut für Graphische Datenverarbeitung IGD
Fraunhoferstr. 5  |  64283 Darmstadt  |  Germany
Tel +49 6151 155-606  |  Fax +49 6151 155-139
johannes.mueller-roemer at igd.fraunhofer.de  |  www.igd.fraunhofer.de

-----Original Message-----
From: Dan Liew [mailto:dan at su-root.co.uk] 
Sent: Wednesday, September 17, 2014 19:51
To: Mueller-Roemer, Johannes Sebastian
Cc: llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] proposal to avoid zlib dependency.

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: zlibfix2.patch
Type: application/octet-stream
Size: 2169 bytes
Desc: zlibfix2.patch
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140918/fb1e636f/attachment.obj>