Page MenuHomePhabricator

Fix cmake for zlib
Needs RevisionPublic

Authored by wenlei on Jan 9 2020, 6:49 PM.

Details

Summary

system_libs should be set to library names instead of library path. find_package(ZLIB) populates ZLIB_LIBRARY with path and that evetually gets rolled into INTERFACE_LINK_LIBRARIES of LLVMSupport which made it not portable.

Event Timeline

wenlei created this revision.Jan 9 2020, 6:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2020, 6:49 PM

Is there a way to get the basename from ZLIB_LIBRARY by any chance? Not saying it's actually necessary, just wondering.

Is there a way to get the basename from ZLIB_LIBRARY by any chance? Not saying it's actually necessary, just wondering.

That would be trying to recover z from a/b/c/libz.so. Given how system_libs is set for other libs (line 8), I think it's better to just set it directly.

I should also mention that the problem was introduced in this change: https://github.com/llvm/llvm-project/commit/abb00753069554c538f3d850897373d093389945#diff-cb177ae2458d3cc8a8e499510ab787c2

compnerd requested changes to this revision.Jan 13 2020, 11:46 AM

I'm not sure I understand the motivation for this change. Can you please expand on that? How are you using it that it doesn't work for you? Additionally, z is definitely the wrong name, please compute that by using get_file_name_component, CMAKE_SHARED_LIBRARY_PREFIX, and string(REGEX...)

This revision now requires changes to proceed.Jan 13 2020, 11:46 AM
wenlei added a comment.Mar 3 2020, 3:12 PM

I'm not sure I understand the motivation for this change. Can you please expand on that? How are you using it that it doesn't work for you?

@compnerd, the first problem is system_libs will be propagated into INTERFACE_LINK_LIBRARIES which is supposed to be relocatable (https://cmake.org/cmake/help/v3.3/prop_tgt/INTERFACE_LINK_LIBRARIES.html). However ZLIB_LIBRARY contains the absolute path zlib, so having ZLIB_LIBRARY in system_libs is wrong as it leads to non-relocatable interface libs.

Additionally, z is definitely the wrong name, please compute that by using get_file_name_component, CMAKE_SHARED_LIBRARY_PREFIX, and string(REGEX...)

The problem also goes beyond absolute path vs relative path. If you look at line 11 for example, the way these libraries are described is like this: rt for librt.so. So we had to revert it back to use z for zlib.so for our usage of LLVM libs. What you suggested sounds like a good idea, but I won't be able to revisit this any time soon. So feel free to take over the patch. Thanks.