Transitively link native libraries properly. #84

bobsomers · 2018-05-10T21:58:51Z

Prior to this change, the Rust rules have a conceptual mismatch with how the C++ rules work. This makes writing Rust bindings to a native library impossible.

Bazel's model of C++ linking is to underlink all intermediate cc_library targets and do the final link at the cc_binary target. To bring the Rust rules in line with the C++ rules, this change removes any native library linking from rust_library targets, but provides the information transitively so that the rust_binary target can link all transitive native libraries during the final link.

One thing to note is that Rust libraries (.rlib) behave slightly differently. While they do not contain their rlib dependencies, they do reference their rlib dependencies, so the proper behavior with rlibs is to always link (--extern) the direct rlib deps, regardless of whether we are building a rust_library or rust_binary.

I added an example to the ffi examples of the following dep chain:
rust_binary -> rust_library -> cc_library -> cc_library

This is the typical way of binding a C++ library, where the cc_library on the far right is the actual C++ library you are binding and the one upstream from it is a C API over the C++ library. The rust_library upstream of that is the Rust FFI declarations and finally the rust_binary calls into the bindings through the rust_library. For good measure, I added an additional rust_library dependency to the rust_library to make sure that we still do the right thing with respect to rust_library -> rust_library dependencies.

Fixes #78.

Prior to this change, the Rust rules have a conceptual mismatch with how the C++ rules work. This makes writing Rust bindings to a native library impossible. Bazel's model of C++ linking is to underlink all intermediate cc_library targets and do the final link at the cc_binary target. To bring the Rust rules in line with the C++ rules, this change removes any native library linking from rust_library targets, but provides the information transitively so that the rust_binary target can link all transitive native libraries during the final link. One thing to note is that Rust libraries (.rlib) behave slightly differently. While they do not contain their rlib dependencies, they do reference their rlib dependencies, so the proper behavior with rlibs is to always link (--extern) the direct rlib deps, regardless of whether we are building a rust_library or rust_binary. I added an example to the ffi examples of the following dep chain: rust_binary -> rust_library -> cc_library -> cc_library This is the typical way of binding a C++ library, where the cc_library on the far right is the actual C++ library you are binding and the one upstream from it is a C API over the C++ library. The rust_library upstream of that is the Rust FFI declarations and finally the rust_binary calls into the bindings through the rust_library. For good measure, I added an additional rust_library dependency to the rust_library to make sure that we still do the right thing with respect to rust_library -> rust_library dependencies.

mfarrugi

Looks good to me, but would like to avoid extra and duplicated flags.

mfarrugi · 2018-05-11T01:10:22Z

rust/toolchain.bzl

@@ -49,17 +50,19 @@ def build_rustc_command(ctx, toolchain, crate_name, crate_type, src, output_dir,
          "--codegen ar=%s" % ar,
          "--codegen linker=%s" % cc,
          "--codegen link-args='%s'" % ' '.join(cpp_fragment.link_options),
+          "--codegen link-args='%s'" % ' '.join(cpp_fragment.mostly_static_link_options([], False)),


Do you know if this (or the line above it) is a noop with no native dependencies?

This is also terrifying to me in the context of figuring out what I would need to do for #61, do you know what that should look like already?

Yeah, this is pretty unfortunate, but I can't find a way to get "whatever the right set of default C++ linking options are" from Bazel. The link_opts here are usually empty. The fully_static..., mostly_static..., and dynamic_... variants contain what I need, but are apparently deprecated, and also don't give you any insight into which of the three is appropriate in this situation.

What I really need is just whatever the appropriate C++ linker options are, based on the sysroot, embedded runtimes, etc. for the given C++ toolchain. For example in a default crosstool this will add -lstdc++ during a cc_binary link, but the Rust link step doesn't know to do this, nor should it. Essentially if we're depending on any native deps, I need to know what the full set of links flags are for those native deps. Maybe a Bazel dev could point me in the right direction to find those link flags.

mfarrugi · 2018-05-11T01:10:53Z

rust/rust.bzl

@@ -535,12 +562,16 @@ def _rust_doc_test_impl(ctx):
                        working_dir=".",
                        allow_cc_deps=False,
                        in_runfiles=True)
+  search_flags = depinfo.rust_search_flags + depinfo.native_search_flags


This is probably worth a comment highlighting the difference from rust_library, took me a little bit to notice

Ah, I made the same comment on the rust_library. Definitely need comments for this bit of subtleness

Will comment this to call out the different behavior.

mfarrugi · 2018-05-11T01:12:11Z

rust/rust.bzl

@@ -172,8 +186,7 @@ def _setup_deps(deps, name, working_dir, allow_cc_deps=False,
      libs += native_libs
      transitive_libs += native_libs
      symlinked_libs += native_libs
-      link_flags += ["-l static=" + dep.label.name]
-      has_native = True
+      native_link_flags += ["-l static={}".format(_native_lib_name(native_lib)) for native_lib in dep.cc.libs]


In #61 I ran into needing to deduplicate and order the link flags, are you confident this isn't necessary for static libraries?

I would also feel a little better deduplicating the flags regardless, to reduce noise in the command lines and general paranoia.

How does this fail on dylibs? Looks like maybe a linker error on build?

Probably a good idea. I'm not super familiar with what kinds of things you can put in a depset, but if I can put generic strings in a depset than I think that's the right way to do this instead of appending to a list.

I think when I tried using this in a different codebase (which has dylibs in the dep chain) it failed with undefined references, but it was actually caused by the _native_lib_name() function being too primitive. It doesn't take into account dylibs with SONAME extensions, like libfoo.so.3.0. Plus it really needs to link those with -l dynamic= instead of -l static=.

I'm not quite sure how to pass those dylibs to rustc. When Bazel encounters an SONAME dylib it links with -l:libfoo.so.3.0 instead of -lfoo to specify the exact .so it wants to link with. Maybe for those dylibs we should pass through the -l: form as a raw linker arg instead of using rustc's library linking flags.

mfarrugi · 2018-05-11T01:17:49Z

rust/rust.bzl

-      link_flags = link_flags)
+      rust_search_flags = ["-L dependency=%s" % deps_dir],
+      rust_link_flags = rust_link_flags,
+      native_search_flags = ["-L native=%s" % deps_dir],


would prefer not passing -L native when it's not necessary

Makes sense, I'll filter it out if there are no native deps in the dep chain.

mfarrugi · 2018-05-11T01:20:26Z

rust/toolchain.bzl

@@ -6,7 +6,8 @@ ZIP_PATH = "/usr/bin/zip"

 # Utility methods that use the toolchain provider.
 def build_rustc_command(ctx, toolchain, crate_name, crate_type, src, output_dir,
-                         depinfo, output_hash=None, rust_flags=[]):
+                         depinfo, output_hash=None, rust_flags=[],


If you check crate_type == "bin" here to build link_flags out of depinfo, it may be a little easier to read.

Agreed. That also lets us centralize the explanation about linking (e.g. why we only need native stuff for binaries) instead of scattering it out into the individual rule impls.

I think in an earlier attempt I actually did that, but it ended up being quite complicated. That was a while back though. I can take another crack at it and see if we can centralize the linking logic here instead of spread between the rust_library and rust_binary rules.

mfarrugi · 2018-05-11T01:31:41Z

@acmcarther any thoughts?

@SirVer if you have time, can you check that this fixes your more complicated use case?

acmcarther

Great explanation of the problem and I learned quite a bit about Rust and CPP linking from the review.

I'm unclear on the interaction with #61, you may want to work with mfarrugi@ on that.

acmcarther · 2018-05-11T17:18:14Z

rust/rust.bzl

@@ -265,6 +274,8 @@ def _rust_library_impl(ctx):
                        ctx.label.name,
                        output_dir,
                        allow_cc_deps=True)
+  search_flags = depinfo.rust_search_flags


It may be worth explaining here (in a comment above this line) what you explained in the PR description: Rust libraries don't need to find or link the native dependencies, they only need to track them (where they're ultimately linked in a rust_binary). Therefore, we're only using the rust_(search|link)_flags here

Will do 👍

acmcarther · 2018-05-11T17:19:54Z

rust/rust.bzl

@@ -307,7 +320,8 @@ def _rust_library_impl(ctx):
      rust_srcs = ctx.files.srcs,
      rust_deps = ctx.attr.deps,
      transitive_libs = depinfo.transitive_libs,
-      rust_lib = rust_lib)
+      rust_lib = rust_lib,
+      native_link_flags = depinfo.native_link_flags)


I'm a bit confused. Is depinfo.native_search_flags not being retained? Does it need to be?

AFAICT it doesn't need to be. Since all the deps are collected and symlinked into a single .deps directory there's only one search path for any native dep.

acmcarther · 2018-05-11T17:21:10Z

rust/rust.bzl

@@ -535,12 +562,16 @@ def _rust_doc_test_impl(ctx):
                        working_dir=".",
                        allow_cc_deps=False,
                        in_runfiles=True)
+  search_flags = depinfo.rust_search_flags + depinfo.native_search_flags


Ah, I made the same comment on the rust_library. Definitely need comments for this bit of subtleness

acmcarther · 2018-05-11T17:22:02Z

rust/toolchain.bzl

@@ -6,7 +6,8 @@ ZIP_PATH = "/usr/bin/zip"

 # Utility methods that use the toolchain provider.
 def build_rustc_command(ctx, toolchain, crate_name, crate_type, src, output_dir,
-                         depinfo, output_hash=None, rust_flags=[]):
+                         depinfo, output_hash=None, rust_flags=[],


Agreed. That also lets us centralize the explanation about linking (e.g. why we only need native stuff for binaries) instead of scattering it out into the individual rule impls.

bobsomers · 2018-05-11T20:01:54Z

@acmcarther I think @mfarrugi changes (#61) should merge first. I opened this PR to kickstart review, but I can rework this once the shared library support is in master.

SirVer · 2018-05-15T15:24:34Z

@mfarrugi I am excited to try this. I will be traveling till mid June though so will realistically not get around to do so till then.

acmcarther · 2018-05-24T01:37:29Z

#61 now merged

mfarrugi · 2018-05-25T19:54:45Z

Should we also be picking up cc_library.linkopts? Is that out of scope?

SirVer · 2018-06-07T12:11:41Z

@mfarrugi @bobsomers I have given this a shot now and I am unable to make my binary compile now. It always reports unresolved symbols coming from openssl. The chain looks something like this:

tool -> grpc -> openssl-rs -> openssl_sys -> openssl c bindings
     -> rusotto -> openssl-rs -> openssl_sys -> openssl c bindings

Before this branch I could get it to link, but it would crash on use.

m4b · 2019-03-29T01:26:30Z

What happened here ? Did work stop ? It looks like this is going to get really urgent once #209 lands. I’ve tested on that branch and now I don’t think it’s possible to use rust bazel rules and do:

c static lib-> rust static lib -> c static lib -> c binary

without omitting every single c library that a rust lib depends on but which will also be passed in the final c binary link

For precisely the reasons outlined here, eg the c/c++ rules collect deps at the analysis level, but don’t emit/ar the object files into the static library a given compilation unit depends on; eg a final link (a binary target) will collect all the required static libs and pass them to the linker and all symbols will get resolved, without duplicates.

However since the rustc call presumably calls ar with all the static deps it depends on, and emits them into the resulting rust static library, a subsequent link with a c binary with the rust lib and a c static lib the rust lib depends on, will result in duplicate symbol errors in the linking stage.

aiuto · 2019-08-18T02:13:55Z

ping... anyone there?
My guess is that Bazel has changed enough in 15+ months so that this may need a major update.

damienmg · 2020-05-18T15:36:21Z

This PR is stalled, there was no update from the original contributor so we have to close it.

googlebot added the cla: yes label May 10, 2018

mfarrugi reviewed May 11, 2018

View reviewed changes

acmcarther reviewed May 11, 2018

View reviewed changes

mfarrugi mentioned this pull request May 22, 2018

Add support for depending on shared libraries on linux. #61

Merged

damienmg closed this May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transitively link native libraries properly. #84

Transitively link native libraries properly. #84

bobsomers commented May 10, 2018

mfarrugi left a comment

mfarrugi May 11, 2018

mfarrugi May 11, 2018

bobsomers May 11, 2018

mfarrugi May 11, 2018

acmcarther May 11, 2018

bobsomers May 11, 2018

mfarrugi May 11, 2018

mfarrugi May 11, 2018

mfarrugi May 11, 2018

bobsomers May 11, 2018

mfarrugi May 11, 2018

bobsomers May 11, 2018

mfarrugi May 11, 2018

acmcarther May 11, 2018

bobsomers May 11, 2018

mfarrugi commented May 11, 2018

acmcarther left a comment

acmcarther May 11, 2018

bobsomers May 11, 2018

acmcarther May 11, 2018

bobsomers May 11, 2018

acmcarther May 11, 2018

acmcarther May 11, 2018

bobsomers commented May 11, 2018

SirVer commented May 15, 2018

acmcarther commented May 24, 2018

mfarrugi commented May 25, 2018

SirVer commented Jun 7, 2018

m4b commented Mar 29, 2019

aiuto commented Aug 18, 2019

damienmg commented May 18, 2020

Transitively link native libraries properly. #84

Transitively link native libraries properly. #84

Conversation

bobsomers commented May 10, 2018

mfarrugi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfarrugi commented May 11, 2018

acmcarther left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobsomers commented May 11, 2018

SirVer commented May 15, 2018

acmcarther commented May 24, 2018

mfarrugi commented May 25, 2018

SirVer commented Jun 7, 2018

m4b commented Mar 29, 2019

aiuto commented Aug 18, 2019

damienmg commented May 18, 2020