Rust link arguments are not passed transitively to dependant rust targets

[SirVer]

Simple repro case is in this branch.

bazel run @examples//hello_world exposes the problem.

The example is a simple dependency chain: rust_binary ("hello_world") depends on rust_library ("hello_from_c_sys") which depends on cc_library ("hello_from_c"). The c library requires c++, therefore needs linking with -lstdc++. It should be sufficient to specify this in the rust_library rule, but right now all rust_binary rules that transitively depend on the lib need to specify it as well: https://github.com/bazelbuild/rules_rust/compare/master...SirVer:link_example?expand=1#diff-e85472ad14a7cde9a8510d9beae332c5R18

[mfarrugi]

Afaiu #61 handles the build step on osx, but doesn't correctly set the rpath.

You can try commenting out compute_rpaths from the pr branch (I don't have a mac to test on) to get it to build, then using LD_LIBRARY_PATH to get it to run.

At this point we should be asking @mhlopko for advice on not reinventing cross platform dylib linking, in the context of the cc Skylark project.

(I have no idea if collecting link args is a generally correct thing to do.)

[SirVer]

@mfarrugi Just to clarify, the linked repro case also doesn't link on Linux for me without commenting in the "-Clink-args=-lstdc++" in the rust_binary.

[mfarrugi]

On the PR branch? If so, I'll take a look as transitive dylibs ought to be handled.

[SirVer]

@mfarrugi Yes, the branch linked in OP: https://github.com/bazelbuild/rules_rust/compare/master...SirVer:link_example?expand=1#diff-e85472ad14a7cde9a8510d9beae332c5R18

[acmcarther]

Do you happen to know off hand what Cargo's current treatment of Clink-args is? If you don't, I can whip up a mock crate and verbose build it to see. I could easily believe that this is an oversight on our (rules_rust) part.

If it is indeed an oversight, we'll probably need to promote this flag to a rule-level attr so that the transitives can be tracked.

[mfarrugi]

@SirVer ah sorry, I meant to ask if your change worked on top of the #61 branch that's not merged in yet. With that PR, nothing should need to provide -Clink-args since it should be able to pickup transitive dependencies from the cc_library, but libc++ might still be a special case.

[SirVer]

@mfarrugi I will try next week.

[hlopko]

I think you'll eventually want to have transitive linkopts-like attribute on rust_library anyway. It is true though that it could be emulated by rust_library depending on a cc_import:

rust_library(
  name = "a",
  deps = [ ":cc" ],
)

cc_import(
  name = "cc",
  linkopts = [ "-lstdc++" ],
)

but this cannot be implemented currently because we would need bazelbuild/bazel#4570 to be implemented.

[SirVer]

@mfarrugi Doesn't work on #61 for me. I still need to add the -lstdc++.

[bobsomers]

I've been dealing with the same issue, which shows up immediately once you try writing Rust bindings to a C++ library. The way I've tried to go about this is by writing C API wrapper (cc_library) that depends on the C++ library and make therust_library bindings use the FFI to call into the cc_library C API.

In other words, I have a dep chain that looks like this:
rust_binary -> rust_library (bindings) -> cc_library (C API) -> cc_library (C++ library to bind)

From the digging I've done so far into how the cc rules and rust rules work, it seems there is a conceptual impedance mismatch with how things are implemented right now.

• Bazel's model for linking C++ code is to underlink all intermediate cc_library targets (see Control linking of dependencies for cc_library  bazel#492), leaving lots of undefined references that are all resolved in the big final link at the cc_binary target.

• The Rust rules currently link against direct dependencies (Rust or C++) and only use the full set of transitive deps as action inputs, not for linking. This obviously assumes that you don't need to link transitive deps because they were included when linking the direct deps, but they weren't, because the direct deps were underlinked.

There are actually two errors that I see here. Fixing these should bring the Rust rules conceptually in line with what the cc rules do:

1. The rust_library rule should not link any C++ libraries. This ensures intermediate rust_library targets are underlinked in the same way C++ library targets are.

2. The rust_binary rule should fully link all transitive C++ libraries in the same way the cc_binary rule does.

Does that logic seem correct? I'm particularly curious what you think, @mhlopko.

[SirVer]

@mhlopko @acmcarther

@bobsomers is right. I actually have a failure mode right now that is not properly handled by the way rust rules are currently linking. I have a rust_binary that uses grpc and rusoto - both depend on openssl at the bottom. Running this binary crashes right now, because openssl is not properly initialized - because it seems the symbols from both linking steps are not correctly deduplicated.

So the rust rules must not link everything until the binary level, just as bazel does for c++.

[bobsomers]

Quick update: I have the transitive linking issues mostly fixed. I still need to fix up the conflicting rlib name example, but otherwise you can properly transitively link as I described above. I have an example that produces a rust_binary that depends on a rust_library that depends on a cc_library (C API) that depends on a cc_library (C++ library) and it links and runs.

The solution is mostly orthogonal to #61, so merging this transitive fix with shared library support should be doable.

If anyone wants to play around with it while I fix the last few things and get it ready for a PR, here's my branch: https://github.com/bobsomers/rules_rust/tree/transitive_linking

The example of interest is bazel run @examples//ffi/rust_calling_c:hello

[hlopko]

Cc_library actually doesn't necessarily do any linking (when using gold linker we only compile to objects and then link everything in cc_binary with --start-lib a.o b.o c.o --end-lib).

The main reason for "underlinking" is to make the build not so quadratic. Imagine we have this dep graph: bin -> liba -> libb -> libc. When we change libc, ideally we won't have to rebuild liba and libb. Ideally we wouldn't have to relink everything even in the binary, but that's how C++ works so meh.

Plus the diamond problem as @SirVer mentioned is a chapter on its own.

So if we can, we should make rust_library to be a bunch of objects, or a static archive at most.

[GregBowyer]

@bobsomers was there anything we need to make you patch work?

[bobsomers]

@GregBowyer It probably needs to be rethought now that Bazel has redesigned how C++ toolchains work. I haven't had the time to get up to speed on the new toolchains or work on this due to a new infant at home.

[ali5h]

is there any short term solution for this problem?

[GregBowyer]

Humm is there a test case, this might be solved by #217, if not it might not be terribly hard to support excluding WASM

[ali5h]

at least for me this is the issue #264

[asomers]

This is not just a problem for Bazel. I've seen it for two different FFI libraries, where the C library is underlinked.

rust -> libpmc.so -> libc++.so
rust -> libzfs_core.so -> libzutil.so

In both cases, specifying the transitive dependency in build.rs is insufficient; Rust binaries only link to the direct dependency.

[asomers]

I found a solution when using Cargo. You need to pass the -as-needed option to rustc, like this:

println!("cargo:rustc-link-lib=dylib:-as-needed=zutil");

https://rust-lang.github.io/rfcs/2951-native-link-modifiers.html

[UebelAndre]

Does #849 address the issues reported in this issue?

[hlopko]

That and a bunch of other changes we made over the course of 2021. Can somebody try with the latest rules to see if it is still a problem? If yes, could you share a tiny reproduction case? Is the problem occuring on Linux or also on other platforms?

In any case I feel like we made a lot of progress - we are building some mixed Rust / C++ binaries internally and everything seems to be working, and I expect that the problem mentioned in thiss issue is either fixed, or is not reproducible on Linux (we don't do Windows or Darwin builds).

[kshcherban]

There's a problem with rust pkg-config (cargo-raze?) and OS X.
Rust library libpulse-simple-sys depends on a pulseaudio library that is installed with homebrew. libpulse-simple-sys builds fine itself but if there's any rust_binary that depends on some rust_library that depends on libpulse-simple-sys then bazel fails with: ld: library not found for -l:libpulse-simple.0.dylib.

But libpulse-simple.0.dylib can be found in /usr/local/lib

locate libpulse-simple.0.dylib
/usr/local/Cellar/pulseaudio/14.2/lib/libpulse-simple.0.dylib
/usr/local/lib/libpulse-simple.0.dylib

[mickeyh]

Does #849 address the issues reported in this issue?

I think it does (at least, I have successfully had linker arguments passed from a cc_library into a rust_binary).

However, in one particular edge case (rust_tests that cover inline tests (using the crate attribute)) it still fails. I'm pretty confident that this occurs because in this case:

1. _rust_test_impl passes crate.deps directly as transitive dependencies, BUT
2. get_linker_and_args only checks for linker opts that come from direct deps.

Stated differently: the assumption in get_linker_and_args that "This includes linkopts from transitive dependencies since they get merged up" is false for rust_tests that cover inline tests using the crate attribute.

A workaround is to manually add the target that is listed in crate also in deps. I'm not sure if doing this for all rust_tests is "correct" though (I'm unfortunately not familiar enough with these rules).

[scentini]

I believe 1 is not part of the problem; although we collect the deps in a depset, we flatten the depset here: https://github.com/bazelbuild/rules_rust/blob/main/rust/private/rustc.bzl#L207.
For 2 - if someone volunteers to write a PR - the fix would be to replace the gettattr("deps") call with crate_info.deps, similar to https://github.com/bazelbuild/rules_rust/pull/1543/files

[mickeyh]

@scentini I've put up a PR #1569 , which I think does what you describe. PTAL when you have the chance.