Skip to content
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] Some improvements to the LLVM build system #55354

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

giordano
Copy link
Contributor

@giordano giordano commented Aug 2, 2024

Elaboration of the issue

After #55180 we implicitly require an LLVM built with Zlib support, but compiling Julia with make USE_BINARYBUILDER_LLVM=0 builds an LLVM without Zlib support, despite the fact we attempt to request it at

LLVM_CMAKE += -DLLVM_ENABLE_ZLIB=ON -DZLIB_LIBRARY="$(build_prefix)/lib"

This was first identified in #55337.

Explanation of how configuration of LLVM failed

ZLIB_LIBRARY must be the path to the zlib library, but we currently set it to the libdir where the library is installed (introduced in #42622):

LLVM_CMAKE += -DLLVM_ENABLE_ZLIB=ON -DZLIB_LIBRARY="$(build_prefix)/lib"

which is wrong. However, CMake is actually able to find Zlib correctly, but then the check at https://github.com/llvm/llvm-project/blob/46425b8d0fac3c529aa4a716d19abd7032e452f3/llvm/cmake/config-ix.cmake#L139-L141 uses the value of ZLIB_LIBRARY to list the Zlib to link for the test, but being ZLIB_LIBRARY a directory, CMake doesn't see any valid Zlib and thus tries to run the test without linking any Zlib, and the test silently fails (they're silent only when LLVM_ENABLE_ZLIB=ON), resulting in no usable Zlib available, even if found.

Proposed solution

ZLIB_ROOT is the only hint recommended by the CMake module FindZLIB. This PR replaces a broken ZLIB_LIBRARY with an appropriate ZLIB_ROOT. Also, we set LLVM_ENABLE_ZLIB=FORCE_ON which is the only way to make CMake fail loudly if no usable Zlib is available, and avoid going on with a non-usable build.

Other comments

I debugged this together with @gbaraldi, thanks!.

I confirm this fixes #55337 for me, it should likely address JuliaCI/julia-buildkite#373 as well.

Also, options COMPILER_RT_ENABLE_IOS, COMPILER_RT_ENABLE_WATCHOS, COMPILER_RT_ENABLE_TVOS, and HAVE_HISTEDIT_H don't exist anymore, and they are removed.

`ZLIB_LIBRARY` must be the path to the zlib library, but we currently set it to
the libdir where the library is installed, which is wrong.  Also, `ZLIB_ROOT` is
the only hint recommended by the CMake module `FindZLIB`:
<https://cmake.org/cmake/help/latest/module/FindZLIB.html#hints>.
@giordano giordano added domain:building Build system, or building Julia or its dependencies backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Aug 2, 2024
deps/llvm.mk Outdated Show resolved Hide resolved
This makes tests to check zlib is usable error out loudly, instead of silently
proceeding with a broken build.
@gbaraldi
Copy link
Member

gbaraldi commented Aug 5, 2024

#42622 was done so that the nix build worked as expected so @rbvermaa could you test if this breaks the Nix from source workflow?

@rbvermaa
Copy link
Contributor

rbvermaa commented Aug 5, 2024

#42622 was done so that the nix build worked as expected so @rbvermaa could you test if this breaks the Nix from source workflow?

Ran into another issue with curl, but that seems unrelated, and I will dig into it. LLVM seems to build fine, so should be good to go from our side, thanks for the ping!

@giordano giordano merged commit b0a8024 into JuliaLang:master Aug 6, 2024
10 checks passed
@giordano giordano deleted the mg/llvm-cmake branch August 6, 2024 08:46
giordano added a commit that referenced this pull request Aug 6, 2024
After #55180 we implicitly require an LLVM built with Zlib support, but
compiling Julia with `make USE_BINARYBUILDER_LLVM=0` builds an LLVM
without Zlib support, despite the fact we attempt to request it at

https://github.com/JuliaLang/julia/blob/996351f5f6651d1508aef3c35c7d37eb22a0fb1e/deps/llvm.mk#L97
This was first identified in #55337.

`ZLIB_LIBRARY` must be the path to the zlib library, but we currently
set it to the libdir where the library is installed (introduced in

https://github.com/JuliaLang/julia/blob/996351f5f6651d1508aef3c35c7d37eb22a0fb1e/deps/llvm.mk#L97
which is wrong. However, CMake is actually able to find Zlib correctly,
but then the check at
https://github.com/llvm/llvm-project/blob/46425b8d0fac3c529aa4a716d19abd7032e452f3/llvm/cmake/config-ix.cmake#L139-L141
uses the value of `ZLIB_LIBRARY` to list the Zlib to link for the test,
but being `ZLIB_LIBRARY` a directory, CMake doesn't see any valid Zlib
and thus tries to run the test without linking any Zlib, and the test
silently fails (they're silent only when `LLVM_ENABLE_ZLIB=ON`),
resulting in no usable Zlib available, even if found.

`ZLIB_ROOT` is the only [hint recommended by the CMake module
`FindZLIB`](https://cmake.org/cmake/help/latest/module/FindZLIB.html#hints).
This PR replaces a broken `ZLIB_LIBRARY` with an appropriate
`ZLIB_ROOT`. Also, we set `LLVM_ENABLE_ZLIB=FORCE_ON` which is the only
way to make CMake fail loudly if no usable Zlib is available, and avoid
going on with a non-usable build.

I confirm this fixes #55337 for me, it should likely address
JuliaCI/julia-buildkite#373 as well.

Also, options `COMPILER_RT_ENABLE_IOS`, `COMPILER_RT_ENABLE_WATCHOS`,
`COMPILER_RT_ENABLE_TVOS`, and `HAVE_HISTEDIT_H` don't exist anymore,
and they are removed.
giordano added a commit that referenced this pull request Aug 6, 2024
After #55180 we implicitly require an LLVM built with Zlib support, but
compiling Julia with `make USE_BINARYBUILDER_LLVM=0` builds an LLVM
without Zlib support, despite the fact we attempt to request it at

https://github.com/JuliaLang/julia/blob/996351f5f6651d1508aef3c35c7d37eb22a0fb1e/deps/llvm.mk#L97
This was first identified in #55337.

`ZLIB_LIBRARY` must be the path to the zlib library, but we currently
set it to the libdir where the library is installed (introduced in

https://github.com/JuliaLang/julia/blob/996351f5f6651d1508aef3c35c7d37eb22a0fb1e/deps/llvm.mk#L97
which is wrong. However, CMake is actually able to find Zlib correctly,
but then the check at
https://github.com/llvm/llvm-project/blob/46425b8d0fac3c529aa4a716d19abd7032e452f3/llvm/cmake/config-ix.cmake#L139-L141
uses the value of `ZLIB_LIBRARY` to list the Zlib to link for the test,
but being `ZLIB_LIBRARY` a directory, CMake doesn't see any valid Zlib
and thus tries to run the test without linking any Zlib, and the test
silently fails (they're silent only when `LLVM_ENABLE_ZLIB=ON`),
resulting in no usable Zlib available, even if found.

`ZLIB_ROOT` is the only [hint recommended by the CMake module
`FindZLIB`](https://cmake.org/cmake/help/latest/module/FindZLIB.html#hints).
This PR replaces a broken `ZLIB_LIBRARY` with an appropriate
`ZLIB_ROOT`. Also, we set `LLVM_ENABLE_ZLIB=FORCE_ON` which is the only
way to make CMake fail loudly if no usable Zlib is available, and avoid
going on with a non-usable build.

I confirm this fixes #55337 for me, it should likely address
JuliaCI/julia-buildkite#373 as well.

Also, options `COMPILER_RT_ENABLE_IOS`, `COMPILER_RT_ENABLE_WATCHOS`,
`COMPILER_RT_ENABLE_TVOS`, and `HAVE_HISTEDIT_H` don't exist anymore,
and they are removed.
@giordano giordano removed backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Aug 6, 2024
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
### Elaboration of the issue

After JuliaLang#55180 we implicitly require an LLVM built with Zlib support, but
compiling Julia with `make USE_BINARYBUILDER_LLVM=0` builds an LLVM
without Zlib support, despite the fact we attempt to request it at

https://github.com/JuliaLang/julia/blob/996351f5f6651d1508aef3c35c7d37eb22a0fb1e/deps/llvm.mk#L97
This was first identified in JuliaLang#55337.

### Explanation of how configuration of LLVM failed

`ZLIB_LIBRARY` must be the path to the zlib library, but we currently
set it to the libdir where the library is installed (introduced in
JuliaLang#42622):

https://github.com/JuliaLang/julia/blob/996351f5f6651d1508aef3c35c7d37eb22a0fb1e/deps/llvm.mk#L97
which is wrong. However, CMake is actually able to find Zlib correctly,
but then the check at
https://github.com/llvm/llvm-project/blob/46425b8d0fac3c529aa4a716d19abd7032e452f3/llvm/cmake/config-ix.cmake#L139-L141
uses the value of `ZLIB_LIBRARY` to list the Zlib to link for the test,
but being `ZLIB_LIBRARY` a directory, CMake doesn't see any valid Zlib
and thus tries to run the test without linking any Zlib, and the test
silently fails (they're silent only when `LLVM_ENABLE_ZLIB=ON`),
resulting in no usable Zlib available, even if found.

### Proposed solution

`ZLIB_ROOT` is the only [hint recommended by the CMake module
`FindZLIB`](https://cmake.org/cmake/help/latest/module/FindZLIB.html#hints).
This PR replaces a broken `ZLIB_LIBRARY` with an appropriate
`ZLIB_ROOT`. Also, we set `LLVM_ENABLE_ZLIB=FORCE_ON` which is the only
way to make CMake fail loudly if no usable Zlib is available, and avoid
going on with a non-usable build.

### Other comments

I confirm this fixes JuliaLang#55337 for me, it should likely address
JuliaCI/julia-buildkite#373 as well.

Also, options `COMPILER_RT_ENABLE_IOS`, `COMPILER_RT_ENABLE_WATCHOS`,
`COMPILER_RT_ENABLE_TVOS`, and `HAVE_HISTEDIT_H` don't exist anymore,
and they are removed.
@NickCao
Copy link

NickCao commented Aug 28, 2024

#42622 was done so that the nix build worked as expected so @rbvermaa could you test if this breaks the Nix from source workflow?

Nix build works fine! One issue is libunwind also depends on zlib but is not configured with the correct search path?

NickCao added a commit to NickCao/nixpkgs that referenced this pull request Aug 28, 2024
Bundled libunwind still uses system zlib even without USE_SYSTEM_ZLIB
set, so instead make all libraries use system zlib

Reference: JuliaLang/julia#55354
@giordano
Copy link
Contributor Author

Can you provide more information? For libunwind we use autotools, this is the configure call:

$(dir $<)/configure $(CONFIGURE_COMMON) CPPFLAGS="$(CPPFLAGS) $(LIBUNWIND_CPPFLAGS)" CFLAGS="$(CFLAGS) $(LIBUNWIND_CFLAGS)" --enable-shared --disable-minidebuginfo --disable-tests --enable-zlibdebuginfo --disable-conservative-checks --enable-per-thread-cache
and CONFIGURE_COMMON includes the prefix setting:
CONFIGURE_COMMON = --prefix=$(abspath $(build_prefix)) --build=$(BUILD_MACHINE) --libdir=$(abspath $(build_libdir)) --bindir=$(abspath $(build_depsbindir)) $(CUSTOM_LD_LIBRARY_PATH)
Anyway, this is material for a new issue, it isn't good to keep talking in an unrelated merged PR

@NickCao
Copy link

NickCao commented Aug 28, 2024

See #55617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:building Build system, or building Julia or its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building Julia and LLVM from source is broken on master
5 participants