[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. In D79219#2231927 , @mati865 wrote: > @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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

mati865 added a comment. @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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

mati865 added a comment. @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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

haampie added inline comments. Comment at: llvm/lib/Support/CMakeLists.txt:217 + endif() + if(CMAKE_SHARED_LIBRARY_PREFIX AND CMAKE_SHARED_LIBRARY_SUFFIX AND + zlib_library MATCHES "^${CMAKE_SHARED_LIBRARY_PREFIX}.*${CMAKE_SHARED_LIBRARY_SUFFIX}$") This

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. In D79219#2203887 , @lxfind wrote: > In D79219#2201415 , @phosek wrote: > >> This is correct. That target is provided by `find_package(ZLIB)`. In >> LLVMConfig.cmake, we invoke

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

haampie added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

kuhnel added a comment. @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! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG31e5f7120bdd: [CMake] Simplify CMake handling for zlib (authored by phosek). Repository: rG LLVM Github Monorepo

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. @kuhnel do you want to take a look or is it okay if I just submit the change? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 ___ cfe-commits mailing list

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGccbc1485b55f: [CMake] Simplify CMake handling for zlib (authored by phosek). Repository: rG LLVM Github Monorepo

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 284161. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in compiler-rt/test/lit.common.configured.in lld/test/CMakeLists.txt

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. 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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 284106. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in compiler-rt/test/lit.common.configured.in lld/test/CMakeLists.txt

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

lxfind added a comment. In D79219#2201415 , @phosek wrote: > In D79219#2201109 , @lxfind wrote: > >> @phosek, Under this change, now when I build LLVM (with a basic config >> `cmake -G Ninja

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. In D79219#2201994 , @kuhnel wrote: > This patch broke the Windows compilation on buildbot > and pre-merge > testing

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

kuhnel added a comment. > 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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

kuhnel added a comment. In D79219#2202048 , @phosek wrote: > 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:

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

kuhnel added a comment. 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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. In D79219#2201109 , @lxfind wrote: > @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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. How can I test this change on pre-merge bots? I haven't seen any builds posted on this change before. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

lxfind added a comment. @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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

hans added inline comments. Comment at: llvm/cmake/config-ix.cmake:178 -if (LLVM_ENABLE_ZLIB STREQUAL "FORCE_ON" AND NOT HAVE_LIBZ) - message(FATAL_ERROR "Failed to configure zlib") Could this check be put back? E.g. for now it seems building with

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

This revision was automatically updated to reflect the committed changes. Closed by commit rG1adc494bce44: [CMake] Simplify CMake handling for zlib (authored by phosek). Changed prior to commit: https://reviews.llvm.org/D79219?vs=283398=283434#toc Repository: rG LLVM Github Monorepo

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek reopened this revision. phosek added a comment. This revision is now accepted and ready to land. Sorry about the breakage, that was an unintentional change. I have updated the patch and restored the original behavior on Windows. CHANGES SINCE LAST ACTION

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 283398. phosek marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in compiler-rt/test/lit.common.configured.in

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

hans added a comment. Reverted in 3ab01550b632dad46f9595d74855749557ffd25c Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

hans added a comment. In D79219#2153585 , @phosek wrote: > In D79219#2152747 , @labath wrote: > >> I wouldn't mind separate (internal) cache variable, though I am somewhat >> surprised by this problem. A

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek marked 4 inline comments as done. phosek added inline comments. Comment at: mlir/examples/standalone/CMakeLists.txt:35 +endif() + include(TableGen) stephenneuendorffer wrote: > mehdi_amini wrote: > > mehdi_amini wrote: > > > I am a bit unsure that it is

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

stephenneuendorffer added inline comments. Comment at: mlir/examples/standalone/CMakeLists.txt:35 +endif() + include(TableGen) mehdi_amini wrote: > mehdi_amini wrote: > > I am a bit unsure that it is desirable that it is desirable to need this > > change? > >

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

mehdi_amini added inline comments. Comment at: mlir/examples/standalone/CMakeLists.txt:35 +endif() + include(TableGen) mehdi_amini wrote: > I am a bit unsure that it is desirable that it is desirable to need this > change? > This requires every single user of

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

mehdi_amini added inline comments. Comment at: mlir/examples/standalone/CMakeLists.txt:35 +endif() + include(TableGen) 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

Re: [PATCH] D79219: [CMake] Simplify CMake handling for zlib

Thanks, that addressed the issue. > On Jul 24, 2020, at 1:41 PM, Petr Hosek wrote: > > Thanks for the heads up, this should be addressed by > c86f56e32e724c6018e579bb2bc11e667c96fc96, let me know if there are other > issues. > > On Fri, Jul 24, 2020 at 12:33 PM Vedant Kumar via Phabricator

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

vsk added a comment. @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

Re: [PATCH] D79219: [CMake] Simplify CMake handling for zlib

Thanks for the heads up, this should be addressed by c86f56e32e724c6018e579bb2bc11e667c96fc96, let me know if there are other issues. On Fri, Jul 24, 2020 at 12:33 PM Vedant Kumar via Phabricator < revi...@reviews.llvm.org> wrote: > vsk added a comment. > > @phosek I suspect this is causing a

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

This revision was automatically updated to reflect the committed changes. Closed by commit rG10b1b4a231a4: [CMake] Simplify CMake handling for zlib (authored by phosek). Herald added subscribers: msifontes, jurahul, Kayjukh, grosul1, Joonsoo, stephenneuendorffer, liufengdb, aartbik, lucyrfox,

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 280305. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/CMakeLists.txt clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in compiler-rt/test/lit.common.configured.in

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

This revision was automatically updated to reflect the committed changes. Closed by commit rG1d09ecf36175: [CMake] Simplify CMake handling for zlib (authored by phosek). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. In D79219#2152747 , @labath wrote: > 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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 278216. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/CMakeLists.txt clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in compiler-rt/test/lit.common.configured.in

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

labath added a comment. 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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek reopened this revision. phosek added a comment. This revision is now accepted and ready to land. 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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

This revision was automatically updated to reflect the committed changes. Closed by commit rG8c1a79dc12f3: [CMake] Simplify CMake handling for zlib (authored by phosek). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

smeenai accepted this revision. smeenai added a comment. This revision is now accepted and ready to land. Oops, missed that I'd become a blocking reviewer for this (cos of requesting changes before). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

JDevlieghere accepted this revision. JDevlieghere added a comment. Thanks Petr, LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 274255. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/CMakeLists.txt clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in compiler-rt/test/lit.common.configured.in

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek marked an inline comment as done. phosek added inline comments. Comment at: llvm/cmake/config-ix.cmake:514 + if(ZLIB_FOUND) +set(LLVM_ENABLE_ZLIB "YES" CACHE STRING + "Use zlib for compression/decompression if available. Can be ON, OFF, or FORCE_ON"

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 274256. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/CMakeLists.txt clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in compiler-rt/test/lit.common.configured.in

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

smeenai requested changes to this revision. smeenai added a comment. This revision now requires changes to proceed. Requesting changes to remove from my review queue while the above comments are being addressed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

labath added inline comments. Comment at: llvm/cmake/config-ix.cmake:514 + if(ZLIB_FOUND) +set(LLVM_ENABLE_ZLIB "YES" CACHE STRING + "Use zlib for compression/decompression if available. Can be ON, OFF, or FORCE_ON" JDevlieghere wrote: > phosek wrote:

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

JDevlieghere added inline comments. Comment at: llvm/cmake/config-ix.cmake:514 + if(ZLIB_FOUND) +set(LLVM_ENABLE_ZLIB "YES" CACHE STRING + "Use zlib for compression/decompression if available. Can be ON, OFF, or FORCE_ON" phosek wrote: > JDevlieghere

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. In D79219#2029520 , @JDevlieghere wrote: > 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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 263288. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/CMakeLists.txt clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in compiler-rt/test/lit.common.configured.in

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek updated this revision to Diff 263284. phosek marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 Files: clang/CMakeLists.txt clang/test/CMakeLists.txt clang/test/lit.site.cfg.py.in

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

JDevlieghere added a comment. 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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

smeenai added inline comments. Comment at: lld/test/lit.site.cfg.py.in:16 config.target_triple = "@TARGET_TRIPLE@" -config.python_executable = "@Python3_EXECUTABLE@" -config.have_zlib = @HAVE_LIBZ@ +config.python_executable = "@PYTHON_EXECUTABLE@" +config.have_zlib =

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek added a comment. Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

phosek created this revision. phosek added reviewers: smeenai, compnerd. Herald added subscribers: llvm-commits, lldb-commits, Sanitizers, cfe-commits, hiraditya, mgorny. Herald added projects: clang, Sanitizers, LLDB, LLVM. Rather than handling zlib handling manually, use `find_package` from