Add per-target link flags in the right place (#2963)

Previously we were adding these early on in the link args.

This was a bug - if some transitive dep foo has a linkopt of `-lbar`,
the `-lbar` needs to be later in the link args list than `-lfoo`.

Rather than arbitrarily adding them early on and hoping things work,
this adds the args for each library just after we add a dependency on
that library.

bindgen/private/bindgen.bzl

@@ -303,7 +303,7 @@ def _rust_bindgen_impl(ctx):
             open_arg = True
             continue
 
-    _, _, linker_env = get_linker_and_args(ctx, ctx.attr, "bin", cc_toolchain, feature_configuration, None)
+    _, _, linker_env = get_linker_and_args(ctx, "bin", cc_toolchain, feature_configuration, None)
     env.update(**linker_env)
 
     # Set the dynamic linker search path so that clang uses the libstdcxx from the toolchain.

cargo/private/cargo_build_script.bzl

@@ -385,7 +385,7 @@ def _cargo_build_script_impl(ctx):
     # Pull in env vars which may be required for the cc_toolchain to work (e.g. on OSX, the SDK version).
     # We hope that the linker env is sufficient for the whole cc_toolchain.
     cc_toolchain, feature_configuration = find_cc_toolchain(ctx)
-    linker, link_args, linker_env = get_linker_and_args(ctx, ctx.attr, "bin", cc_toolchain, feature_configuration, None)
+    linker, link_args, linker_env = get_linker_and_args(ctx, "bin", cc_toolchain, feature_configuration, None)
     env.update(**linker_env)
     env["LD"] = linker
     env["LDFLAGS"] = " ".join(_pwd_flags(link_args))

examples/hello_world/src/main.rs

@@ -18,5 +18,5 @@ use hello_lib::greeter;
 
 fn main() {
     let hello = greeter::Greeter::new("Hello");
-    hello.greet("world");
+    hello.greet("world!");
 }

rust/private/rustc.bzl

@@ -398,12 +398,11 @@ def get_cc_user_link_flags(ctx):
     """
     return ctx.fragments.cpp.linkopts
 
-def get_linker_and_args(ctx, attr, crate_type, cc_toolchain, feature_configuration, rpaths, add_flags_for_binary = False):
+def get_linker_and_args(ctx, crate_type, cc_toolchain, feature_configuration, rpaths, add_flags_for_binary = False):
     """Gathers cc_common linker information
 
     Args:
         ctx (ctx): The current target's context object
-        attr (struct): Attributes to use in gathering linker args
         crate_type (str): The target crate's type (i.e. "bin", "proc-macro", etc.).
         cc_toolchain (CcToolchain): cc_toolchain for which we are creating build variables.
         feature_configuration (FeatureConfiguration): Feature configuration to be queried.
@@ -437,13 +436,6 @@ def get_linker_and_args(ctx, attr, crate_type, cc_toolchain, feature_configurati
     else:
         fail("Unknown `crate_type`: {}".format(crate_type))
 
-    # Add linkopts from dependencies. This includes linkopts from transitive
-    # dependencies since they get merged up.
-    for dep in getattr(attr, "deps", []):
-        if CcInfo in dep and dep[CcInfo].linking_context:
-            for linker_input in dep[CcInfo].linking_context.linker_inputs.to_list():
-                for flag in linker_input.user_link_flags:
-                    user_link_flags.append(flag)
     link_variables = cc_common.create_link_variables(
         feature_configuration = feature_configuration,
         cc_toolchain = cc_toolchain,
@@ -1013,7 +1005,7 @@ def construct_arguments(
             else:
                 rpaths = depset()
 
-            ld, link_args, link_env = get_linker_and_args(ctx, attr, crate_info.type, cc_toolchain, feature_configuration, rpaths, add_flags_for_binary = add_flags_for_binary)
+            ld, link_args, link_env = get_linker_and_args(ctx, crate_info.type, cc_toolchain, feature_configuration, rpaths, add_flags_for_binary = add_flags_for_binary)
 
             env.update(link_env)
             rustc_flags.add(ld, format = "--codegen=linker=%s")
@@ -1957,6 +1949,9 @@ def _portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name, for_windows
 
     return []
 
+def _add_user_link_flags(ret, linker_input):
+    ret.extend(["--codegen=link-arg={}".format(flag) for flag in linker_input.user_link_flags])
+
 def _make_link_flags_windows(make_link_flags_args, flavor_msvc):
     linker_input, use_pic, ambiguous_libs, include_link_flags = make_link_flags_args
     ret = []
@@ -1975,6 +1970,7 @@ def _make_link_flags_windows(make_link_flags_args, flavor_msvc):
                 ])
         elif include_link_flags:
             ret.extend(_portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name_for_windows, for_windows = True, flavor_msvc = flavor_msvc))
+    _add_user_link_flags(ret, linker_input)
     return ret
 
 def _make_link_flags_windows_msvc(make_link_flags_args):
@@ -1994,6 +1990,7 @@ def _make_link_flags_darwin(make_link_flags_args):
             ])
         elif include_link_flags:
             ret.extend(_portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name_default, for_darwin = True))
+    _add_user_link_flags(ret, linker_input)
     return ret
 
 def _make_link_flags_default(make_link_flags_args):
@@ -2011,6 +2008,7 @@ def _make_link_flags_default(make_link_flags_args):
             ])
         elif include_link_flags:
             ret.extend(_portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name_default))
+    _add_user_link_flags(ret, linker_input)
     return ret
 
 def _libraries_dirnames(make_link_flags_args):

test/linker_inputs_propagation/BUILD.bazel

@@ -1,6 +1,7 @@
 load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library", "cc_test")
 load(
     "@rules_rust//rust:defs.bzl",
+    "rust_binary",
     "rust_library",
     "rust_shared_library",
     "rust_static_library",
@@ -20,9 +21,15 @@ cc_library(
         "foo_shared.cc",
     ],
     hdrs = ["foo_shared.h"],
-    target_compatible_with = [
-        "@platforms//os:linux",
+)
+
+cc_library(
+    name = "foo_with_linkopts",
+    srcs = [
+        "foo_shared.cc",
     ],
+    hdrs = ["foo_shared.h"],
+    linkopts = ["-L/doesnotexist"],
 )
 
 cc_binary(
@@ -74,21 +81,20 @@ rust_static_library(
     name = "staticlib_uses_foo",
     srcs = ["bar_uses_foo.rs"],
     edition = "2018",
-    target_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//os:windows",
-    ],
     deps = [":foo"],
 )
 
+rust_library(
+    name = "rlib_uses_foo_with_redundant_linkopts",
+    srcs = ["bar_uses_shared_foo_for_rust.rs"],
+    edition = "2018",
+    deps = [":foo_with_linkopts"],
+)
+
 rust_shared_library(
     name = "sharedlib_uses_foo",
     srcs = ["bar_uses_foo.rs"],
     edition = "2018",
-    target_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//os:windows",
-    ],
     deps = [":foo"],
 )
 
@@ -96,10 +102,6 @@ rust_static_library(
     name = "staticlib_uses_shared_foo",
     srcs = ["bar_uses_shared_foo.rs"],
     edition = "2018",
-    target_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//os:windows",
-    ],
     deps = [":import_foo_shared"],
 )
 
@@ -107,49 +109,62 @@ rust_static_library(
     name = "sharedlib_uses_shared_foo",
     srcs = ["bar_uses_shared_foo.rs"],
     edition = "2018",
-    target_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//os:windows",
-    ],
     deps = [":import_foo_shared"],
 )
 
 cc_test(
     name = "depends_on_foo_via_staticlib",
     srcs = ["baz.cc"],
-    target_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//os:windows",
-    ],
+    target_compatible_with = select({
+        "@platforms//os:linux": ["@platforms//:incompatible"],
+        "@platforms//os:macos": ["@platforms//:incompatible"],
+        "@platforms//os:windows": ["@platforms//:incompatible"],
+        "//conditions:default": [],
+    }),
     deps = [":staticlib_uses_foo"],
 )
 
 cc_test(
     name = "depends_on_foo_via_sharedlib",
     srcs = ["baz.cc"],
-    target_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//os:windows",
-    ],
+    target_compatible_with = select({
+        "@platforms//os:linux": ["@platforms//:incompatible"],
+        "@platforms//os:macos": ["@platforms//:incompatible"],
+        "@platforms//os:windows": ["@platforms//:incompatible"],
+        "//conditions:default": [],
+    }),
     deps = [":sharedlib_uses_foo"],
 )
 
 cc_test(
     name = "depends_on_shared_foo_via_sharedlib",
     srcs = ["baz.cc"],
-    target_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//os:windows",
-    ],
+    target_compatible_with = select({
+        "@platforms//os:macos": ["@platforms//:incompatible"],
+        "@platforms//os:windows": ["@platforms//:incompatible"],
+        "//conditions:default": [],
+    }),
     deps = [":sharedlib_uses_shared_foo"],
 )
 
 cc_test(
     name = "depends_on_shared_foo_via_staticlib",
     srcs = ["baz.cc"],
-    target_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//os:windows",
-    ],
+    target_compatible_with = select({
+        "@platforms//os:macos": ["@platforms//:incompatible"],
+        "@platforms//os:windows": ["@platforms//:incompatible"],
+        "//conditions:default": [],
+    }),
     deps = [":staticlib_uses_shared_foo"],
 )
+
+rust_binary(
+    name = "depends_on_foo_with_redundant_linkopts",
+    srcs = ["main_uses_foo.rs"],
+    edition = "2021",
+    target_compatible_with = select({
+        "@platforms//os:windows": ["@platforms//:incompatible"],
+        "//conditions:default": [],
+    }),
+    deps = [":rlib_uses_foo_with_redundant_linkopts"],
+)

test/linker_inputs_propagation/bar_uses_shared_foo_for_rust.rs

@@ -0,0 +1,12 @@
+extern "C" {
+    pub fn foo() -> i32;
+}
+
+/** Safety doc.
+
+  # Safety
+
+*/
+pub fn double_foo() -> i32 {
+    2 * unsafe { foo() }
+}

test/linker_inputs_propagation/main_uses_foo.rs

@@ -0,0 +1,5 @@
+use rlib_uses_foo_with_redundant_linkopts::double_foo;
+
+fn main() {
+    println!("{}", double_foo());
+}

test/unit/linker_inputs_propagation/linker_inputs_propagation_test.bzl

@@ -1,6 +1,6 @@
 """Unittests for propagation of linker inputs through Rust libraries"""
 
-load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts")
+load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts", "unittest")
 
 def _shared_lib_is_propagated_test_impl(ctx):
     env = analysistest.begin(ctx)
@@ -8,7 +8,7 @@ def _shared_lib_is_propagated_test_impl(ctx):
     link_action = [action for action in tut.actions if action.mnemonic == "CppLink"][0]
 
     lib_name = _get_lib_name(ctx, name = "foo_shared")
-    asserts.true(env, _contains_input(link_action.inputs, lib_name))
+    _assert_contains_input(env, link_action.inputs, lib_name)
 
     return analysistest.end(env)
 
@@ -18,17 +18,34 @@ def _static_lib_is_not_propagated_test_impl(ctx):
     link_action = [action for action in tut.actions if action.mnemonic == "CppLink"][0]
 
     lib_name = _get_lib_name(ctx, name = "foo")
-    asserts.false(env, _contains_input(link_action.inputs, lib_name))
+    asserts.false(env, _assert_contains_input(env, link_action.inputs, lib_name))
 
     return analysistest.end(env)
 
-def _contains_input(inputs, name):
+def _dependency_linkopts_are_propagated_test_impl(ctx):
+    env = analysistest.begin(ctx)
+    tut = analysistest.target_under_test(env)
+    link_action = [action for action in tut.actions if action.mnemonic == "Rustc"][0]
+
+    # Expect a library's own linkopts to come after the flags we create to link them.
+    # This is required, because linkopts are ordered and the linker will only apply later ones when resolving symbols required for earlier ones.
+    # This means that if one of our transitive deps has a linkopt like `-lfoo`, the dep will see the symbols of foo at link time.
+    _assert_contains_in_order(env, link_action.argv, ["-lstatic=foo_with_linkopts", "-Clink-arg=-lfoo_with_linkopts", "--codegen=link-arg=-L/doesnotexist"])
+    return analysistest.end(env)
+
+def _assert_contains_input(env, inputs, name):
     for input in inputs.to_list():
         # We cannot check for name equality because rlib outputs contain
         # a hash in their name.
         if input.basename.startswith(name):
-            return True
-    return False
+            return
+    unittest.fail(env, "Expected {} to contain a library starting with {}".format(inputs.to_list(), name))
+
+def _assert_contains_in_order(env, haystack, needle):
+    for i in range(len(haystack)):
+        if haystack[i:i + len(needle)] == needle:
+            return
+    unittest.fail(env, "Expected {} to contain {}".format(haystack, needle))
 
 def _get_lib_name(ctx, name):
     if ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]):
@@ -51,6 +68,10 @@ shared_lib_is_propagated_test = analysistest.make(
     },
 )
 
+dependency_linkopts_are_propagated_test = analysistest.make(
+    _dependency_linkopts_are_propagated_test_impl,
+)
+
 def _linker_inputs_propagation_test():
     static_lib_is_not_propagated_test(
         name = "depends_on_foo_via_staticlib",
@@ -72,6 +93,11 @@ def _linker_inputs_propagation_test():
         target_under_test = "//test/linker_inputs_propagation:depends_on_shared_foo_via_sharedlib",
     )
 
+    dependency_linkopts_are_propagated_test(
+        name = "dependency_linkopts_are_propagated",
+        target_under_test = "//test/linker_inputs_propagation:depends_on_foo_with_redundant_linkopts",
+    )
+
 def linker_inputs_propagation_test_suite(name):
     """Entry-point macro called from the BUILD file.
 
@@ -87,5 +113,6 @@ def linker_inputs_propagation_test_suite(name):
             ":depends_on_shared_foo_via_staticlib",
             ":depends_on_foo_via_sharedlib",
             ":depends_on_shared_foo_via_sharedlib",
+            ":dependency_linkopts_are_propagated",
         ],
     )