-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build with zlib on Unix #456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This likely brings a dependency on zlib to objwriter as well (i.e. this is also a product change). The pre-existing -DLLVM_ENABLE_ZLIB=OFF
on Windows is there because LLVM will bring in something that depends on zlib if left unchecked, at least on Windows.
I don't have big concerns around it since ilc
already seems to depend on zlib on Linux anyway, but calling it out.
I assume mac is fine and ships with a zlib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
From the docs: https://llvm.org/docs/HowToCrossCompileLLVM.html
which (after trying out different ways to satisfy cmake's "find_package() config-mode" on CBL-mariner without modifying the LLVM source files) translates to my second commit. Lets see how it goes. 🤞😅 |
should be |
Switching images with rootfs to use |
llvm/cmake/config-ix.cmake
Outdated
if("${ZLIB_ROOT}" STREQUAL "") | ||
find_package(ZLIB REQUIRED) | ||
else() | ||
include(FindZLIB) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in https://reviews.llvm.org/D70519 there was an attempt to use FindZLIB and it was noted it changes the value in llvm-config --system-libs
, is that still the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will select $ZLIB_ROOT/libz.so
which is enough for our purposes. As long as RPATH is correct (which I haven't checked as full cross-build was taking ages on my machine), it will load on any system with libz. On NativeAOT, libz is a system prerequisite on build machine, so ilc will be ok.
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Good to merge? |
@directhex @akoeplinger any concerns from Mono side? This is changing build configuration everywhere. It looks good from CoreCLR test tools/objwriter perspective. |
Cool 👍 (assuming it works 😅) |
Thank you! |
Per #990 we would like to avoid installing `sudo` onto our build images if possible. This was added for dotnet/llvm-project#456, which I believe didn't end up using `sudo`.
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
llvm-dwarfdump requires zlib support in dotnet/runtime#85192 (comment). It doesn't hurt having it enabled in the other tools (distro, such as Ubuntu, have it enabled for llvm-*).