Use library paths instead of -Lnative/-lstatic when linking native dependencies
#841
Conversation
Fixes static library naming issues that cause the build of a rust_binary that
depends on a cc_library that itself depends on a rust_library to fail.
Consider a rust_binary that depends on a cc_library that itself depends
on a rust_library:
```
.
├── bin.rs
├── BUILD
└── lib.rs
```
```rust
// bin.rs
fn main() {
println!("hi");
}
```
```rust
// lib.rs: empty
```
```bazel
load("@rules_rust//rust:defs.bzl", "rust_benchmark", "rust_binary", "rust_library")
rust_library(
name = "rust_lib",
srcs = ["lib.rs"],
)
cc_library(
name = "cc_lib",
srcs = [],
deps = [":rust_lib"],
)
rust_binary(
name = "bin",
srcs = ["bin.rs"],
deps = [":cc_lib"],
)
```
Running `bazel build //project:bin` runs into linker errors like:
```
/usr/bin/ld.gold: error: cannot find -lrust_lib
/usr/bin/ld.gold: error: cannot find -lrustc_std_workspace_alloc-1ff59d4f23b10626
```
This is because the linker expects the static library corresponding to `-lname`
to be named `libname.a`. Also Bazel CC link actions get confused by static
library names that have multiple dots, e.g., end in "libcrate.rlib.a", so
updated the scripts to produce them as "libcrate-rlib.a" instead.
Fixes an issue that causes the build of a rust_binary that depends on a
cc_library that depends on a rust_library to fail.
Consider a rust_binary that depends on a cc_library that itself depends
on a rust_library:
```
.
├── bin.rs
├── BUILD
└── lib.rs
```
```rust
// bin.rs
fn main() {
println!("hi");
}
```
```rust
// lib.rs: empty
```
```bazel
load("@rules_rust//rust:defs.bzl", "rust_benchmark", "rust_binary", "rust_library")
rust_library(
name = "rust_lib",
srcs = ["lib.rs"],
)
cc_library(
name = "cc_lib",
srcs = [],
deps = [":rust_lib"],
)
rust_binary(
name = "bin",
srcs = ["bin.rs"],
deps = [":cc_lib"],
)
```
Running `bazel build //project:bin` runs into linker errors like:
/usr/bin/ld.gold: error: bazel-out/k8-fastbuild/bin/external/rust_linux_x86_64/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_unwind-5f5ff14665a8d5c5-rlib.a(panic_unwind-5f5ff14665a8d5c5.panic_unwind.p9ngf95o-cgu.0.rcgu.o): multiple definition of '__rust_panic_cleanup'
/usr/bin/ld.gold: bazel-out/k8-fastbuild/bin/external/rust_linux_x86_64/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_abort-c6166278e71d9daf-rlib.a(panic_abort-c6166278e71d9daf.panic_abort.4p8k2zko-cgu.0.rcgu.o): previous definition here
That's because we're passing both panic_unwind and panic_abort to the linker.
The standard library by default depends on panic_unwind. This updates the rules
to prefer panic_unwind. It would be nice to have an option for the user to
pick the panic runtime similarly to how it could be done upstream:
https://rustc-dev-guide.rust-lang.org/panic-implementation.html#step-2-the-panic-runtime
I'll add a additional test case there.
This should address the Windows buildbot failure.
… into rust_cc_rust
… into rust_cc_rust
They are needed to compile the examples.
…bazelbuild-main
krasimirgg
changed the title
-Lnative/-lstatic when linking native dependencies
|
Oh, CI doesn't like the dylibs change :/ |
Trying out to build them with -Clink-arg produces test binaries that fail to find the dynamic library at runtime, e.g; https://buildkite.com/bazel/rules-rust-rustlang/builds/3850#d19409bc-a30a-4944-9f81-e5da5e9d06a6 /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1458/execroot/rules_rust/bazel-out/k8-fastbuild/bin/external/examples/ffi/rust_calling_c/matrix_dylib_test.launcher.runfiles/rules_rust/external/examples/ffi/rust_calling_c/matrix_dylib_test: error while loading shared libraries: bazel-out/k8-fastbuild/bin/_solib_k8/_U@examples_S_Sffi_Srust_Ucalling_Uc_Sc_Cnative_Umatrix_Uso___Uexternal_Sexamples_Sffi_Srust_Ucalling_Uc_Sc/libnative_matrix_so.so: cannot open shared object file: No such file or directory Added a TODO to investigate.
…/rules_rust into same_cc_library_names
I'm sorry, I tried it out, noticed the failure and then left the PR in a broken state for a while. I'm still interested in pursuing this change for dylibs, but I'll need to dig deeper in the dynamic linking process to figure this out. I left a TODO and will try resolve it in a follow-up PR. |
|
Has this changed the order that args get passed to the linker to produce the final executable? When using rules_rust after this commit, I get link failures when using a It appears that the link opts and the objects they come from are losing order relative to each other. e.g rather than |
|
I did not intend to change the order that args get passed to the linker, just how each native dep is passed. In your example, how are the dependencies of the Line 392 in 75c63c3 Also which linker are you using? I might have instructed my linker to ignore backreferences to the standard libraries via –warn-backrefs-exclude and hence missed this. |
|
So the example I found this with was pretty complex, using the A build log is linked below, from this build. The rust specific failure is right at the bottom. However, since this is such a weird setup, I'll see if I can create a small reproduction that doesn't depend on such big projects. Build log: rules-proto-grpc-rules-proto-grpc_build_1159_build-and-test-all-on-ubuntu-18-dot-04-openjdk-11.log |
|
+1 for this breaking builds using
This means C++ code must be sandwiched between Rust code in the link order, or the whole thing has to be wrapped in I don't see a way to fix it with the approach used by this PR: any way of passing a generic linker argument through You can see my problematic toolchain at https://github.com/frc971/971-Robot-Code/blob/c34b3ea019461766d70b43b41f509faa6b8001f3/tools/cpp/BUILD#L122. I can work on pulling out a smaller example if that would help, but I think the CI build referenced in dtolnay/cxx#947 is a more convenient example. For reference, I've got some other |
Thank you for the detailed report! I'm looking into this. |
Inspired from bazelbuild#841 (comment).
This reverts rules_rust to use `-Lnative/-lstatic` instead of library paths per bazelbuild#840 (comment).
|
Adding a note here for anybody from the future: #1148 (and related PRs) fix this for me. |
This fixes #840 by updating the rules to use library paths instead of
-Lnative/-lstaticwhen linking native dependencies.