Support building phobos against a system copy of zlib #4742
Conversation
|
I have now seen that #4716 already existed, but compared to it, this PR correctly handles linking to static phobos and it doesn't affect the druntime libraries at the cost of slightly changing the compiler linker code. |
|
I think it would be beneficial to add a changelog entry and a CI run for this option. Is circleci the preferred runner? |
|
Thanks for doing this; LGTM after a superficial glance. Yeah some CI coverage would be great; CircleCI is just kept because it doesn't hurt/get in the way; Cirrus is unfortunately not a sufficient free option anymore, so in essence it's all GitHub's own CI now, they managed to kill all competitors (for open-source projects) unfortunately (edit: and with the help of crypto-kiddies). You could e.g. select one of the jobs in https://github.com/ldc-developers/ldc/blob/master/.github/workflows/supported_llvm_versions.yml; we use them for testing some CMake permutations too. |
| @@ -61,6 +61,10 @@ if (RT_SUPPORT_SANITIZERS) | |||
| list(APPEND D_FLAGS -d-version=SupportSanitizers) | |||
| endif() | |||
|
|
|||
| if(PHOBOS_SYSTEM_ZLIB) | |||
There was a problem hiding this comment.
Ideally, we'd bake this into the ldc-build-runtime CMake invocations too, in
ldc/runtime/ldc-build-runtime.d.in
Lines 152 to 157 in 0cc531e
CHANGELOG.md
Outdated
| @@ -5,6 +5,7 @@ | |||
| - LLVM for prebuilt packages bumped to v18.1.8 (incl. macOS arm64). (#4712) | |||
| - Android: NDK for prebuilt package bumped from r26d to r27. (#4711) | |||
| - ldc2.conf: %%ldcconfigpath%% placeholder added - specifies the directory where current configuration file is located. (#4717) | |||
| - Add support for building against a system copy of zlib through `-DPHOBOS_SYSTEM_ZLIB=ON` (#4742) | |||
There was a problem hiding this comment.
When you're revising this PR, please add a trailing full stop here.
CMakeLists.txt
Outdated
| @@ -149,6 +149,9 @@ set(LDC_ENABLE_ASSERTIONS "${LLVM_ENABLE_ASSERTIONS}" CACHE BOOL "Enable LDC ass | |||
| # Allow user to specify mimalloc.o location, to be linked with `ldc2` only | |||
| set(ALTERNATIVE_MALLOC_O "" CACHE STRING "If specified, adds ALTERNATIVE_MALLOC_O object file to LDC link, to override the CRT malloc.") | |||
|
|
|||
| # Most linux distributions have a policy of not bundling dependencies like zlib | |||
| set(PHOBOS_SYSTEM_ZLIB OFF CACHE BOOL "Use the system copy of zlib when building phobos") | |||
There was a problem hiding this comment.
I'd probably slightly rephrase this, something like 'use system zlib instead of Phobos' vendored version'.
Note that the D bindings have hardcoded zlib version numbers which might be off then: https://github.com/dlang/phobos/blob/f7e523bc3c69506c3e7b4da7e78af93ed8547910/etc/c/zlib.d#L45-L47
The actual API has been pretty stable AFAIK, so I don't think this matters in practice.
There was a problem hiding this comment.
I've asked in the Gentoo irc rooms what would are the appropriate version of zlib that we can use given that we hardcode the headers to a certain version and the response was that, given it's zlib, so long as we link against libz.so.1 it should be fine.
If we want to restrict the version we can do that inside the find_package call so if we end up with an incompatible version of zlib in the future we can fail during configuration. For now I think that we shouldn't overly restrict the check unless we know for certain that a zlib version doesn't work.
This is achieved by linking the dynamic variant of phobos against zlib and having the compiler add -lz when linking an application that embeds the static variant of phobos. Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
|
Rebased onto master to fix the macos xcode failure. |
Many Linux distributions including Debian, Fedora, and, Gentoo, have a policy of unbundling 3rd party dependencies like zlib from software projects. This PR implements support for this so that distributions don't have to implement the same logic individually.
Currently Debian does remove the zlib code but they leave the static variants of libphobos with undefined symbols to zlib functions making
-link-defaultlib-shared=falserequire the additional-lzswitch when specified by users. Fedora tried a patch but it resulted in the build being broken so it has been reverted. Gentoo doesn't do anything currently but I am making this PR instead.These changes are all behind the
-DPHOBOS_SYSTEM_ZLIB=TRUEcmake argument so the current code will continue to behave the same and the dependency on zlib is strictly opt-in. A possible unwanted interaction is that, when cross-compiling, ldc2 will try to link-lzjust like it would when building for the build host but zlib may not exist on the target system. The only solution for this is specifying-defaultlib= -L-l:libphobos2-ldc.a -L-l:libdruntime-ldc.awhich is a mouthful but it is either this or complicating the linker code more.An implementation downside of the code is that the phobos config option is placed inside the root CMakeLists.txt because the compiler needs to know whether it needs to links zlib when building statically and, since it is built with a custom_target its target compile arguments can not be modified through something like
target_compile_definitionsafter the runtime subdirectory has been included.What this PR doesn't support is
MULTILIB=ONandPHOBOS_SYSTEM_ZLIB=TRUEsimultaneously because cmake, or any other build system that I know of, doesn't allow to specify the ABI when looking for a package. The standard way of compiling for multiple ABIs is to setup a different build directory and configuring cmake for that ABI which is done by setting PKG_CONFIG_PATH to point to the correct libdir and setting environment variables for the toolchain similar toCC="gcc -m32" CXX="g++ -m32" DMD="ldmd -m32". The current cmake code worked for this with only minor changes but I didn't dive in too deeply yet.I've made plenty of last minute improvements that ended up breaking something so it's fine if this PR waits for the new upcoming release to happen first and then be merged.