phosek added a comment. In D79219#2029520 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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. ================ Comment at: llvm/cmake/config-ix.cmake:506 -if (LLVM_ENABLE_ZLIB ) - # Check if zlib is available in the system. - if ( NOT HAVE_ZLIB_H OR NOT HAVE_LIBZ ) - set(LLVM_ENABLE_ZLIB 0) +if(LLVM_ENABLE_ZLIB) + if(LLVM_ENABLE_ZLIB STREQUAL FORCE_ON) ---------------- smeenai wrote: > How come you're moving this out of the MSan check block? Isn't that a > behavior change? This was done in the original change, I'm not quite sure what's the motivation, @compnerd do you know? ================ 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: > If I understand correctly, after running the configuration phase with > LLVM_ENABLE_ZLIB to FORCE_ON, the cached value will be overwritten by ON? 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
