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" ---------------- labath wrote: > JDevlieghere wrote: > > phosek wrote: > > > 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. > > 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. > 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). I've updated the change to shadow the variable as suggested by Jonas. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
