Fixes for rust_binary that depends on a cc_library that depends on a rust_library #825
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
|
Hi and thanks for opening this PR! 😄 Could you also add a regression test to the |
I'll add a additional test case there.
This should address the Windows buildbot failure.
Added some tests. |
There was a problem hiding this comment.
Thanks for adding a test!
I think this generally looks pretty good but I'm wondering if the logic for parsing out panic_unwind vs panic_abort could be moved out of rust_stdlib_filegroup and into rust_toolchain (specifically _make_libstd_and_allocator_ccinfo). I think the rust_stdlib_filegroup rule should be gathering everything from a rust_std artifact but not making any decisions on what to or not-to include. I would say, either leave both panic_unwind and panic_abort in std_rlibs (and therefor dot_a_files) and parse them out later, or update the StdLibInfo provider to have panic_unwind_files and panic_abort_files that have the appropriate things parsed out of the various fields there. I think I lean towards the latter but not strongly. How does that sound?
There was a problem hiding this comment.
This looks good to me! Thank you! 🙏
I'll give @hlopko a chance to take a look before merging just to be safe 😅
… into rust_cc_rust
… into rust_cc_rust
Consider a case of a
rust_binarythat depends on acc_librarythat itself dependson a
rust_library:// lib.rs: emptyRunning
bazel build //project:binruns into several linker errors, which this PR tries to address:-lcrate, the linker looks for a static library namedlibcrate.a.updated the scripts to produce them as "libcrate-rlib.a" instead.
panic_abortandpanic_unwindare alternatives; attempting to pass both to the linker runs into duplicate symbol errors. Updated the rules to preferpanic_unwindif both are available. This is since the standard library by default depends onpanic_unwind. Note that in platforms wherepanic_unwindis not available,panic_abortwill be picked. In the future we could add a feature that allows users to pick the panic strategy, aligned with how it will be done in upstream rustc (Support different panic strategies. rust-lang/wg-cargo-std-aware#29).Another thing worth noting is that the way we currently process
.rlib-s into static libs to pass on to CC rules doesn't quite work when using the current stablerustc 1.53.0; it works for the betarustcand beyond: on Linux, acrate.rlibis an archive that contains .o files (good) and a Rust metadata filelib.rmeta(caution). We symlinkcrate.rlibintolibcrate.a. The linker generally checks that all members of an archive passed in a--whole-archivesection are ELF files. The commit rust-lang/rust@c79419a on Jun 4 updatedrustcso that the producedlib.rmetais an ELF file (previously it was a binary blob), which satisfies the linker checks. This PR does not attempt to address the case wherelib.rmetais not an ELF file, hence even with this PR in, trying to run the example with the current stablerustc 1.53.0will produce linker errors complaining that an archive member is not recognized, like this one/usr/bin/ld.gold: error: rust_lib: plugin failed to claim member lib.rmeta at 2480.With this PR,
bazel build //project:binworks (with sufficiently recentrustc, as described above).