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

Make llvmunwind depend on llvm #45380

Merged
merged 1 commit into from
May 29, 2022
Merged

Conversation

fxcoudert
Copy link
Contributor

Fixes #45379

@giordano giordano added building Build system, or building Julia or its dependencies system:mac Affects only macOS external dependencies Involves LLVM, OpenBLAS, or other linked libraries labels May 19, 2022
@vchuravy
Copy link
Member

Our LLVMunwind is trailing our LLVM version so this now tightly couples them. We should upgrade LLVMUnwind to match the LLVM version first.

@giordano
Copy link
Contributor

Our LLVMunwind is trailing our LLVM version so this now tightly couples them. We should upgrade LLVMUnwind to match the LLVM version first.

Isn't that orthogonal to this PR? This is about the order with which we have to build the various dependencies.

@fxcoudert
Copy link
Contributor Author

This enforces the right build order, in the case where both are built from source. It just fixes the build, but does not change the versions used or couple them any more than they already are.

@vchuravy
Copy link
Member

What I am saying is that linking LLVMUnwind12 against LLVM13 may not work.

We use a standalone built of LLVMUnwind https://github.com/JuliaPackaging/Yggdrasil/blob/master/L/LLVMLibUnwind/common.jl

@vchuravy
Copy link
Member

Ah I see it just adds an ordering? But why is that necessary? These two libraries should be independent of each other

@fxcoudert
Copy link
Contributor Author

fxcoudert commented May 19, 2022

These two libraries should be independent of each other

They're not, because later in that file:

LLVMUNWIND_OPTS := $(CMAKE_COMMON) -DCMAKE_BUILD_TYPE=MinSizeRel -DLIBUNWIND_ENABLE_PEDANTIC=OFF -DLLVM_CONFIG_PATH=$(build_depsbindir)/llvm-config

So this PR is only fixing a build failure, but not otherwise changing the already existing dependency. It is not, in other words, making things worse on that side.

@vchuravy
Copy link
Member

Can you build without setting the llvm-config path?

@fxcoudert
Copy link
Contributor Author

If I remove LLVM_CONFIG_PATH entirely, it gives this warning:

CMake Warning at CMakeLists.txt:65 (message):
  UNSUPPORTED LIBUNWIND CONFIGURATION DETECTED: llvm-config not found and
  LLVM_MAIN_SRC_DIR not defined.  Reconfigure with
  -DLLVM_CONFIG=path/to/llvm-config or -DLLVM_PATH=path/to/llvm-source-root.


CMake Warning at CMakeLists.txt:76 (message):
  Not found:

but otherwise appears to build without trouble.

@ViralBShah
Copy link
Member

Should we merge?

@fxcoudert
Copy link
Contributor Author

One test failure in tester_linuxaarch64.

Error in testset InteractiveUtils:
Error During Test at none:1
  Got exception outside of a @test
  ProcessExitedException(6)
  Stacktrace:
    [1] try_yieldto(undo::typeof(Base.ensure_rescheduled))
      @ Base ./task.jl:881
    [2] wait()
      @ Base ./task.jl:945
    [3] wait(c::Base.GenericCondition{ReentrantLock})
      @ Base ./condition.jl:124
    [4] take_buffered(c::Channel{Any})
      @ Base ./channels.jl:416
    [5] take!(c::Channel{Any})
      @ Base ./channels.jl:410
    [6] take!(::Distributed.RemoteValue)
      @ Distributed /buildworker/worker/tester_linuxaarch64/build/share/julia/stdlib/v1.9/Distributed/src/remotecall.jl:726
    [7] remotecall_fetch(::Function, ::Distributed.Worker, ::String, ::Vararg{String}; kwargs::Base.Pairs{Symbol, UInt128, Tuple{Symbol}, NamedTuple{(:seed,), Tuple{UInt128}}})
      @ Distributed /buildworker/worker/tester_linuxaarch64/build/share/julia/stdlib/v1.9/Distributed/src/remotecall.jl:461
    [8] remotecall_fetch(::Function, ::Int64, ::String, ::Vararg{String}; kwargs::Base.Pairs{Symbol, UInt128, Tuple{Symbol}, NamedTuple{(:seed,), Tuple{UInt128}}})
      @ Distributed /buildworker/worker/tester_linuxaarch64/build/share/julia/stdlib/v1.9/Distributed/src/remotecall.jl:492
    [9] macro expansion
      @ /buildworker/worker/tester_linuxaarch64/build/share/julia/test/runtests.jl:260 [inlined]
   [10] (::var"#45#57"{Vector{Task}, var"#print_testworker_errored#53"{ReentrantLock, Int64, Int64}, var"#print_testworker_stats#51"{ReentrantLock, Int64, Int64, Int64, Int64, Int64, Int64}, Vector{Any}, Dict{String, DateTime}})()
      @ Main ./task.jl:494

@fxcoudert
Copy link
Contributor Author

The same error is showing in an unrelated PR on freebsd64: #45435
I think it's fluke, and the PR is ready to be merged.

@ViralBShah
Copy link
Member

@vchuravy merge?

@fxcoudert
Copy link
Contributor Author

This is needed for source builds to work on macOS. Could it be merged, and possibly backported to 1.8 before release?

@ViralBShah
Copy link
Member

I'm merging this since it should really not be an issue.

@ViralBShah ViralBShah merged commit 6ea9034 into JuliaLang:master May 29, 2022
@fxcoudert fxcoudert deleted the llvmunwind branch May 29, 2022 14:58
@vchuravy
Copy link
Member

As I said before I think this is wrong. Our builds of LLVMUnwind and LLVM proper are NOT linked. So the use of llvm-config is definitely wrong.

I don't know what the right solution is. My preferred solution is to build LLVMUnwind as part of LLVM so that these issues do not arise. But as the situation is this is a "pseudo"-fix. I do not have a Mac. @vtjnash this seems more in your wheel house.

@ViralBShah
Copy link
Member

If it is ok, this helps us get past the current build failure on macs from source? In the meanwhile, can we separately open an issue to build LLVMUnwind the right way?

@DilumAluthge
Copy link
Member

Pinging @ararslan and @omus, since they've done the most work on LLVMUnwind in Yggdrasil.

@ViralBShah
Copy link
Member

ViralBShah commented May 29, 2022

Just a note that @vtjnash approved this PR. I'm marking it for backport so that the build for homebrew on mac can go through, whatever the longer term decision is.

@ViralBShah ViralBShah added the backport 1.8 Change should be backported to release-1.8 label May 29, 2022
KristofferC pushed a commit that referenced this pull request Jun 7, 2022
@KristofferC KristofferC mentioned this pull request Jun 7, 2022
36 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies external dependencies Involves LLVM, OpenBLAS, or other linked libraries system:mac Affects only macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source build: llvmunwind should be compiled after llvm
7 participants