Skip to content

Commit

Permalink
Use library paths instead of -Lnative/-lstatic when linking native …
Browse files Browse the repository at this point in the history
…dependencies (#841)

* update static library naming mixed rust+cc deps

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.

* don't pass panic_abort if panic_unwind is present

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

* rename test/unit/stdlib_ordering to /test/unit/stdlib

I'll add a additional test case there.

* add tests

* restrict test assert to only rlibs and libs

This should address the Windows buildbot failure.

* move panic_{unwind,abort} filtering to _make_libstd_and_allocator_ccinfo

* remove `-rlib` postfix from the names of static library symlinks

* move _make_dota to utils

* remove postfix argument docstring

* when linking, emit the path to a native lib instead of -lnativelibname

* fix native_deps_test for mac and windows

* don't emit `-Lnative=package` for native libraries

* Bring back `-Lnative` for dynamic libs

They are needed to compile the examples.

* also handle dylibs via `-Clink-arg`

* Put back -ldylib for dylibs

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.

Co-authored-by: UebelAndre <github@uebelandre.com>
Co-authored-by: Marcel Hlopko <hlopko@google.com>
  • Loading branch information
3 people committed Jul 22, 2021
1 parent ac7378f commit 4da6858
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 22 deletions.
11 changes: 7 additions & 4 deletions rust/private/rustc.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -837,9 +837,14 @@ def _get_crate_dirname(crate):

def _portable_link_flags(lib):
if lib.static_library or lib.pic_static_library:
return ["-lstatic=%s" % get_lib_name(get_preferred_artifact(lib))]
return ["-C", "link-arg=%s" % get_preferred_artifact(lib).path]
elif _is_dylib(lib):
return ["-ldylib=%s" % get_lib_name(get_preferred_artifact(lib))]
# TODO: Consider switching dylibs to use -Clink-arg as above.
return [
"-Lnative=%s" % get_preferred_artifact(lib).dirname,
"-ldylib=%s" % get_lib_name(get_preferred_artifact(lib)),
]

return []

def _make_link_flags_windows(linker_input):
Expand Down Expand Up @@ -894,8 +899,6 @@ def _add_native_link_flags(args, dep_info, crate_type, toolchain, cc_toolchain,
feature_configuration (FeatureConfiguration): feature configuration to use with cc_toolchain
"""
args.add_all(dep_info.transitive_noncrates, map_each = _libraries_dirnames, uniquify = True, format_each = "-Lnative=%s")

if crate_type in ["lib", "rlib"]:
return

Expand Down
48 changes: 30 additions & 18 deletions test/unit/native_deps/native_deps_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ load("@rules_cc//cc:defs.bzl", "cc_library")
load("//rust:defs.bzl", "rust_binary", "rust_library", "rust_proc_macro", "rust_shared_library", "rust_static_library")
load("//test/unit:common.bzl", "assert_argv_contains", "assert_argv_contains_not", "assert_argv_contains_prefix_suffix")

def _native_dep_lib_name(ctx):
if ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]):
return "native_dep.lib"
else:
return "libnative_dep.a"

def _lib_has_no_native_libs_test_impl(ctx):
env = analysistest.begin(ctx)
tut = analysistest.target_under_test(env)
Expand All @@ -22,7 +28,6 @@ def _rlib_has_no_native_libs_test_impl(ctx):
actions = analysistest.target_actions(env)
action = actions[0]
assert_argv_contains(env, action, "--crate-type=rlib")
assert_argv_contains_prefix_suffix(env, action, "-Lnative=", "/native_deps")
assert_argv_contains_not(env, action, "-lstatic=native_dep")
assert_argv_contains_not(env, action, "-ldylib=native_dep")
return analysistest.end(env)
Expand All @@ -32,7 +37,6 @@ def _dylib_has_native_libs_test_impl(ctx):
tut = analysistest.target_under_test(env)
actions = analysistest.target_actions(env)
action = actions[0]
assert_argv_contains_prefix_suffix(env, action, "-Lnative=", "/native_deps")
assert_argv_contains(env, action, "--crate-type=dylib")
assert_argv_contains(env, action, "-lstatic=native_dep")
return analysistest.end(env)
Expand All @@ -42,19 +46,17 @@ def _cdylib_has_native_libs_test_impl(ctx):
tut = analysistest.target_under_test(env)
actions = analysistest.target_actions(env)
action = actions[0]
assert_argv_contains_prefix_suffix(env, action, "-Lnative=", "/native_deps")
assert_argv_contains(env, action, "--crate-type=cdylib")
assert_argv_contains(env, action, "-lstatic=native_dep")
assert_argv_contains_prefix_suffix(env, action, "link-arg=", "/native_deps/" + _native_dep_lib_name(ctx))
return analysistest.end(env)

def _staticlib_has_native_libs_test_impl(ctx):
env = analysistest.begin(ctx)
tut = analysistest.target_under_test(env)
actions = analysistest.target_actions(env)
action = actions[0]
assert_argv_contains_prefix_suffix(env, action, "-Lnative=", "/native_deps")
assert_argv_contains(env, action, "--crate-type=staticlib")
assert_argv_contains(env, action, "-lstatic=native_dep")
assert_argv_contains_prefix_suffix(env, action, "link-arg=", "/native_deps/" + _native_dep_lib_name(ctx))
return analysistest.end(env)

def _proc_macro_has_native_libs_test_impl(ctx):
Expand All @@ -63,18 +65,16 @@ def _proc_macro_has_native_libs_test_impl(ctx):
actions = analysistest.target_actions(env)
asserts.equals(env, 1, len(actions))
action = actions[0]
assert_argv_contains_prefix_suffix(env, action, "-Lnative=", "/native_deps")
assert_argv_contains(env, action, "--crate-type=proc-macro")
assert_argv_contains(env, action, "-lstatic=native_dep")
assert_argv_contains_prefix_suffix(env, action, "link-arg=", "/native_deps/" + _native_dep_lib_name(ctx))
return analysistest.end(env)

def _bin_has_native_libs_test_impl(ctx):
env = analysistest.begin(ctx)
tut = analysistest.target_under_test(env)
actions = analysistest.target_actions(env)
action = actions[0]
assert_argv_contains_prefix_suffix(env, action, "-Lnative=", "/native_deps")
assert_argv_contains(env, action, "-lstatic=native_dep")
assert_argv_contains_prefix_suffix(env, action, "link-arg=", "/native_deps/" + _native_dep_lib_name(ctx))
return analysistest.end(env)

def _extract_linker_args(argv):
Expand All @@ -88,17 +88,17 @@ def _bin_has_native_dep_and_alwayslink_test_impl(ctx):

if ctx.target_platform_has_constraint(ctx.attr._macos_constraint[platform_common.ConstraintValueInfo]):
want = [
"-lstatic=native_dep",
"link-arg=bazel-out/darwin-fastbuild/bin/test/unit/native_deps/libnative_dep.a",
"link-arg=-Wl,-force_load,bazel-out/darwin-fastbuild/bin/test/unit/native_deps/libalwayslink.lo",
]
elif ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]):
want = [
"-lstatic=native_dep",
"link-arg=bazel-out/x64_windows-fastbuild/bin/test/unit/native_deps/native_dep.lib",
"link-arg=/WHOLEARCHIVE:bazel-out/x64_windows-fastbuild/bin/test/unit/native_deps/alwayslink.lo.lib",
]
else:
want = [
"-lstatic=native_dep",
"link-arg=bazel-out/k8-fastbuild/bin/test/unit/native_deps/libnative_dep.a",
"link-arg=-Wl,--whole-archive",
"link-arg=bazel-out/k8-fastbuild/bin/test/unit/native_deps/libalwayslink.lo",
"link-arg=-Wl,--no-whole-archive",
Expand All @@ -124,7 +124,7 @@ def _cdylib_has_native_dep_and_alwayslink_test_impl(ctx):
]
else:
want = [
"-lstatic=native_dep",
"link-arg=bazel-out/k8-fastbuild/bin/test/unit/native_deps/libnative_dep.a",
"link-arg=-Wl,--whole-archive",
"link-arg=bazel-out/k8-fastbuild/bin/test/unit/native_deps/libalwayslink.lo",
"link-arg=-Wl,--no-whole-archive",
Expand All @@ -133,10 +133,22 @@ def _cdylib_has_native_dep_and_alwayslink_test_impl(ctx):
return analysistest.end(env)

rlib_has_no_native_libs_test = analysistest.make(_rlib_has_no_native_libs_test_impl)
staticlib_has_native_libs_test = analysistest.make(_staticlib_has_native_libs_test_impl)
cdylib_has_native_libs_test = analysistest.make(_cdylib_has_native_libs_test_impl)
proc_macro_has_native_libs_test = analysistest.make(_proc_macro_has_native_libs_test_impl)
bin_has_native_libs_test = analysistest.make(_bin_has_native_libs_test_impl)
staticlib_has_native_libs_test = analysistest.make(_staticlib_has_native_libs_test_impl, attrs = {
"_macos_constraint": attr.label(default = Label("@platforms//os:macos")),
"_windows_constraint": attr.label(default = Label("@platforms//os:windows")),
})
cdylib_has_native_libs_test = analysistest.make(_cdylib_has_native_libs_test_impl, attrs = {
"_macos_constraint": attr.label(default = Label("@platforms//os:macos")),
"_windows_constraint": attr.label(default = Label("@platforms//os:windows")),
})
proc_macro_has_native_libs_test = analysistest.make(_proc_macro_has_native_libs_test_impl, attrs = {
"_macos_constraint": attr.label(default = Label("@platforms//os:macos")),
"_windows_constraint": attr.label(default = Label("@platforms//os:windows")),
})
bin_has_native_libs_test = analysistest.make(_bin_has_native_libs_test_impl, attrs = {
"_macos_constraint": attr.label(default = Label("@platforms//os:macos")),
"_windows_constraint": attr.label(default = Label("@platforms//os:windows")),
})
bin_has_native_dep_and_alwayslink_test = analysistest.make(_bin_has_native_dep_and_alwayslink_test_impl, attrs = {
"_macos_constraint": attr.label(default = Label("@platforms//os:macos")),
"_windows_constraint": attr.label(default = Label("@platforms//os:windows")),
Expand Down

0 comments on commit 4da6858

Please sign in to comment.