This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Use find_package to discover zlib
ClosedPublic

Authored by v.g.vassilev on Oct 17 2017, 7:57 AM.

Details

Reviewers
beanz
Summary

This allows us to use standard cmake utilities to point to non-system zlib locations.

Diff Detail

Event Timeline

v.g.vassilev created this revision.Oct 17 2017, 7:57 AM
beanz requested changes to this revision.Oct 17 2017, 11:02 AM
beanz added inline comments.
interpreter/CMakeLists.txt
27 ↗(On Diff #119321)

This shouldn't be here.

interpreter/llvm/src/lib/Support/CMakeLists.txt
25 ↗(On Diff #119321)

The correct variable to use here is ZLIB_LIBRARIES. While ZLIB_LIBRARY probably works in most cases, it is an implementation detail of the FindZLIB CMake package.

This revision now requires changes to proceed.Oct 17 2017, 11:02 AM
v.g.vassilev edited edge metadata.
v.g.vassilev marked 2 inline comments as done.

Address comments.

interpreter/CMakeLists.txt
27 ↗(On Diff #119321)

Oops ;)

beanz accepted this revision.Oct 17 2017, 1:22 PM

LGTM

This revision is now accepted and ready to land.Oct 17 2017, 1:22 PM
v.g.vassilev closed this revision.Oct 17 2017, 1:32 PM

Landed in r316025.

Reverted in r316029 because the bots got angry at me:

CMake Error at /usr/local/share/cmake-3.7/Modules/FindPackageHandleStandardArgs.cmake:138 (message):
  Could NOT find ZLIB (missing: ZLIB_LIBRARY ZLIB_INCLUDE_DIR)
Call Stack (most recent call first):
  /usr/local/share/cmake-3.7/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  /usr/local/share/cmake-3.7/Modules/FindZLIB.cmake:114 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  cmake/config-ix.cmake:135 (find_package)
  CMakeLists.txt:585 (include)

@beanz, do you have an idea what went wrong?

I assume this would fix it:

find_package(ZLIB)
if (ZLIB_FOUND)
  set(HAVE_LIBZ 1)
else()
  check_library_exists(z compress2 "" HAVE_LIBZ)
  if(HAVE_LIBZ)
     set(ZLIB_LIBRARIES z)
  endif()
endif()

The question is whether that's the right thing to do.

Revised patch landed in r316150.

Reverted in r316153...