On 17 September 2014 16:34, Mueller-Roemer, Johannes Sebastian <Johannes.Sebastian.Mueller-Roemer at igd.fraunhofer.de> wrote:> I'll have another look tomorrow.Thanks.> IMO, it is unnecessary clutter. It will fail if ${ZLIB_LIBRARIES} or ${ZLIB_INCLUDE_DIRS} are used anyways as they will be set to NOTFOUND. Or we go back to a silent-fail solution:Fair enough. I've never been a big fan of silent failure. So perhaps just this... ``` if (LLVM_ENABLE_ZLIB ) find_package(ZLIB REQUIRED) set(HAVE_ZLIB_H 1) set(HAVE_LIBZ 1) list(APPEND CMAKE_REQUIRED_LIBRARIES ${ZLIB_LIBRARIES}) list(APPEND CMAKE_REQUIRED_INCLUDES ${ZLIB_INCLUDE_DIRS}) else() set(HAVE_ZLIB_H 0) set(HAVE_LIBZ 0) endif() ``` -- Dan Liew PhD Student - Imperial College London
Mueller-Roemer, Johannes Sebastian
2014-Sep-17 16:26 UTC
[LLVMdev] proposal to avoid zlib dependency.
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
if ( LLVM_ENABLE_ZLIB AND HAVE_LIBZ )
set(system_libs ${system_libs} z)
endif()
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.
Patch attached.
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.
--
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 17:52
To: Mueller-Roemer, Johannes Sebastian
Cc: llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] proposal to avoid zlib dependency.
On 17 September 2014 16:34, Mueller-Roemer, Johannes Sebastian
<Johannes.Sebastian.Mueller-Roemer at igd.fraunhofer.de>
wrote:> I'll have another look tomorrow.
Thanks.
> IMO, it is unnecessary clutter. It will fail if ${ZLIB_LIBRARIES} or
${ZLIB_INCLUDE_DIRS} are used anyways as they will be set to NOTFOUND. Or we go
back to a silent-fail solution:
Fair enough. I've never been a big fan of silent failure. So perhaps just
this...
```
if (LLVM_ENABLE_ZLIB )
find_package(ZLIB REQUIRED)
set(HAVE_ZLIB_H 1)
set(HAVE_LIBZ 1)
list(APPEND CMAKE_REQUIRED_LIBRARIES ${ZLIB_LIBRARIES})
list(APPEND CMAKE_REQUIRED_INCLUDES ${ZLIB_INCLUDE_DIRS})
else()
set(HAVE_ZLIB_H 0)
set(HAVE_LIBZ 0)
endif()
```
--
Dan Liew
PhD Student - Imperial College London
-------------- next part --------------
A non-text attachment was scrubbed...
Name: zlibfix.patch
Type: application/octet-stream
Size: 1928 bytes
Desc: zlibfix.patch
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140917/1adb46c9/attachment.obj>
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 byDo 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>