This is an archive of the discontinued LLVM Phabricator instance.

[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

phosek created this revision.Apr 30 2020, 5:22 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptApr 30 2020, 5:22 PM
Herald added subscribers: llvm-commits, lldb-commits, Restricted Project and 3 others. · View Herald Transcript
smeenai added inline comments.May 8 2020, 2:07 PM
lld/test/lit.site.cfg.py.in
17

This seems like an unintended/unrelated change.

llvm/cmake/config-ix.cmake
523–524

How come you're moving this out of the MSan check block? Isn't that a behavior change?

llvm/lib/Support/CMakeLists.txt
209

Even if there's no prefix, wouldn't you still wanna strip the suffix?

JDevlieghere added a subscriber: JDevlieghere.EditedMay 11 2020, 8:50 AM

I'm in favor of this change. I'm not too happy with how this works in CMake, I've expressed similar concerns when the FORCE_ON approach was suggested in D71306.

I really like what we ended up with in LLDB. The TL;DR is that we have 3 values: ON, OFF and Auto. The later will resolve to either ON or OFF depending on whether the dependency was found. The "clever" part is that we use shadowing to avoid having another CMake variable without overwriting the cache.

Here's what it looks like in LLDB: https://github.com/llvm/llvm-project/blob/3df40007e6317955176dff0fa9e0b029dc4a11d1/lldb/cmake/modules/LLDBConfig.cmake#L26

Maybe we can consider something similar here? I'd be more than happy to hoist the corresponding CMake into llvm.

(edit: added myself as a reviewer so this shows up in my review queue)

llvm/cmake/config-ix.cmake
531

If I understand correctly, after running the configuration phase with LLVM_ENABLE_ZLIB to FORCE_ON, the cached value will be overwritten by ON?

phosek updated this revision to Diff 263284.May 11 2020, 2:48 PM
phosek marked 2 inline comments as done.

I'm in favor of this change. I'm not too happy with how this works in CMake, I've expressed similar concerns when the FORCE_ON approach was suggested in D71306.

I really like what we ended up with in LLDB. The TL;DR is that we have 3 values: ON, OFF and Auto. The later will resolve to either ON or OFF depending on whether the dependency was found. The "clever" part is that we use shadowing to avoid having another CMake variable without overwriting the cache.

Here's what it looks like in LLDB: https://github.com/llvm/llvm-project/blob/3df40007e6317955176dff0fa9e0b029dc4a11d1/lldb/cmake/modules/LLDBConfig.cmake#L26

Maybe we can consider something similar here? I'd be more than happy to hoist the corresponding CMake into llvm.

(edit: added myself as a reviewer so this shows up in my review queue)

I'm fine either way, to me ON/OFF/AUTO and FORCE_ON/ON/OFF is just a different way to spell the same thing, my only concern would be breaking existing users who might be already relying on FORCE_ON.

llvm/cmake/config-ix.cmake
523–524

This was done in the original change, I'm not quite sure what's the motivation, @compnerd do you know?

531

Correct, we used FORCE_ON above when invoking find_package setting REQUIRED which makes the check fail if the library is missing. Recording this information is important for LLVM consumers because it'll be stored in LLVMConfig.cmake and AFAIU we don't want to propagate FORCE_ON there.

phosek updated this revision to Diff 263288.May 11 2020, 2:53 PM
JDevlieghere added inline comments.May 11 2020, 3:01 PM
llvm/cmake/config-ix.cmake
531

I get that. My worry is that if for whatever reason the library disappears (system upgrade, package removal, ...) this will silently disable ZLIB support because now LLVM_ENABLE_ZLIB just equals on. This might sound far fetched, but happens all the time with "internal installs". This is why I prefer the ON/OFF/Auto approach because it doesn't update the cache variable, but would set the internal variable to either ON or OFF.

labath added a subscriber: labath.May 12 2020, 12:13 AM
labath added inline comments.
llvm/cmake/config-ix.cmake
531

I agree with Jonas, though I don't think the actual values (FORCE_ON/ON vs. ON/Auto) really matter here. The important part is not overwriting the cache variables specified by the user, as that can create various problems with reconfigurations (like the zlib becoming unavailable example).

smeenai requested changes to this revision.May 13 2020, 2:25 PM

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