Page MenuHomePhabricator

[CMake] Link against ZLIB::ZLIB
ClosedPublic

Authored by phosek on Feb 6 2020, 3:24 PM.

Details

Summary

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

Diff Detail

Event Timeline

phosek created this revision.Feb 6 2020, 3:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2020, 3:24 PM
phosek added a comment.Feb 6 2020, 3:25 PM

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

phosek updated this revision to Diff 243033.Feb 6 2020, 3:26 PM

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

smeenai added inline comments.Feb 10 2020, 9:15 AM
llvm/lib/Support/CMakeLists.txt
198

Shouldn't NAME_WE get rid of this already?

hans added a subscriber: hans.Feb 19 2020, 5:23 AM

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

smeenai added inline comments.Feb 19 2020, 10:06 AM
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?

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).

@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 updated this revision to Diff 246058.Feb 21 2020, 10:16 PM
phosek marked 3 inline comments as done.Feb 21 2020, 10:19 PM

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 accepted this revision.Feb 21 2020, 10:50 PM

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

This revision is now accepted and ready to land.Feb 21 2020, 10:50 PM
phosek marked 3 inline comments as done.
phosek added inline comments.
llvm/lib/Support/CMakeLists.txt
198

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

This revision was automatically updated to reflect the committed changes.
phosek marked an inline comment as done.
rnk added a subscriber: rnk.Feb 29 2020, 8:01 PM

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 added a comment.Mar 2 2020, 1:49 AM
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 added a comment.Mar 2 2020, 2:08 AM
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 added a subscriber: wenlei.Mar 3 2020, 3:18 PM

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.