-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
libunwind: use correct compiler for cross compilation #61544
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
Hmm, actually the change wasn't properly tested, fixing it. |
Works fine in one of my two environments and not in another one, still digging, but at least that's an improvement, as it worked on none of those before |
Fixes rust-lang#59917 Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
Thanks for the PR, but I don't think that this is the best way to solve the issue since the build script should already be using |
@alexcrichton In my case, i'm building from x86_64 a rustc with both x86_64 and i686 targets. CC, AR, etc env vars point to the x86_64 compiler, are picked by I don't have enough time right now but I'll provide logs with and without the patch so you can see the difference. |
Logs without the patch (in a clean environment which doesn't set the CC & friends env vars)
|
Relevant parts of config.toml:
|
Logs with the patch applied:
|
Now, in an environment (our package manager) which sets a lot of env vars such as CC, LD, AR and friends, it still fails (at link time though, not the same failure), didn't find the reason yet. But as it builds just fine in a "clean" environment, that's already a great progress. I'm 100% fine with doing all this differently, I just posted that to gather some cmoments about how the proper way would look like. At least it shows that if we |
The fail I'm seeing when building from our package manager (beta branch) with the patch applied.
Relevant env set by the package manager:
|
It looks like this is doing something that basically isn't well supported now which is to enable the compilation of libunwind. That requires a C++ compiler but the C++ compiler isn't properly configured in rustbuild for libstd, just for the rustc build and beyond (because historically only LLVM needed a C++ compiler). The fix for this is to handle that somehow, not by introducing yet-another-new-env-var to convey compiler information. |
Last failure was actually caused by lld, works fine with gold. I'll see how these are set for the rustc build and try to move those definitions around so that libstd can use them too |
Ok, now I'm really puzzled.
It seems that the env is actually here, as expected, so why isn't it used by |
Oh, CC, AR, CFLAGS are there but CXX isn't |
Replaces rust-lang#61544 Fixes rust-lang#59917 We need CXX to build llvm-libunwind which can be enabled for all targets. As we needed it for all hosts anyways, just move the detection so that it is ran for all targets (which contains all hosts) instead. Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
…hton Use libunwind from llvm-project submodule for musl targets This avoid downloading libunwind by using the scheme introduced in rust-lang#61544
Use libunwind from llvm-project submodule for musl targets This avoid downloading libunwind by using the scheme introduced in #61544
…hton Use libunwind from llvm-project submodule for musl targets This avoid downloading libunwind by using the scheme introduced in rust-lang#61544
…hton Use libunwind from llvm-project submodule for musl targets This avoid downloading libunwind by using the scheme introduced in rust-lang#61544
…hton Use libunwind from llvm-project submodule for musl targets This avoid downloading libunwind by using the scheme introduced in rust-lang#61544
…hton Use libunwind from llvm-project submodule for musl targets This avoid downloading libunwind by using the scheme introduced in rust-lang#61544
Fixes #59917