Page MenuHomePhabricator

[CMake] Simplify CMake handling for zlib
ClosedPublic

Authored by phosek on Apr 30 2020, 5:22 PM.

Details

Summary

Rather than handling zlib handling manually, use find_package from CMake
to find zlib properly. Use this to normalize the LLVM_ENABLE_ZLIB,
HAVE_ZLIB, HAVE_ZLIB_H. Furthermore, require zlib if LLVM_ENABLE_ZLIB is
set to YES, which requires the distributor to explicitly select whether
zlib is enabled or not. This simplifies the CMake handling and usage in
the rest of the tooling.

This is a reland of rGabb0075 with all followup changes and fixes that should address
issues that were reported in PR44780.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Requesting changes to remove from my review queue while the above comments are being addressed.

This revision now requires changes to proceed.May 13 2020, 2:25 PM
phosek updated this revision to Diff 274255.Jun 29 2020, 3:04 PM
phosek marked an inline comment as done.
phosek added inline comments.
llvm/cmake/config-ix.cmake
531

I've updated the change to shadow the variable as suggested by Jonas.

phosek updated this revision to Diff 274256.Jun 29 2020, 3:06 PM
JDevlieghere accepted this revision.Jun 30 2020, 1:59 PM

Thanks Petr, LGTM

smeenai accepted this revision.Jul 14 2020, 7:11 PM

Oops, missed that I'd become a blocking reviewer for this (cos of requesting changes before).

This revision is now accepted and ready to land.Jul 14 2020, 7:11 PM
This revision was automatically updated to reflect the committed changes.
phosek reopened this revision.Jul 14 2020, 8:02 PM

I had to revert this change because it broke bots that don't have zlib installed. What I haven't realized is that the shadowed variable will only be accessible from the same file. I could move the logic from llvm/lib/Support/CMakeLists.txt into llvm/cmake/config-ix.cmake, but that doesn't handle all the other files that need to access that variable, such as all the */test/CMakeLists.txt files. So I don't know if shadowing is the way to go here. Maybe we should set a different internal cached variable (HAVE_LIBZ?) and use that to check if zlib is available. What do you think?

This revision is now accepted and ready to land.Jul 14 2020, 8:02 PM

I wouldn't mind separate (internal) cache variable, though I am somewhat surprised by this problem. A (non-cache) variable set in one of the parent scopes should still take precedence over a cache variable with the same name. And since config-ix.cmake is included from the top CMakeLists.txt, the value it defines should be available everywhere. Was this a problem for the regular build, or only for some of the more exotic build setups that don't start with llvm/CMakeLists.txt ?

phosek updated this revision to Diff 278216.Jul 15 2020, 9:14 AM

I wouldn't mind separate (internal) cache variable, though I am somewhat surprised by this problem. A (non-cache) variable set in one of the parent scopes should still take precedence over a cache variable with the same name. And since config-ix.cmake is included from the top CMakeLists.txt, the value it defines should be available everywhere. Was this a problem for the regular build, or only for some of the more exotic build setups that don't start with llvm/CMakeLists.txt ?

Never mind, it's working as expected. The problem is that we disable zlib detection on Windows which I missed before. I'm not sure why that's the case, I tested this on Windows and it seems to be working fine, but for now I've kept the existing behavior, we can consider enabling zlib on Windows in a follow up change.

This revision was automatically updated to reflect the committed changes.
phosek reopened this revision.Jul 23 2020, 5:51 PM
This revision is now accepted and ready to land.Jul 23 2020, 5:51 PM
phosek updated this revision to Diff 280305.Jul 23 2020, 5:51 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2020, 11:06 PM
vsk added a subscriber: vsk.Jul 24 2020, 12:33 PM

@phosek I suspect this is causing a cmake error on the lldb standalone bot, would you mind taking a look?

http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake-standalone/1858/

CMake Error at /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/clang-build/lib/cmake/llvm/AddLLVM.cmake:823 (add_executable):
  Target "lit-cpuid" links to target "ZLIB::ZLIB" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?
Call Stack (most recent call first):
  cmake/modules/AddLLDB.cmake:165 (add_llvm_executable)
  utils/lit-cpuid/CMakeLists.txt:1 (add_lldb_executable)
mehdi_amini added inline comments.Jul 25 2020, 6:09 PM
mlir/examples/standalone/CMakeLists.txt
35 ↗(On Diff #280331)

I am a bit unsure that it is desirable that it is desirable to need this change?
This requires every single user of LLVM to do this. Is there a way this can be done in the call to find_package(LLVM) instead?

mehdi_amini added inline comments.Jul 25 2020, 6:13 PM
mlir/examples/standalone/CMakeLists.txt
35 ↗(On Diff #280331)

(Doc for the usual way to integrate LLVM: https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project )

mlir/examples/standalone/CMakeLists.txt
35 ↗(On Diff #280331)

I tend to agree with Mehdi. I think stuff like this which is *required* to use LLVM should get pushed into what LLVM exports. (Actually there is a bunch of other configuration stuff which also needs to get pushed into the export, otherwise configuration defaults are all wrong and things like that.)

phosek marked 4 inline comments as done.Jul 27 2020, 11:59 AM
phosek added inline comments.
mlir/examples/standalone/CMakeLists.txt
35 ↗(On Diff #280331)

Implemented in D84691.

hans added a subscriber: hans.Aug 5 2020, 3:12 AM

I wouldn't mind separate (internal) cache variable, though I am somewhat surprised by this problem. A (non-cache) variable set in one of the parent scopes should still take precedence over a cache variable with the same name. And since config-ix.cmake is included from the top CMakeLists.txt, the value it defines should be available everywhere. Was this a problem for the regular build, or only for some of the more exotic build setups that don't start with llvm/CMakeLists.txt ?

Never mind, it's working as expected. The problem is that we disable zlib detection on Windows which I missed before. I'm not sure why that's the case, I tested this on Windows and it seems to be working fine, but for now I've kept the existing behavior, we can consider enabling zlib on Windows in a follow up change.

:-( We rely on zlib on Windows, and we pass -DLLVM_ENABLE_ZLIB=FORCE_ON to ensure that the build breaks if it can't be used for some reason.

It seems your change both disabled use of zlib on Windows, and also removed the check which made that an error when using FORCE_ON, which means we didn't catch this until further down the line.

phosek reopened this revision.Aug 5 2020, 2:21 PM
phosek updated this revision to Diff 283398.
phosek marked an inline comment as done.

Sorry about the breakage, that was an unintentional change. I have updated the patch and restored the original behavior on Windows.

This revision is now accepted and ready to land.Aug 5 2020, 2:22 PM
This revision was automatically updated to reflect the committed changes.
hans added inline comments.Aug 6 2020, 4:25 AM
llvm/cmake/config-ix.cmake
178

Could this check be put back? E.g. for now it seems building with -DLLVM_USE_SANITIZERS=Memory and -DLLVM_ENABLE_ZLIB=FORCE_ON will silently not use zlib.

lxfind added a subscriber: lxfind.EditedAug 6 2020, 1:55 PM

@phosek, Under this change, now when I build LLVM (with a basic config cmake -G Ninja --LLVM_ENABLE_PROJECTS=clang ../llvm), in file build_dir/lib/cmake/llvm/LLVMExports.cmake, I see this:

set_target_properties(LLVMSupport PROPERTIES
  INTERFACE_LINK_LIBRARIES "curses;m;ZLIB::ZLIB;LLVMDemangle"

This seems broken to me. Can you take a look?

phosek added a comment.Aug 6 2020, 4:06 PM

@phosek, Under this change, now when I build LLVM (with a basic config cmake -G Ninja --LLVM_ENABLE_PROJECTS=clang ../llvm), in file build_dir/lib/cmake/llvm/LLVMExports.cmake, I see this:

set_target_properties(LLVMSupport PROPERTIES
  INTERFACE_LINK_LIBRARIES "curses;m;ZLIB::ZLIB;LLVMDemangle"

This seems broken to me. Can you take a look?

This is correct. That target is provided by find_package(ZLIB). In LLVMConfig.cmake, we invoke find_package(ZLIB) when zlib support is enabled. If you're using LLVMExports.cmake, you'll need to invoke find_package(ZLIB) yourself.

kuhnel added a subscriber: kuhnel.Aug 7 2020, 12:36 AM

This patch broke the Windows compilation on buildbot and pre-merge testing. So I'll revert it to get pre-merge testing back online. Otherwise this will cause false-positives for pre-merge testing.

The error message:

FAILED: bin/llvm-profdata.exe 
cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_exe --intdir=tools\llvm-profdata\CMakeFiles\llvm-profdata.dir --rc="C:\Program Files (x86)\Windows Kits\10\bin\10.0.18362.0\x64\rc.exe" --mt="C:\Program Files (x86)\Windows Kits\10\bin\10.0.18362.0\x64\mt.exe" --manifests  -- C:\BuildTools\VC\Tools\MSVC\14.26.28801\bin\Hostx64\x64\link.exe /nologo tools\llvm-profdata\CMakeFiles\llvm-profdata.dir\llvm-profdata.cpp.obj tools\llvm-profdata\CMakeFiles\llvm-profdata.dir\__\__\resources\windows_version_resource.rc.res  /out:bin\llvm-profdata.exe /implib:lib\llvm-profdata.lib /pdb:bin\llvm-profdata.pdb /version:0.0  /machine:x64 /STACK:10000000 /INCREMENTAL:NO /subsystem:console  lib\LLVMCore.lib lib\LLVMProfileData.lib lib\LLVMSupport.lib lib\LLVMCore.lib lib\LLVMBinaryFormat.lib lib\LLVMRemarks.lib lib\LLVMBitstreamReader.lib lib\LLVMSupport.lib psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib C:\GnuWin\lib\zlib.lib delayimp.lib -delayload:shell32.dll -delayload:ole32.dll lib\LLVMDemangle.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cd ."
LINK: command "C:\BuildTools\VC\Tools\MSVC\14.26.28801\bin\Hostx64\x64\link.exe /nologo tools\llvm-profdata\CMakeFiles\llvm-profdata.dir\llvm-profdata.cpp.obj tools\llvm-profdata\CMakeFiles\llvm-profdata.dir\__\__\resources\windows_version_resource.rc.res /out:bin\llvm-profdata.exe /implib:lib\llvm-profdata.lib /pdb:bin\llvm-profdata.pdb /version:0.0 /machine:x64 /STACK:10000000 /INCREMENTAL:NO /subsystem:console lib\LLVMCore.lib lib\LLVMProfileData.lib lib\LLVMSupport.lib lib\LLVMCore.lib lib\LLVMBinaryFormat.lib lib\LLVMRemarks.lib lib\LLVMBitstreamReader.lib lib\LLVMSupport.lib psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib C:\GnuWin\lib\zlib.lib delayimp.lib -delayload:shell32.dll -delayload:ole32.dll lib\LLVMDemangle.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:bin\llvm-profdata.exe.manifest" failed (exit code 1120) with the following output:
LLVMSupport.lib(Compression.cpp.obj) : error LNK2019: unresolved external symbol compress2 referenced in function "class llvm::Error __cdecl llvm::zlib::compress(class llvm::StringRef,class llvm::SmallVectorImpl<char> &,int)" (?compress@zlib@llvm@@YA?AVError@2@VStringRef@2@AEAV?$SmallVectorImpl@D@2@H@Z)
LLVMSupport.lib(Compression.cpp.obj) : error LNK2019: unresolved external symbol compressBound referenced in function "class llvm::Error __cdecl llvm::zlib::compress(class llvm::StringRef,class llvm::SmallVectorImpl<char> &,int)" (?compress@zlib@llvm@@YA?AVError@2@VStringRef@2@AEAV?$SmallVectorImpl@D@2@H@Z)
LLVMSupport.lib(Compression.cpp.obj) : error LNK2019: unresolved external symbol uncompress referenced in function "class llvm::Error __cdecl llvm::zlib::uncompress(class llvm::StringRef,class llvm::SmallVectorImpl<char> &,unsigned __int64)" (?uncompress@zlib@llvm@@YA?AVError@2@VStringRef@2@AEAV?$SmallVectorImpl@D@2@_K@Z)
  Hint on symbols that are defined and could potentially match:
    "class llvm::Error __cdecl llvm::zlib::uncompress(class llvm::StringRef,class llvm::SmallVectorImpl<char> &,unsigned __int64)" (?uncompress@zlib@llvm@@YA?AVError@2@VStringRef@2@AEAV?$SmallVectorImpl@D@2@_K@Z)
    "class llvm::Error __cdecl llvm::zlib::uncompress(class llvm::StringRef,char *,unsigned __int64 &)" (?uncompress@zlib@llvm@@YA?AVError@2@VStringRef@2@PEADAEA_K@Z)
LLVMSupport.lib(Compression.cpp.obj) : error LNK2019: unresolved external symbol crc32 referenced in function "unsigned int __cdecl llvm::zlib::crc32(class llvm::StringRef)" (?crc32@zlib@llvm@@YAIVStringRef@2@@Z)
  Hint on symbols that are defined and could potentially match:
    "unsigned int __cdecl llvm::zlib::crc32(class llvm::StringRef)" (?crc32@zlib@llvm@@YAIVStringRef@2@@Z)
C:\GnuWin\lib\zlib.lib : warning LNK4272: library machine type 'x86' conflicts with target machine type 'x64'
bin\llvm-profdata.exe : fatal error LNK1120: 4 unresolved externals
kuhnel reopened this revision.Aug 7 2020, 12:43 AM
This revision is now accepted and ready to land.Aug 7 2020, 12:43 AM
kuhnel requested changes to this revision.Aug 7 2020, 12:43 AM
This revision now requires changes to proceed.Aug 7 2020, 12:43 AM
phosek added a comment.Aug 7 2020, 1:02 AM

How can I test this change on pre-merge bots? I haven't seen any builds posted on this change before.

phosek added a comment.Aug 7 2020, 1:14 AM

This patch broke the Windows compilation on buildbot and pre-merge testing. So I'll revert it to get pre-merge testing back online. Otherwise this will cause false-positives for pre-merge testing.

The error message:

FAILED: bin/llvm-profdata.exe 
cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_exe --intdir=tools\llvm-profdata\CMakeFiles\llvm-profdata.dir --rc="C:\Program Files (x86)\Windows Kits\10\bin\10.0.18362.0\x64\rc.exe" --mt="C:\Program Files (x86)\Windows Kits\10\bin\10.0.18362.0\x64\mt.exe" --manifests  -- C:\BuildTools\VC\Tools\MSVC\14.26.28801\bin\Hostx64\x64\link.exe /nologo tools\llvm-profdata\CMakeFiles\llvm-profdata.dir\llvm-profdata.cpp.obj tools\llvm-profdata\CMakeFiles\llvm-profdata.dir\__\__\resources\windows_version_resource.rc.res  /out:bin\llvm-profdata.exe /implib:lib\llvm-profdata.lib /pdb:bin\llvm-profdata.pdb /version:0.0  /machine:x64 /STACK:10000000 /INCREMENTAL:NO /subsystem:console  lib\LLVMCore.lib lib\LLVMProfileData.lib lib\LLVMSupport.lib lib\LLVMCore.lib lib\LLVMBinaryFormat.lib lib\LLVMRemarks.lib lib\LLVMBitstreamReader.lib lib\LLVMSupport.lib psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib C:\GnuWin\lib\zlib.lib delayimp.lib -delayload:shell32.dll -delayload:ole32.dll lib\LLVMDemangle.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cd ."
LINK: command "C:\BuildTools\VC\Tools\MSVC\14.26.28801\bin\Hostx64\x64\link.exe /nologo tools\llvm-profdata\CMakeFiles\llvm-profdata.dir\llvm-profdata.cpp.obj tools\llvm-profdata\CMakeFiles\llvm-profdata.dir\__\__\resources\windows_version_resource.rc.res /out:bin\llvm-profdata.exe /implib:lib\llvm-profdata.lib /pdb:bin\llvm-profdata.pdb /version:0.0 /machine:x64 /STACK:10000000 /INCREMENTAL:NO /subsystem:console lib\LLVMCore.lib lib\LLVMProfileData.lib lib\LLVMSupport.lib lib\LLVMCore.lib lib\LLVMBinaryFormat.lib lib\LLVMRemarks.lib lib\LLVMBitstreamReader.lib lib\LLVMSupport.lib psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib C:\GnuWin\lib\zlib.lib delayimp.lib -delayload:shell32.dll -delayload:ole32.dll lib\LLVMDemangle.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:bin\llvm-profdata.exe.manifest" failed (exit code 1120) with the following output:
LLVMSupport.lib(Compression.cpp.obj) : error LNK2019: unresolved external symbol compress2 referenced in function "class llvm::Error __cdecl llvm::zlib::compress(class llvm::StringRef,class llvm::SmallVectorImpl<char> &,int)" (?compress@zlib@llvm@@YA?AVError@2@VStringRef@2@AEAV?$SmallVectorImpl@D@2@H@Z)
LLVMSupport.lib(Compression.cpp.obj) : error LNK2019: unresolved external symbol compressBound referenced in function "class llvm::Error __cdecl llvm::zlib::compress(class llvm::StringRef,class llvm::SmallVectorImpl<char> &,int)" (?compress@zlib@llvm@@YA?AVError@2@VStringRef@2@AEAV?$SmallVectorImpl@D@2@H@Z)
LLVMSupport.lib(Compression.cpp.obj) : error LNK2019: unresolved external symbol uncompress referenced in function "class llvm::Error __cdecl llvm::zlib::uncompress(class llvm::StringRef,class llvm::SmallVectorImpl<char> &,unsigned __int64)" (?uncompress@zlib@llvm@@YA?AVError@2@VStringRef@2@AEAV?$SmallVectorImpl@D@2@_K@Z)
  Hint on symbols that are defined and could potentially match:
    "class llvm::Error __cdecl llvm::zlib::uncompress(class llvm::StringRef,class llvm::SmallVectorImpl<char> &,unsigned __int64)" (?uncompress@zlib@llvm@@YA?AVError@2@VStringRef@2@AEAV?$SmallVectorImpl@D@2@_K@Z)
    "class llvm::Error __cdecl llvm::zlib::uncompress(class llvm::StringRef,char *,unsigned __int64 &)" (?uncompress@zlib@llvm@@YA?AVError@2@VStringRef@2@PEADAEA_K@Z)
LLVMSupport.lib(Compression.cpp.obj) : error LNK2019: unresolved external symbol crc32 referenced in function "unsigned int __cdecl llvm::zlib::crc32(class llvm::StringRef)" (?crc32@zlib@llvm@@YAIVStringRef@2@@Z)
  Hint on symbols that are defined and could potentially match:
    "unsigned int __cdecl llvm::zlib::crc32(class llvm::StringRef)" (?crc32@zlib@llvm@@YAIVStringRef@2@@Z)
C:\GnuWin\lib\zlib.lib : warning LNK4272: library machine type 'x86' conflicts with target machine type 'x64'
bin\llvm-profdata.exe : fatal error LNK1120: 4 unresolved externals

Looking at the error, it seems like you have a 32-bit version of zlib installed and in your search path on a 64-bit version of Windows. I'm not quite sure how to handle that in CMake.

kuhnel added a comment.Aug 7 2020, 1:31 AM

How can I test this change on pre-merge bots? I haven't seen any builds posted on this change before.

You can just look at the mlir-windows buildbot, this also showed the failing build: http://lab.llvm.org:8011/builders/mlir-windows?numbuilds=200
However it's lagging behind quite a bit.

Pre-merge testing is using a different infrastructure and is not sending notifications on failing master build. We're still trying to figure out how to deal with a broken master branch properly without building every revision...
Right now it's building master every 4 hours using the same setup used for pre-merge testing: https://buildkite.com/llvm-project/llvm-master-build/builds

kuhnel added a comment.Aug 7 2020, 1:40 AM

Looking at the error, it seems like you have a 32-bit version of zlib installed and in your search path on a 64-bit version of Windows. I'm not quite sure how to handle that in CMake.

For mlir-windows buildbot:
I have no clue on what is installed there.

For pre-merge testing:
I guess zlib is part of GNUWin32 package being installed. This is the recommended setup for Visual Studio. The pre-merge builds are running in a docker container on Windows, so you can check what software is being installed.

lxfind added a comment.Aug 7 2020, 1:51 PM

@phosek, Under this change, now when I build LLVM (with a basic config cmake -G Ninja --LLVM_ENABLE_PROJECTS=clang ../llvm), in file build_dir/lib/cmake/llvm/LLVMExports.cmake, I see this:

set_target_properties(LLVMSupport PROPERTIES
  INTERFACE_LINK_LIBRARIES "curses;m;ZLIB::ZLIB;LLVMDemangle"

This seems broken to me. Can you take a look?

This is correct. That target is provided by find_package(ZLIB). In LLVMConfig.cmake, we invoke find_package(ZLIB) when zlib support is enabled. If you're using LLVMExports.cmake, you'll need to invoke find_package(ZLIB) yourself.

@phosek, Thanks for the reply. I am not too familiar with this. Could you please elaborate more on why setting it to ZLIB::ZLIB here is more modern? And how one is expected to deal with ZLIB::ZLIB that shows up in LLVMExports.cmake` (in the modern way)? Should one string pattern matching for ZLIB::ZLIB and invoke find_package(ZLIB) again? (previously one could simply concatenate -l with the library name in there.

phosek updated this revision to Diff 284106.Aug 7 2020, 5:52 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 8 2020, 4:45 PM
This revision was automatically updated to reflect the committed changes.
phosek reopened this revision.Aug 8 2020, 7:35 PM
phosek updated this revision to Diff 284161.

I have finally managed to reproduce the issue on my Windows machine, the latest version of the patch should address the issue. The previous logic would fail to find zlib altogether on Windows, which is why this issue haven't manifested before. find_package locates the zlib correctly, but we need to ensure that the library that was found can be used.

@kuhnel do you want to take a look or is it okay if I just submit the change?

This revision was not accepted when it landed; it landed in state Needs Review.Aug 11 2020, 8:24 PM
This revision was automatically updated to reflect the committed changes.

This is correct. That target is provided by find_package(ZLIB). In LLVMConfig.cmake, we invoke find_package(ZLIB) when zlib support is enabled. If you're using LLVMExports.cmake, you'll need to invoke find_package(ZLIB) yourself.

@phosek, Thanks for the reply. I am not too familiar with this. Could you please elaborate more on why setting it to ZLIB::ZLIB here is more modern? And how one is expected to deal with ZLIB::ZLIB that shows up in LLVMExports.cmake` (in the modern way)? Should one string pattern matching for ZLIB::ZLIB and invoke find_package(ZLIB) again? (previously one could simply concatenate -l with the library name in there.

@lxfind ZLIB::ZLIB is the name of the imported target that's defined by CMake when you invoke find_dependency(ZLIB). Ideally, you'd just use LLVMConfig.cmake which invokes that for you if needed. https://pabloariasal.github.io/2018/02/19/its-time-to-do-cmake-right/ has some more details.

ZLIB::ZLIB includes the correct include and library directory, so you no longer have to manually append -I or -l, all you need is to include ZLIB::ZLIB in the list of your dependencies, which is one advantage. Another advantage is that ZLIB::ZLIB can expand to different things, for example some users may want to statically link zlib in which case they would configure their build accordingly and ZLIB::ZLIB would expand into /path/to/libz.a (this is for example what we do in our toolchain build), others may be fine using the default system library in which case ZLIB::ZLIB would typically expand to just z.

@phosek sorry for the late reply, the builds on master are still green, so your changes are working on Windows as well.

Thank you for the extra work!

Great, one benefit of this is that zlib can now be detected in non-system libs. Maybe we should handle ncurses / TERMINFO in a similar manner? It currently has similar logic as finding zlib had

haampie added inline comments.Aug 18 2020, 3:09 PM
llvm/lib/Support/CMakeLists.txt
217

This will not work on macOS since cmake will find stub libraries by default (/path/to/libz.tbd), see https://github.com/Kitware/CMake/blob/master/Modules/Platform/Darwin.cmake#L71 for the search order.

Instead you most likely want to use CMAKE_FIND_LIBRARY_PREFIXES and CMAKE_FIND_LIBRARY_SUFFIXES

@phosek in MSYS2 (targeting x86_64-w64-windows-gnu) Zlib works properly for LLVM 10 but with master I'm now seeing:

-- Constructing LLVMBuild project information
-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a
CMake Error at lib/Support/CMakeLists.txt:9 (string):
  string sub-command REGEX, mode REPLACE: regex "^(lib|)" matched an empty
  string.
Call Stack (most recent call first):
  lib/Support/CMakeLists.txt:226 (get_system_libname)

-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a was printed by my change to help debugging it.
FYI zlib_library is set here: https://github.com/llvm/llvm-project/blob/8e06bf6b3a2e8d25e56cd52dca0cf3ff1b37b5d1/llvm/lib/Support/CMakeLists.txt#L218

@phosek in MSYS2 (targeting x86_64-w64-windows-gnu) Zlib works properly for LLVM 10 but with master I'm now seeing:

-- Constructing LLVMBuild project information
-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a
CMake Error at lib/Support/CMakeLists.txt:9 (string):
  string sub-command REGEX, mode REPLACE: regex "^(lib|)" matched an empty
  string.
Call Stack (most recent call first):
  lib/Support/CMakeLists.txt:226 (get_system_libname)

-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a was printed by my change to help debugging it.
FYI zlib_library is set here: https://github.com/llvm/llvm-project/blob/8e06bf6b3a2e8d25e56cd52dca0cf3ff1b37b5d1/llvm/lib/Support/CMakeLists.txt#L218

Looks like that's an issue introduced by D86134 or D86245. Can you print the value of ${CMAKE_FIND_LIBRARY_PREFIXES} and ${CMAKE_FIND_LIBRARY_SUFFIXES} in your build?

@phosek

Looks like that's an issue introduced by D86134 or D86245.

Indeed, I apologize for bothering you. Should I move the discussion to one of patches created by @haampie?

Continuing the discussion with if( CMAKE_FIND_LIBRARY_PREFIXES ) and if( CMAKE_FIND_LIBRARY_SUFFIXES ) from https://reviews.llvm.org/D86245 changed to false llvm-config --system-libs prints -llibz.dll.a so we need those replaces to bring back correct -lz value.

Can you print the value of ${CMAKE_FIND_LIBRARY_PREFIXES} and ${CMAKE_FIND_LIBRARY_SUFFIXES} in your build?

-- CMAKE_FIND_LIBRARY_PREFIXES=lib;
-- CMAKE_FIND_LIBRARY_SUFFIXES=.dll.a;.a;.lib

Which matches https://gitlab.kitware.com/cmake/cmake/-/blob/269d1a86249ea037a2884133daffc8c44a38d926/Modules/Platform/Windows-GNU.cmake