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

Wed Aug 5 03:12:17 PDT 2020

hans added a comment.

In D79219#2153585 <https://reviews.llvm.org/D79219#2153585>, @phosek wrote:

> In D79219#2152747 <https://reviews.llvm.org/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 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79219/new/

https://reviews.llvm.org/D79219