[CMake] Link against ZLIB::ZLIB

[phosek]

Summary

This is the imported target that find_package(ZLIB) defines.

[phosek]

This is a reland of rG00b3d49d3a86 which addresses the llvm-config --system-libs issue.

[smeenai]

What does the llvm-config --system-libs output look like with this?

[smeenai]

llvm/lib/Support/CMakeLists.txt

198

Shouldn't NAME_WE get rid of this already?

[hans]

This is mentioned as potentially fixing the release blocker https://bugs.llvm.org/show_bug.cgi?id=44780
Is there any progress here?

[smeenai]

llvm/lib/Support/CMakeLists.txt

195

I tried patching this locally, and for whatever reason, neither IMPORTED_LOCATION_${CMAKE_BUILD_TYPE} nor just IMPORTED_LOCATION work for me; I have to use LOCATION. https://cmake.org/cmake/help/latest/prop_tgt/IMPORTED_LOCATION.html suggests that LOCATION (or LOCATION_<CONFIG>) is the right thing to do for reading the location as well? I'm on CMake 3.15.1 though, and that part of the help text only appears starting with 3.15.

Furthermore, I don't know how this would work with multi-config generators. Perhaps we should just set up a series of fallbacks?

[smeenai]

So with this, llvm-config --system-libs will print out -lz instead of the full path to the zlib library that was found. That's potentially not accurate in case you've set up your LLVM's CMake to find a different zlib than what -lz would find by default. On the other hand, it does mean you can use the llvm-config --system-libs output across systems, which is probably a more useful property.

libxml2 has the same issue as well, @hans (llvm-config --system-libs prints out an absolute path for it right now).

[compnerd]

@smeenai I agree that -lz is less than idea, however, for llvm-config, I think that is fine. In general, I think that we will want to phase out llvm-config in the longer term in favor of pkgconfig and CMake.

[phosek]

In D74176#1862853, @smeenai wrote:

What does the llvm-config --system-libs output look like with this?

$ ./bin/llvm-config --system-libs
-lrt -ldl -lpthread -lm -lz

I've updated the change. It's a question what output should this produce, especially when you use a custom zlib. When llvm-config is used on the same machine, using a full path might be arguably more correct, because you might have multiple different zlib versions and you want to report the one that was used to build LLVM. However, when used on a different machine, reporting a full path probably isn't the right solution since that path will likely be invalid on that machine.

llvm/lib/Support/CMakeLists.txt

198

In theory yes, but I think that using the _PREFIX variable is more precise.

[smeenai]

LGTM.

If this is picked to 10.0, D74106 will need to be picked as well, right?

llvm/lib/Support/CMakeLists.txt

198

I tested locally with CMake 3.4.3, and it looks like LOCATION works for reading even on that version, so I'm just gonna assume it also works in subsequent versions :D

[phosek]

llvm/lib/Support/CMakeLists.txt

198

I tested it against 3.13.4 so hopefully should be fine in other versions as well.

[rnk]

This breaks the build in the same way the last llvm-config change did. See https://bugs.llvm.org/show_bug.cgi?id=19403#c5 and rGe441a584f3f7d743ab77031a47d9ad60ee56b53d.

[hans]

In D74176#1899793, @rnk wrote:

and rGe441a584f3f7d743ab77031a47d9ad60ee56b53d.

I've pushed that to 10.x in f5fd8a37c18439102eb30c85dadac68c260a1a0d, as we had also merged the patch it's fixing.

I'll look at doing the same thing for zlib. Also, cmake is horrible.

[hans]

In D74176#1900473, @hans wrote:

In D74176#1899793, @rnk wrote:

and rGe441a584f3f7d743ab77031a47d9ad60ee56b53d.

I've pushed that to 10.x in f5fd8a37c18439102eb30c85dadac68c260a1a0d, as we had also merged the patch it's fixing.

I'll look at doing the same thing for zlib. Also, cmake is horrible.

Nice, I see rnk already did this in 1079c68aa0fdb14d270a31c0df49a2afc5ed2485.

So hopefully we're all good? Please let me know otherwise.

I've pushed these three changes to 10.x for PR44780:

D74106 50a6d3a6486d  "[CMake] Use PUBLIC link mode for static libraries"
D74176 2181bf40d871  "[CMake] Link against ZLIB::ZLIB"
       1079c68aa0fd  "Attempt to fix ZLIB CMake logic on Windows"

[wenlei]

This change broke our use of LLVMSupport library, and I also noticed it's been reverted already.

It was previously broken even without this patch, and we had to hack/patch it internally to make it work for our use case. Here's what we had internally: https://reviews.llvm.org/D72490. It's not landed, but there's some discussion about this.

llvm/lib/Support/CMakeLists.txt

 if(LLVM_ENABLE_ZLIB)
-  set(system_libs ${ZLIB_LIBRARY})
+  set(imported_libs ZLIB::ZLIB)
 endif()
 
 if( MSVC OR MINGW )
   # libuuid required for FOLDERID_Profile usage in lib/Support/Windows/Path.inc.
   # advapi32 required for CryptAcquireContextW in lib/Support/Windows/Path.inc.
   set(system_libs ${system_libs} psapi shell32 ole32 uuid advapi32)
 elseif( CMAKE_HOST_UNIX )
   if( HAVE_LIBRT )
# System
   Watchdog.cpp
 
   ADDITIONAL_HEADER_DIRS
   Unix
   Windows
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/ADT
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Support
   ${Backtrace_INCLUDE_DIRS}
-  LINK_LIBS ${system_libs} ${delayload_flags} ${Z3_LINK_FILES}
+  LINK_LIBS ${system_libs} ${imported_libs} ${delayload_flags} ${Z3_LINK_FILES}
   )
 
-set_property(TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS "${system_libs}")
+set(llvm_system_libs ${system_libs})
+
+if(LLVM_ENABLE_ZLIB)
+  get_property(zlib_library TARGET ZLIB::ZLIB PROPERTY IMPORTED_LOCATION_${CMAKE_BUILD_TYPE})
+  get_filename_component(zlib_library ${zlib_library} NAME_WE)
+  STRING(REGEX REPLACE "^${CMAKE_STATIC_LIBRARY_PREFIX}" "" zlib_library ${zlib_library})
+  STRING(REGEX REPLACE "${CMAKE_STATIC_LIBRARY_SUFFIX}$" "" zlib_library ${zlib_library})
+  set(llvm_system_libs ${llvm_system_libs} "${zlib_library}")
+endif()
+
+set_property(TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS "${llvm_system_libs}")
 
 if(LLVM_WITH_Z3)
   target_include_directories(LLVMSupport SYSTEM
     PRIVATE
     ${Z3_INCLUDE_DIR}
     )
 endif()