Address review comments

Author: gigaroby

rust/private/providers.bzl

@@ -50,7 +50,7 @@ DepInfo = provider(
         "transitive_build_infos": "depset[BuildInfo]",
         "transitive_crate_outputs": "depset[File]: All transitive crate outputs.",
         "transitive_crates": "depset[CrateInfo]",
-        "transitive_metadata_outputs": "depset[File]: All transitive metadata dependencies (.rlib, for crates that provide them) and all transitive object dependencies (.rlib) for crates that don't provide metadata.",
+        "transitive_metadata_outputs": "depset[File]: All transitive metadata dependencies (.rmeta, for crates that provide them) and all transitive object dependencies (.rlib) for crates that don't provide metadata.",
         "transitive_noncrates": "depset[LinkerInput]: All transitive dependencies that aren't crates.",
     },
 )

rust/private/rustc.bzl

@@ -542,9 +542,7 @@ def _depend_on_metadata(crate_info, force_depend_on_objects):
     if force_depend_on_objects:
         return False
 
-    is_lib = crate_info.type in ("rlib", "lib")
-    build_metadata = crate_info.metadata != None
-    return is_lib and build_metadata
+    return crate_info.type in ("rlib", "lib")
 
 def collect_inputs(
         ctx,
@@ -705,7 +703,7 @@ def construct_arguments(
         remap_path_prefix = ".",
         use_json_output = False,
         build_metadata = False,
-        force_link_to_objects = False):
+        force_depend_on_objects = False):
     """Builds an Args object containing common rustc flags
 
     Args:
@@ -734,7 +732,7 @@ def construct_arguments(
         remap_path_prefix (str, optional): A value used to remap `${pwd}` to. If set to a falsey value, no prefix will be set.
         use_json_output (bool): Have rustc emit json and process_wrapper parse json messages to output rendered output.
         build_metadata (bool): Generate CLI arguments for building *only* .rmeta files. This requires use_json_output.
-        force_link_to_objects (bool): Force linking to concrete (rlib) dependencies, instead of metadata (rlib) even if they are available.
+        force_depend_on_objects (bool): Force using `.rlib` object files instead of metadata (`.rmeta`) files even if they are available.
 
     Returns:
         tuple: A tuple of the following items
@@ -842,9 +840,9 @@ def construct_arguments(
 
         error_format = "json"
 
-        if build_metadata:
-            # Configure process_wrapper to terminate rustc when metadata are emitted
-            process_wrapper_flags.add("--rustc-quit-on-rmeta", "true")
+    if build_metadata:
+        # Configure process_wrapper to terminate rustc when metadata are emitted
+        process_wrapper_flags.add("--rustc-quit-on-rmeta", "true")
 
     rustc_flags.add("--error-format=" + error_format)
 
@@ -918,7 +916,7 @@ def construct_arguments(
 
         _add_native_link_flags(rustc_flags, dep_info, linkstamp_outs, ambiguous_libs, crate_info.type, toolchain, cc_toolchain, feature_configuration)
 
-    use_metadata = _depend_on_metadata(crate_info, force_link_to_objects)
+    use_metadata = _depend_on_metadata(crate_info, force_depend_on_objects)
 
     # These always need to be added, even if not linking this crate.
     add_crate_link_flags(rustc_flags, dep_info, force_all_deps_direct, use_metadata)

rust/private/rustdoc.bzl

@@ -121,7 +121,7 @@ def rustdoc_compile_action(
         emit = [],
         remap_path_prefix = None,
         force_link = True,
-        force_link_to_objects = is_test,
+        force_depend_on_objects = is_test,
     )
 
     # Because rustdoc tests compile tests outside of the sandbox, the sysroot

test/unit/pipelined_compilation/pipelined_compilation_test.bzl

@@ -112,7 +112,7 @@ def _pipelined_compilation_test():
     second_lib_test(name = "second_lib_test", target_under_test = ":second", target_compatible_with = NOT_WINDOWS)
     bin_test(name = "bin_test", target_under_test = ":bin", target_compatible_with = NOT_WINDOWS)
 
-def _custom_rule_test_impl(ctx):
+def _rmeta_is_propagated_through_custom_rule_test_impl(ctx):
     env = analysistest.begin(ctx)
     tut = analysistest.target_under_test(env)
 
@@ -124,54 +124,88 @@ def _custom_rule_test_impl(ctx):
     rlib_inputs = [i for i in rust_action.inputs.to_list() if i.path.endswith(".rlib")]
 
     seen_wrapper_metadata = False
-    seen_to_wrap = False
+    seen_to_wrap_metadata = False
     for mi in metadata_inputs:
         if "libwrapper" in mi.path:
             seen_wrapper_metadata = True
         if "libto_wrap" in mi.path:
-            seen_to_wrap = True
+            seen_to_wrap_metadata = True
 
-    seen_wrapper_rlib = True
+    seen_wrapper_rlib = False
+    seen_to_wrap_rlib = False
     for ri in rlib_inputs:
         if "libwrapper" in ri.path:
             seen_wrapper_rlib = True
+        if "libto_wrap" in ri.path:
+            seen_to_wrap_rlib = True
 
     if ctx.attr.generate_metadata:
         asserts.true(env, seen_wrapper_metadata, "expected dependency on metadata for 'wrapper' but not found")
+        asserts.false(env, seen_wrapper_rlib, "expected no dependency on object for 'wrapper' but it was found")
     else:
-        asserts.true(env, seen_wrapper_rlib, "expected dependency on rlib for 'wrapper' but not found")
+        asserts.true(env, seen_wrapper_rlib, "expected dependency on object for 'wrapper' but not found")
+        asserts.false(env, seen_wrapper_metadata, "expected no dependency on metadata for 'wrapper' but it was found")
 
-    asserts.true(env, seen_to_wrap, "expected dependency on metadata for 'to_wrap' but not found")
+    asserts.true(env, seen_to_wrap_metadata, "expected dependency on metadata for 'to_wrap' but not found")
+    asserts.false(env, seen_to_wrap_rlib, "expected no dependency on object for 'to_wrap' but it was found")
 
     return analysistest.end(env)
 
-custom_rule_test = analysistest.make(_custom_rule_test_impl, attrs = {"generate_metadata": attr.bool()}, config_settings = ENABLE_PIPELINING)
+def _rmeta_is_used_when_building_custom_rule_test_impl(ctx):
+    env = analysistest.begin(ctx)
+    tut = analysistest.target_under_test(env)
+
+    # This is the custom rule invocation of rustc.
+    rust_action = [act for act in tut.actions if act.mnemonic == "Rustc"][0]
 
-def _custom_rule_test(generate_metadata, prefix):
+    # We want to check that the action depends on metadata, regardless of ctx.attr.generate_metadata
+    seen_to_wrap_rlib = False
+    seen_to_wrap_rmeta = False
+    for act in rust_action.inputs.to_list():
+        if "libto_wrap" in act.path and act.path.endswith('.rlib'):
+            seen_to_wrap_rlib = True
+        elif "libto_wrap" in act.path and act.path.endswith('.rmeta'):
+            seen_to_wrap_rmeta = True
+
+    asserts.true(env, seen_to_wrap_rmeta, "expected dependency on metadata for 'to_wrap' but not found")
+    asserts.false(env, seen_to_wrap_rlib, "expected no dependency on object for 'to_wrap' but it was found")
+
+    return analysistest.end(env)
+
+rmeta_is_propagated_through_custom_rule_test = analysistest.make(_rmeta_is_propagated_through_custom_rule_test_impl, attrs = {"generate_metadata": attr.bool()}, config_settings = ENABLE_PIPELINING)
+rmeta_is_used_when_building_custom_rule_test = analysistest.make(_rmeta_is_used_when_building_custom_rule_test_impl, config_settings = ENABLE_PIPELINING)
+
+def _custom_rule_test(generate_metadata, suffix):
     rust_library(
-        name = "to_wrap" + prefix,
+        name = "to_wrap" + suffix,
         crate_name = "to_wrap",
         srcs = ["custom_rule_test/to_wrap.rs"],
         edition = "2021",
     )
     wrap(
-        name = "wrapper" + prefix,
+        name = "wrapper" + suffix,
         crate_name = "wrapper",
-        target = ":to_wrap" + prefix,
+        target = ":to_wrap" + suffix,
         generate_metadata = generate_metadata,
     )
     rust_library(
-        name = "uses_wrapper" + prefix,
+        name = "uses_wrapper" + suffix,
         srcs = ["custom_rule_test/uses_wrapper.rs"],
-        deps = [":wrapper" + prefix],
+        deps = [":wrapper" + suffix],
         edition = "2021",
     )
 
-    custom_rule_test(
-        name = "custom_rule_test" + prefix,
+    rmeta_is_propagated_through_custom_rule_test(
+        name = "rmeta_is_propagated_through_custom_rule_test" + suffix,
         generate_metadata = generate_metadata,
         target_compatible_with = NOT_WINDOWS,
-        target_under_test = ":uses_wrapper" + prefix,
+        target_under_test = ":uses_wrapper" + suffix,
+    )
+
+    rmeta_is_used_when_building_custom_rule_test(
+        name = "rmeta_is_used_when_building_custom_rule_test" + suffix,
+        target_compatible_with = NOT_WINDOWS,
+        target_under_test = ":wrapper" + suffix,
     )
 
 def pipelined_compilation_test_suite(name):
@@ -181,15 +215,17 @@ def pipelined_compilation_test_suite(name):
         name: Name of the macro.
     """
     _pipelined_compilation_test()
-    _custom_rule_test(generate_metadata = True, prefix = "_with_metadata")
-    _custom_rule_test(generate_metadata = False, prefix = "_without_metadata")
+    _custom_rule_test(generate_metadata = True, suffix = "_with_metadata")
+    _custom_rule_test(generate_metadata = False, suffix = "_without_metadata")
 
     native.test_suite(
         name = name,
         tests = [
             ":bin_test",
             ":second_lib_test",
-            ":custom_rule_test_with_metadata",
-            ":custom_rule_test_without_metadata",
+            ":rmeta_is_propagated_through_custom_rule_test_with_metadata",
+            ":rmeta_is_propagated_through_custom_rule_test_without_metadata",
+            ":rmeta_is_used_when_building_custom_rule_test_with_metadata",
+            ":rmeta_is_used_when_building_custom_rule_test_without_metadata",
         ],
     )

test/unit/pipelined_compilation/wrap.bzl

@@ -78,7 +78,6 @@ EOF
             is_test = False,
         ),
         output_hash = output_hash,
-        force_all_deps_direct = True,
     )
 
 wrap = rule(