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.
Please take a look at D70519 for the issues with this approach.
Also, while I do agree with you that we should not auto-select dependencies, I think this runs contrary to the llvm philosophy that the default built should "just work" (I don't know if that's formalized anywhere, but I know it's certainly what (some?) people expect).
@labath I think you are misunderstanding the patch. This is not autoselecting the dependencies. It is simply doing that based on an existing option that we have - LLVM_ENABLE_ZLIB. We could always search for zlib and override the results with LLVM_ENABLE_ZLIB as well. The current build will continue to just work - zlib is used only for the compressed debug sections (which requires the user to opt-in).
Sorry, didn't see the question. From my local build:
$ ./bin/llvm-config --system-libs /usr/lib/x86_64-linux-gnu/libz.so -lrt -ldl -ltinfo -lpthread -lm /usr/lib/x86_64-linux-gnu/libxml2.so
/usr/lib/x86_64-linux-gnu/libz.so is definitely better than -l/usr/lib/x86_64-linux-gnu/libz.so, as it's at least a valid link line. It's not completely equivalent, and may not work in all cases -- last time I accidentally changed this, I got about a dozen emails from people I broke, and I wouldn't be surprised if one of them depended on the subtle differences here. However, the fact that libxml is also spelled out this way gives me some hope...
That said, this still leaves us with the "problem" that the default cmake configuration will be broken for people who don't have zlib installed (pretty much everyone on windows, at least). You can try that out by deleting/renaming/uninstalling zlib.h from your system. With this patch you get a configuration error whereas previously zlib support was automatically disabled. Whether this "problem" is a problem or "working as intended" depends on what do you expect out of your build system, but it's definitely not consistent with how other llvm dependencies are managed.
Anyway, I don't really have a horse in this race, but I figured I should at least warn you about the problems you're likely to run into. (and I really wish @beanz would say something here, as he's the one who's complaining about build system inconsistencies...)
this is now in master, and I am seeing build failures in cross-building clang, e.g. when building clang for arm on a x86_64 host. its resorting to finding, libz from buildhost instead of target sysroot ( using --sysroot) and failing in link step. e.g.
.... -o bin/llvm-config -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.a /usr/lib/libz.so -lrt -ldl -ltinfo -lm lib/libLLVMDemangle.a
aarch64-yoe-linux-musl/aarch64-yoe-linux-musl-ld: /usr/lib/libz.so: error adding symbols: file in wrong format
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)
you can see that its adding /usr/lib/libz.so to linker cmdline while cross linking.
That is not the problem. Since in cases of target its finding is properly.
there are multiple pieces where some are being built for native( build host) some for target so it thinks its building for
buildhost (native) llvm-config e.g. is one such case. Replacing it with -lz fixes it becasue compile environment is providing
right --sysroot for native or cross cases and it always links with correct libs.