From d766c3ce14c4969a246dea83816ecfc8a9183a91 Mon Sep 17 00:00:00 2001 From: Roberto Bampi Date: Wed, 23 Mar 2022 12:17:53 +0100 Subject: [PATCH 1/7] rules_rust: enable pipelined compilation. Pipelined compilation allows better parallelism during builds as it allows libraries to generate lightweight metadata files to unlock other depencies. These metadata files (.rmeta) can only be used to unlock library -> library dependencies and do not affect builds in any other way. This is currently the default in cargo: https://internals.rust-lang.org/t/evaluating-pipelined-rustc-compilation/10199. Pipelined compilation will be disabled by default and will need to be enabled via flag. Pipelined compilation is not supported on windows and will thus always be disabled. --- .bazelci/presubmit.yml | 1 + docs/flatten.md | 9 +- docs/providers.md | 9 +- proto/proto.bzl | 10 +- rust/private/common.bzl | 2 + rust/private/providers.bzl | 2 + rust/private/rust.bzl | 11 +- rust/private/rustc.bzl | 229 ++++++++++++++++-- rust/private/rustdoc.bzl | 4 + rust/private/utils.bzl | 22 ++ rust/settings/BUILD.bazel | 8 + rust/toolchain.bzl | 5 + test/process_wrapper/rustc_quit_on_rmeta.rs | 11 +- test/unit/force_all_deps_direct/generator.bzl | 13 + test/unit/pipelined_compilation/BUILD.bazel | 4 + test/unit/pipelined_compilation/bin.rs | 5 + test/unit/pipelined_compilation/first.rs | 4 + test/unit/pipelined_compilation/my_macro.rs | 6 + .../pipelined_compilation_test.bzl | 125 ++++++++++ test/unit/pipelined_compilation/second.rs | 7 + util/process_wrapper/main.rs | 53 ++-- util/process_wrapper/options.rs | 5 +- util/process_wrapper/output.rs | 4 +- util/process_wrapper/rustc.rs | 72 ++++-- 24 files changed, 541 insertions(+), 80 deletions(-) create mode 100644 test/unit/pipelined_compilation/BUILD.bazel create mode 100644 test/unit/pipelined_compilation/bin.rs create mode 100644 test/unit/pipelined_compilation/first.rs create mode 100644 test/unit/pipelined_compilation/my_macro.rs create mode 100644 test/unit/pipelined_compilation/pipelined_compilation_test.bzl create mode 100644 test/unit/pipelined_compilation/second.rs diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index 06d1868147..864496d7b9 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml @@ -15,6 +15,7 @@ default_windows_targets: &default_windows_targets - "-//bindgen/..." - "-//test/proto/..." - "-//tools/rust_analyzer/..." + - "-//test/unit/pipelined_compilation/..." crate_universe_vendor_example_targets: &crate_universe_vendor_example_targets - "//vendor_external:crates_vendor" - "//vendor_local_manifests:crates_vendor" diff --git a/docs/flatten.md b/docs/flatten.md index 725a9fc8b0..d70d2e2ddf 100644 --- a/docs/flatten.md +++ b/docs/flatten.md @@ -1350,8 +1350,8 @@ A test rule for performing `rustfmt --check` on a set of targets ## CrateInfo 
-CrateInfo(aliases, compile_data, deps, edition, is_test, name, output, owner, proc_macro_deps, root,
-          rustc_env, srcs, type, wrapped_crate_type)
+CrateInfo(aliases, compile_data, deps, edition, is_test, metadata, name, output, owner,
+          proc_macro_deps, root, rustc_env, srcs, type, wrapped_crate_type)
A provider containing general Crate information. @@ -1366,6 +1366,7 @@ A provider containing general Crate information. | deps | depset[DepVariantInfo]: This crate's (rust or cc) dependencies' providers. | | edition | str: The edition of this crate. | | is_test | bool: If the crate is being compiled in a test context | +| metadata | File: The rmeta file produced for this crate. It is optional. | | name | str: The name of this crate. | | output | File: The output File that will be produced, depends on crate type. | | owner | Label: The label of the target that produced this CrateInfo | @@ -1383,7 +1384,8 @@ A provider containing general Crate information. 
 DepInfo(dep_env, direct_crates, link_search_path_files, transitive_build_infos,
-        transitive_crate_outputs, transitive_crates, transitive_noncrates)
+        transitive_crate_outputs, transitive_crates, transitive_metadata_outputs,
+        transitive_noncrates)
A provider containing information about a Crate's dependencies. @@ -1399,6 +1401,7 @@ A provider containing information about a Crate's dependencies. | 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 crate metadata outputs. | | transitive_noncrates | depset[LinkerInput]: All transitive dependencies that aren't crates. | diff --git a/docs/providers.md b/docs/providers.md index 884a1542d1..fa5a1cef5d 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -10,8 +10,8 @@ ## CrateInfo 
-CrateInfo(aliases, compile_data, deps, edition, is_test, name, output, owner, proc_macro_deps, root,
-          rustc_env, srcs, type, wrapped_crate_type)
+CrateInfo(aliases, compile_data, deps, edition, is_test, metadata, name, output, owner,
+          proc_macro_deps, root, rustc_env, srcs, type, wrapped_crate_type)
A provider containing general Crate information. @@ -26,6 +26,7 @@ A provider containing general Crate information. | deps | depset[DepVariantInfo]: This crate's (rust or cc) dependencies' providers. | | edition | str: The edition of this crate. | | is_test | bool: If the crate is being compiled in a test context | +| metadata | File: The rmeta file produced for this crate. It is optional. | | name | str: The name of this crate. | | output | File: The output File that will be produced, depends on crate type. | | owner | Label: The label of the target that produced this CrateInfo | @@ -43,7 +44,8 @@ A provider containing general Crate information. 
 DepInfo(dep_env, direct_crates, link_search_path_files, transitive_build_infos,
-        transitive_crate_outputs, transitive_crates, transitive_noncrates)
+        transitive_crate_outputs, transitive_crates, transitive_metadata_outputs,
+        transitive_noncrates)
A provider containing information about a Crate's dependencies. @@ -59,6 +61,7 @@ A provider containing information about a Crate's dependencies. | 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 crate metadata outputs. | | transitive_noncrates | depset[LinkerInput]: All transitive dependencies that aren't crates. | diff --git a/proto/proto.bzl b/proto/proto.bzl index 9cb83f3e7e..b5e65a3392 100644 --- a/proto/proto.bzl +++ b/proto/proto.bzl @@ -44,7 +44,7 @@ load("//rust:defs.bzl", "rust_common") load("//rust/private:rustc.bzl", "rustc_compile_action") # buildifier: disable=bzl-visibility -load("//rust/private:utils.bzl", "compute_crate_name", "determine_output_hash", "find_toolchain", "transform_deps") +load("//rust/private:utils.bzl", "can_build_metadata", "compute_crate_name", "determine_output_hash", "find_toolchain", "transform_deps") RustProtoInfo = provider( doc = "Rust protobuf provider info", @@ -212,6 +212,13 @@ def _rust_proto_compile(protos, descriptor_sets, imports, crate_name, ctx, is_gr crate_name, output_hash, )) + rust_metadata = None + if can_build_metadata(toolchain, ctx, "rlib"): + rust_metadata = ctx.actions.declare_file("%s/lib%s-%s.rmeta" % ( + output_dir, + crate_name, + output_hash, + )) # Gather all dependencies for compilation compile_action_deps = depset( @@ -234,6 +241,7 @@ def _rust_proto_compile(protos, descriptor_sets, imports, crate_name, ctx, is_gr proc_macro_deps = depset([]), aliases = {}, output = rust_lib, + metadata = rust_metadata, edition = proto_toolchain.edition, rustc_env = {}, is_test = False, diff --git a/rust/private/common.bzl b/rust/private/common.bzl index 1cf84cb1c1..3de7cb54f9 100644 --- a/rust/private/common.bzl +++ b/rust/private/common.bzl @@ -47,6 +47,8 @@ def _create_crate_info(**kwargs): """ if not "wrapped_crate_type" in kwargs: kwargs.update({"wrapped_crate_type": None}) + if not "metadata" in kwargs: + kwargs.update({"metadata": None}) return CrateInfo(**kwargs) rust_common = struct( diff --git a/rust/private/providers.bzl b/rust/private/providers.bzl index 0a1d924e83..7e28244e83 100644 --- a/rust/private/providers.bzl +++ b/rust/private/providers.bzl @@ -22,6 +22,7 @@ CrateInfo = provider( "deps": "depset[DepVariantInfo]: This crate's (rust or cc) dependencies' providers.", "edition": "str: The edition of this crate.", "is_test": "bool: If the crate is being compiled in a test context", + "metadata": "File: The rmeta file produced for this crate. It is optional.", "name": "str: The name of this crate.", "output": "File: The output File that will be produced, depends on crate type.", "owner": "Label: The label of the target that produced this CrateInfo", @@ -49,6 +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 crate metadata outputs.", "transitive_noncrates": "depset[LinkerInput]: All transitive dependencies that aren't crates.", }, ) diff --git a/rust/private/rust.bzl b/rust/private/rust.bzl index f50303e3c9..c6361bdfab 100644 --- a/rust/private/rust.bzl +++ b/rust/private/rust.bzl @@ -13,10 +13,12 @@ # limitations under the License. # buildifier: disable=module-docstring +load("@bazel_skylib//lib:paths.bzl", "paths") load("//rust/private:common.bzl", "rust_common") load("//rust/private:rustc.bzl", "rustc_compile_action") load( "//rust/private:utils.bzl", + "can_build_metadata", "compute_crate_name", "dedent", "determine_output_hash", @@ -25,7 +27,6 @@ load( "get_import_macro_deps", "transform_deps", ) - # TODO(marco): Separate each rule into its own file. def _assert_no_deprecated_attributes(_ctx): @@ -316,6 +317,13 @@ def _rust_library_common(ctx, crate_type): ) rust_lib = ctx.actions.declare_file(rust_lib_name) + rust_metadata = None + if can_build_metadata(toolchain, ctx, crate_type): + rust_metadata = ctx.actions.declare_file( + paths.replace_extension(rust_lib_name, ".rmeta"), + sibling = rust_lib, + ) + deps = transform_deps(ctx.attr.deps) proc_macro_deps = transform_deps(ctx.attr.proc_macro_deps + get_import_macro_deps(ctx)) @@ -332,6 +340,7 @@ def _rust_library_common(ctx, crate_type): proc_macro_deps = depset(proc_macro_deps), aliases = ctx.attr.aliases, output = rust_lib, + metadata = rust_metadata, edition = get_edition(ctx.attr, toolchain, ctx.label), rustc_env = ctx.attr.rustc_env, is_test = False, diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index 21fb46c15e..49622ee834 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -169,6 +169,9 @@ def _should_use_pic(cc_toolchain, feature_configuration, crate_type): return cc_toolchain.needs_pic_for_dynamic_libraries(feature_configuration = feature_configuration) return False +def _is_proc_macro(crate_info): + return "proc-macro" in (crate_info.type, crate_info.wrapped_crate_type) + def collect_deps( deps, proc_macro_deps, @@ -197,6 +200,7 @@ def collect_deps( build_info = None linkstamps = [] transitive_crate_outputs = [] + transitive_metadata_outputs = [] aliases = {k.label: v for k, v in aliases.items()} for dep in depset(transitive = [deps, proc_macro_deps]).to_list(): @@ -222,19 +226,35 @@ def collect_deps( transitive_crates.append( depset( [crate_info], - transitive = [] if "proc-macro" in [ - crate_info.type, - crate_info.wrapped_crate_type, - ] else [dep_info.transitive_crates], + transitive = [] if _is_proc_macro(crate_info) else [dep_info.transitive_crates], + ), + ) + + # If this dependency produces metadata, add it to the metadata outputs. + # If it doesn't (for example a custom library that exports crate_info), + # we depend on crate_info.output. + depend_on = crate_info.metadata + if not crate_info.metadata: + depend_on = crate_info.output + + transitive_metadata_outputs.append( + depset( + [depend_on], + transitive = [dep_info.transitive_metadata_outputs], ), ) + + # If this dependency is a proc_macro, it still can be used for lib crates + # that produce metadata. + if _is_proc_macro(crate_info): + transitive_metadata_outputs.append( + depset([crate_info.output]), + ) + transitive_crate_outputs.append( depset( [crate_info.output], - transitive = [] if "proc-macro" in [ - crate_info.type, - crate_info.wrapped_crate_type, - ] else [dep_info.transitive_crate_outputs], + transitive = [] if _is_proc_macro(crate_info) else [dep_info.transitive_crate_outputs], ), ) @@ -269,6 +289,7 @@ def collect_deps( order = "topological", # dylib link flag ordering matters. ), transitive_crate_outputs = depset(transitive = transitive_crate_outputs), + transitive_metadata_outputs = depset(transitive = transitive_metadata_outputs), transitive_build_infos = depset(transitive = transitive_build_infos), link_search_path_files = depset(transitive = transitive_link_search_paths), dep_env = build_info.dep_env if build_info else None, @@ -505,6 +526,30 @@ def _disambiguate_libs(actions, toolchain, crate_info, dep_info, use_pic): visited_libs[name] = artifact return ambiguous_libs +def _depend_on_metadata(crate_info, force_depend_on_objects): + """Determines if we can depend on metadata for this crate. + + By default (when pipelining is disabled or when the crate type needs to link against + objects) we depend on the set of object files (.rlib). + When pipelining is enabled and the crate type supports depending on metadata, + we depend on metadata files only (.rmeta). + In some rare cases, even if both of those conditions are true, we still want to + depend on objects. This is what force_depend_on_objects is. + + Args: + crate_info (CrateInfo): The Crate to determine this for. + force_depend_on_objects (bool): if set we will not depend on metadata. + + Returns: + Whether we can depend on metadata for this crate. + """ + 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 + def collect_inputs( ctx, file, @@ -516,7 +561,8 @@ def collect_inputs( crate_info, dep_info, build_info, - stamp = False): + stamp = False, + force_depend_on_objects = False): """Gather's the inputs and required input information for a rustc action Args: @@ -532,6 +578,8 @@ def collect_inputs( build_info (BuildInfo): The target Crate's build settings. stamp (bool, optional): Whether or not workspace status stamping is enabled. For more details see https://docs.bazel.build/versions/main/user-manual.html#flag--stamp + force_depend_on_objects (bool, optional): Forces dependencies of this rule to be objects rather than + metadata, even for libraries. This is used in rustdoc tests. Returns: tuple: A tuple: A tuple of the following items: @@ -572,6 +620,10 @@ def collect_inputs( # change. linkstamp_outs = [] + transitive_crate_outputs = dep_info.transitive_crate_outputs + if _depend_on_metadata(crate_info, force_depend_on_objects): + transitive_crate_outputs = dep_info.transitive_metadata_outputs + nolinkstamp_compile_inputs = depset( getattr(files, "data", []) + ([build_info.rustc_env, build_info.flags] if build_info else []) + @@ -580,7 +632,7 @@ def collect_inputs( transitive = [ linker_depset, crate_info.srcs, - dep_info.transitive_crate_outputs, + transitive_crate_outputs, depset(additional_transitive_inputs), crate_info.compile_data, toolchain.all_files, @@ -654,7 +706,10 @@ def construct_arguments( force_all_deps_direct = False, force_link = False, stamp = False, - remap_path_prefix = "."): + remap_path_prefix = ".", + use_json_output = False, + build_metadata = False, + force_link_to_objects = False): """Builds an Args object containing common rustc flags Args: @@ -681,6 +736,9 @@ def construct_arguments( stamp (bool, optional): Whether or not workspace status stamping is enabled. For more details see https://docs.bazel.build/versions/main/user-manual.html#flag--stamp 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. Returns: tuple: A tuple of the following items @@ -692,6 +750,9 @@ def construct_arguments( This is to be passed to the `arguments` parameter of actions - (dict): Common rustc environment variables """ + if build_metadata and not use_json_output: + fail("build_metadata requires parse_json_output") + output_dir = getattr(crate_info.output, "dirname", None) linker_script = getattr(file, "linker_script", None) @@ -761,8 +822,35 @@ def construct_arguments( rustc_flags.add(crate_info.root) rustc_flags.add("--crate-name=" + crate_info.name) rustc_flags.add("--crate-type=" + crate_info.type) + + error_format = "human" if hasattr(attr, "_error_format"): - rustc_flags.add("--error-format=" + attr._error_format[ErrorFormatInfo].error_format) + error_format = attr._error_format[ErrorFormatInfo].error_format + + if use_json_output: + # If --error-format was set to json, we just pass the output through + # Otherwise process_wrapper uses the "rendered" field. + process_wrapper_flags.add("--rustc-output-format", "json" if error_format == "json" else "rendered") + + # Configure rustc json output by adding artifact notifications. + # These will always be filtered out by process_wrapper and will be use to terminate + # rustc when appropriate. + json = ["artifacts"] + if error_format == "short": + json.append("diagnostic-short") + elif error_format == "human" and toolchain.os != "windows": + # If the os is not windows, we can get colorized output. + json.append("diagnostic-rendered-ansi") + + rustc_flags.add("--json=" + ",".join(json)) + + 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") + + rustc_flags.add("--error-format=" + error_format) # Mangle symbols to disambiguate crates with the same name. This could # happen only for non-final artifacts where we compute an output_hash, @@ -789,7 +877,9 @@ def construct_arguments( if emit: rustc_flags.add("--emit=" + ",".join(emit_with_paths)) - rustc_flags.add("--color=always") + if error_format != "json": + # Color is not compatible with json output. + rustc_flags.add("--color=always") rustc_flags.add("--target=" + toolchain.target_flag_value) if hasattr(attr, "crate_features"): rustc_flags.add_all(getattr(attr, "crate_features"), before_each = "--cfg", format_each = 'feature="%s"') @@ -832,11 +922,12 @@ def construct_arguments( _add_native_link_flags(rustc_flags, dep_info, linkstamp_outs, ambiguous_libs, crate_info.type, toolchain, cc_toolchain, feature_configuration) + link_to_metadata = _depend_on_metadata(crate_info, force_link_to_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) + add_crate_link_flags(rustc_flags, dep_info, force_all_deps_direct, link_to_metadata = link_to_metadata) - needs_extern_proc_macro_flag = "proc-macro" in [crate_info.type, crate_info.wrapped_crate_type] and \ - crate_info.edition != "2015" + needs_extern_proc_macro_flag = _is_proc_macro(crate_info) and crate_info.edition != "2015" if needs_extern_proc_macro_flag: rustc_flags.add("--extern") rustc_flags.add("proc_macro") @@ -919,6 +1010,8 @@ def rustc_compile_action( - (DepInfo): The transitive dependencies of this crate. - (DefaultInfo): The output file for this crate, and its runfiles. """ + build_metadata = getattr(crate_info, "metadata", None) + cc_toolchain, feature_configuration = find_cc_toolchain(ctx) dep_info, build_info, linkstamps = collect_deps( @@ -948,6 +1041,14 @@ def rustc_compile_action( stamp = stamp, ) + # If we build metadata, we need to keep the command line of the two invocations + # (rlib and rmeta) as similar as possible, otherwise rustc rejects the rmeta as + # a candidate. + # Because of that we need to add emit=metadata to both the rlib and rmeta invocation. + emit = ["dep-info", "link"] + if build_metadata: + emit.append("metadata") + args, env_from_args = construct_arguments( ctx = ctx, attr = attr, @@ -955,6 +1056,7 @@ def rustc_compile_action( toolchain = toolchain, tool_path = toolchain.rustc.path, cc_toolchain = cc_toolchain, + emit = emit, feature_configuration = feature_configuration, crate_info = crate_info, dep_info = dep_info, @@ -967,8 +1069,35 @@ def rustc_compile_action( build_flags_files = build_flags_files, force_all_deps_direct = force_all_deps_direct, stamp = stamp, + use_json_output = bool(build_metadata), ) + args_metadata = None + if build_metadata: + args_metadata, _ = construct_arguments( + ctx = ctx, + attr = attr, + file = ctx.file, + toolchain = toolchain, + tool_path = toolchain.rustc.path, + cc_toolchain = cc_toolchain, + emit = emit, + feature_configuration = feature_configuration, + crate_info = crate_info, + dep_info = dep_info, + linkstamp_outs = linkstamp_outs, + ambiguous_libs = ambiguous_libs, + output_hash = output_hash, + rust_flags = rust_flags, + out_dir = out_dir, + build_env_files = build_env_files, + build_flags_files = build_flags_files, + force_all_deps_direct = force_all_deps_direct, + stamp = stamp, + use_json_output = True, + build_metadata = True, + ) + env = dict(ctx.configuration.default_shell_env) env.update(env_from_args) @@ -1019,10 +1148,25 @@ def rustc_compile_action( len(crate_info.srcs.to_list()), ), ) + if args_metadata: + ctx.actions.run( + executable = ctx.executable._process_wrapper, + inputs = compile_inputs, + outputs = [build_metadata], + env = env, + arguments = args_metadata.all, + mnemonic = "RustcMetadata", + progress_message = "Compiling Rust metadata {} {}{} ({} files)".format( + crate_info.type, + ctx.label.name, + formatted_version, + len(crate_info.srcs.to_list()), + ), + ) else: # Run without process_wrapper - if build_env_files or build_flags_files or stamp: - fail("build_env_files, build_flags_files, stamp are not supported when building without process_wrapper") + if build_env_files or build_flags_files or stamp or build_metadata: + fail("build_env_files, build_flags_files, stamp, build_metadata are not supported when building without process_wrapper") ctx.actions.run( executable = toolchain.rustc, inputs = compile_inputs, @@ -1304,7 +1448,7 @@ def _get_dir_names(files): dirs[f.dirname] = None return dirs.keys() -def add_crate_link_flags(args, dep_info, force_all_deps_direct = False): +def add_crate_link_flags(args, dep_info, force_all_deps_direct = False, link_to_metadata = False): """Adds link flags to an Args object reference Args: @@ -1312,9 +1456,26 @@ def add_crate_link_flags(args, dep_info, force_all_deps_direct = False): dep_info (DepInfo): The current target's dependency info force_all_deps_direct (bool, optional): Whether to pass the transitive rlibs with --extern to the commandline as opposed to -L. + link_to_metadata: """ + # print(" link_to_metadata ", link_to_metadata) - if force_all_deps_direct: + if not link_to_metadata: + if force_all_deps_direct: + args.add_all( + depset( + transitive = [ + dep_info.direct_crates, + dep_info.transitive_crates, + ], + ), + uniquify = True, + map_each = _crate_to_link_flag, + ) + else: + # nb. Direct crates are linked via --extern regardless of their crate_type + args.add_all(dep_info.direct_crates, map_each = _crate_to_link_flag) + elif force_all_deps_direct: args.add_all( depset( transitive = [ @@ -1323,11 +1484,12 @@ def add_crate_link_flags(args, dep_info, force_all_deps_direct = False): ], ), uniquify = True, - map_each = _crate_to_link_flag, + map_each = _crate_to_link_flag_metadata, ) else: # nb. Direct crates are linked via --extern regardless of their crate_type - args.add_all(dep_info.direct_crates, map_each = _crate_to_link_flag) + args.add_all(dep_info.direct_crates, map_each = _crate_to_link_flag_metadata) + args.add_all( dep_info.transitive_crates, map_each = _get_crate_dirname, @@ -1335,6 +1497,29 @@ def add_crate_link_flags(args, dep_info, force_all_deps_direct = False): format_each = "-Ldependency=%s", ) +def _crate_to_link_flag_metadata(crate): + """A helper macro used by `add_crate_link_flags` for adding crate link flags to a Arg object + + Args: + crate (CrateInfo|AliasableDepInfo): A CrateInfo or an AliasableDepInfo provider + + Returns: + list: Link flags for the given provider + """ + + # This is AliasableDepInfo, we should use the alias as a crate name + if hasattr(crate, "dep"): + name = crate.name + crate_info = crate.dep + else: + name = crate.name + crate_info = crate + + lib_or_meta = crate_info.metadata + if not crate_info.metadata: + lib_or_meta = crate_info.output + return ["--extern={}={}".format(name, lib_or_meta.path)] + def _crate_to_link_flag(crate): """A helper macro used by `add_crate_link_flags` for adding crate link flags to a Arg object diff --git a/rust/private/rustdoc.bzl b/rust/private/rustdoc.bzl index 82fdd4e069..6314da22bf 100644 --- a/rust/private/rustdoc.bzl +++ b/rust/private/rustdoc.bzl @@ -37,6 +37,7 @@ def _strip_crate_info_output(crate_info): aliases = crate_info.aliases, # This crate info should have no output output = None, + metadata = crate_info.metadata, edition = crate_info.edition, rustc_env = crate_info.rustc_env, is_test = crate_info.is_test, @@ -90,6 +91,8 @@ def rustdoc_compile_action( crate_info = crate_info, dep_info = dep_info, build_info = build_info, + # If this is a rustdoc test, we need to depend on rlibs rather than .rmeta. + force_depend_on_objects = is_test, ) # Since this crate is not actually producing the output described by the @@ -118,6 +121,7 @@ def rustdoc_compile_action( emit = [], remap_path_prefix = None, force_link = True, + force_link_to_objects = is_test, ) # Because rustdoc tests compile tests outside of the sandbox, the sysroot diff --git a/rust/private/utils.bzl b/rust/private/utils.bzl index 4ae5b5fc3e..633c90a00f 100644 --- a/rust/private/utils.bzl +++ b/rust/private/utils.bzl @@ -610,3 +610,25 @@ def _replace_all(string, substitutions): string = string[:pattern_start] + replacement + string[after_pattern:] return string + +def can_build_metadata(toolchain, ctx, crate_type): + """Can we build metadata for this rust_library? + + Args: + toolchain (toolchain): The rust toolchain + ctx (ctx): The rule's context object + crate_type (String): one of lib|rlib|dylib|staticlib|cdylib|proc-macro + + Returns: + bool: whether we can build metadata for this rule. + """ + + # In order to enable pipelined compilation we require that: + # 1) The _pipelined_compilation flag is enabled, + # 2) the OS running the rule is something other than windows as we require sandboxing (for now), + # 3) process_wrapper is enabled (this is disabled when compiling process_wrapper itself), + # 4) the crate_type is rlib or lib. + return toolchain._pipelined_compilation and \ + toolchain.os != "windows" and \ + ctx.attr._process_wrapper and \ + crate_type in ("rlib", "lib") diff --git a/rust/settings/BUILD.bazel b/rust/settings/BUILD.bazel index 2ff9a439fc..c5928a15d5 100644 --- a/rust/settings/BUILD.bazel +++ b/rust/settings/BUILD.bazel @@ -29,6 +29,14 @@ bool_flag( build_setting_default = False, ) +# When set, this flag causes rustc to emit .rmeta files and use them for rlib -> rlib dependencies. +# While this involves one extra (short) rustc invocation to build the rmeta file, +# it allows library dependencies to be unlocked much sooner, increasing parallelism during compilation. +bool_flag( + name = "pipelined_compilation", + build_setting_default = False, +) + bzl_library( name = "bzl_lib", srcs = glob(["**/*.bzl"]), diff --git a/rust/toolchain.bzl b/rust/toolchain.bzl index a046c4e58f..90a3c593c2 100644 --- a/rust/toolchain.bzl +++ b/rust/toolchain.bzl @@ -426,6 +426,7 @@ def _rust_toolchain_impl(ctx): rename_first_party_crates = ctx.attr._rename_first_party_crates[BuildSettingInfo].value third_party_dir = ctx.attr._third_party_dir[BuildSettingInfo].value + pipelined_compilation = ctx.attr._pipelined_compilation[BuildSettingInfo].value if ctx.attr.rust_lib: # buildifier: disable=print @@ -536,6 +537,7 @@ def _rust_toolchain_impl(ctx): # Experimental and incompatible flags _rename_first_party_crates = rename_first_party_crates, _third_party_dir = third_party_dir, + _pipelined_compilation = pipelined_compilation, ) return [ toolchain, @@ -673,6 +675,9 @@ rust_toolchain = rule( "_cc_toolchain": attr.label( default = Label("@bazel_tools//tools/cpp:current_cc_toolchain"), ), + "_pipelined_compilation": attr.label( + default = "@rules_rust//rust/settings:pipelined_compilation", + ), "_rename_first_party_crates": attr.label( default = Label("//rust/settings:rename_first_party_crates"), ), diff --git a/test/process_wrapper/rustc_quit_on_rmeta.rs b/test/process_wrapper/rustc_quit_on_rmeta.rs index df32341dc4..55595084e1 100644 --- a/test/process_wrapper/rustc_quit_on_rmeta.rs +++ b/test/process_wrapper/rustc_quit_on_rmeta.rs @@ -6,8 +6,8 @@ mod test { use runfiles::Runfiles; - // fake_rustc runs the fake_rustc binary under process_wrapper with the specified - // process wrapper arguments. No arguments are passed to fake_rustc itself. + /// fake_rustc runs the fake_rustc binary under process_wrapper with the specified + /// process wrapper arguments. No arguments are passed to fake_rustc itself. fn fake_rustc(process_wrapper_args: &[&'static str]) -> String { let r = Runfiles::create().unwrap(); let fake_rustc = r.rlocation( @@ -59,7 +59,12 @@ mod test { #[test] fn test_rustc_quit_on_rmeta_quits() { - let out_content = fake_rustc(&["--rustc-quit-on-rmeta", "true"]); + let out_content = fake_rustc(&[ + "--rustc-quit-on-rmeta", + "true", + "--rustc-output-format", + "rendered", + ]); assert!( !out_content.contains("should not be in output"), "output should not contain 'should not be in output' but did: {}", diff --git a/test/unit/force_all_deps_direct/generator.bzl b/test/unit/force_all_deps_direct/generator.bzl index 2ca319587d..370606b511 100644 --- a/test/unit/force_all_deps_direct/generator.bzl +++ b/test/unit/force_all_deps_direct/generator.bzl @@ -9,6 +9,9 @@ load("//rust/private:providers.bzl", "BuildInfo", "CrateInfo", "DepInfo", "DepVa # buildifier: disable=bzl-visibility load("//rust/private:rustc.bzl", "rustc_compile_action") +# buildifier: disable=bzl-visibility +load("//rust/private:utils.bzl", "can_build_metadata") + def _generator_impl(ctx): rs_file = ctx.actions.declare_file(ctx.label.name + "_generated.rs") ctx.actions.run_shell( @@ -39,6 +42,12 @@ EOF lib_hash = output_hash, extension = ".rlib", ) + rust_metadata_name = "{prefix}{name}-{lib_hash}{extension}".format( + prefix = "lib", + name = crate_name, + lib_hash = output_hash, + extension = ".rmeta", + ) deps = [DepVariantInfo( crate_info = dep[CrateInfo] if CrateInfo in dep else None, @@ -48,6 +57,9 @@ EOF ) for dep in ctx.attr.deps] rust_lib = ctx.actions.declare_file(rust_lib_name) + rust_metadata = None + if can_build_metadata(toolchain, ctx, crate_type): + rust_metadata = ctx.actions.declare_file(rust_metadata_name) return rustc_compile_action( ctx = ctx, attr = ctx.attr, @@ -61,6 +73,7 @@ EOF proc_macro_deps = depset([]), aliases = {}, output = rust_lib, + metadata = rust_metadata, owner = ctx.label, edition = "2018", compile_data = depset([]), diff --git a/test/unit/pipelined_compilation/BUILD.bazel b/test/unit/pipelined_compilation/BUILD.bazel new file mode 100644 index 0000000000..8d363e03ed --- /dev/null +++ b/test/unit/pipelined_compilation/BUILD.bazel @@ -0,0 +1,4 @@ +load(":pipelined_compilation_test.bzl", "pipelined_compilation_test_suite") + +############################ UNIT TESTS ############################# +pipelined_compilation_test_suite(name = "pipelined_compilation_test_suite") diff --git a/test/unit/pipelined_compilation/bin.rs b/test/unit/pipelined_compilation/bin.rs new file mode 100644 index 0000000000..aa32dd243d --- /dev/null +++ b/test/unit/pipelined_compilation/bin.rs @@ -0,0 +1,5 @@ +use second::fun; + +fn main() { + fun() +} diff --git a/test/unit/pipelined_compilation/first.rs b/test/unit/pipelined_compilation/first.rs new file mode 100644 index 0000000000..30c0129d32 --- /dev/null +++ b/test/unit/pipelined_compilation/first.rs @@ -0,0 +1,4 @@ +pub fn first_fun() -> u8 { + 4 // chosen by fair dice roll. + // guaranteed to be random. +} diff --git a/test/unit/pipelined_compilation/my_macro.rs b/test/unit/pipelined_compilation/my_macro.rs new file mode 100644 index 0000000000..035c76101a --- /dev/null +++ b/test/unit/pipelined_compilation/my_macro.rs @@ -0,0 +1,6 @@ +use proc_macro::TokenStream; + +#[proc_macro_attribute] +pub fn noop(_attr: TokenStream, item: TokenStream) -> TokenStream { + item +} diff --git a/test/unit/pipelined_compilation/pipelined_compilation_test.bzl b/test/unit/pipelined_compilation/pipelined_compilation_test.bzl new file mode 100644 index 0000000000..1ccb3c7be5 --- /dev/null +++ b/test/unit/pipelined_compilation/pipelined_compilation_test.bzl @@ -0,0 +1,125 @@ +"""Unittests for rust rules.""" + +load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") +load("//rust:defs.bzl", "rust_binary", "rust_library", "rust_proc_macro") +load("//test/unit:common.bzl", "assert_argv_contains", "assert_list_contains_adjacent_elements", "assert_list_contains_adjacent_elements_not") + +def _second_lib_test_impl(ctx): + env = analysistest.begin(ctx) + tut = analysistest.target_under_test(env) + rlib_action = [act for act in tut.actions if act.mnemonic == "Rustc"][0] + metadata_action = [act for act in tut.actions if act.mnemonic == "RustcMetadata"][0] + + # Both actions should use the same --emit= + assert_argv_contains(env, rlib_action, "--emit=dep-info,link,metadata") + assert_argv_contains(env, metadata_action, "--emit=dep-info,link,metadata") + + # The metadata action should have a .rmeta as output and the rlib action a .rlib + path = rlib_action.outputs.to_list()[0].path + asserts.true( + env, + path.endswith(".rlib"), + "expected Rustc to output .rlib, got " + path, + ) + path = metadata_action.outputs.to_list()[0].path + asserts.true( + env, + path.endswith(".rmeta"), + "expected RustcMetadata to output .rmeta, got " + path, + ) + + # Only the action building metadata should contain --rustc-quit-on-rmeta + assert_list_contains_adjacent_elements_not(env, rlib_action.argv, ["--rustc-quit-on-rmeta", "true"]) + assert_list_contains_adjacent_elements(env, metadata_action.argv, ["--rustc-quit-on-rmeta", "true"]) + + # Check that both actions refer to the metadata of :first, not the rlib + extern_metadata = [arg for arg in metadata_action.argv if arg.startswith("--extern=first=") and "libfirst" in arg and arg.endswith(".rmeta")] + asserts.true( + env, + len(extern_metadata) == 1, + "did not find a --extern=first=*.rmeta but expected one", + ) + extern_rlib = [arg for arg in rlib_action.argv if arg.startswith("--extern=first=") and "libfirst" in arg and arg.endswith(".rmeta")] + asserts.true( + env, + len(extern_rlib) == 1, + "did not find a --extern=first=*.rlib but expected one", + ) + + # Check that the input to both actions is the metadata of :first + input_metadata = [i for i in metadata_action.inputs.to_list() if i.basename.startswith("libfirst")] + asserts.true(env, len(input_metadata) == 1, "expected only one libfirst input, found " + str([i.path for i in input_metadata])) + asserts.true(env, input_metadata[0].extension == "rmeta", "expected libfirst dependency to be rmeta, found " + input_metadata[0].path) + input_rlib = [i for i in rlib_action.inputs.to_list() if i.basename.startswith("libfirst")] + asserts.true(env, len(input_rlib) == 1, "expected only one libfirst input, found " + str([i.path for i in input_rlib])) + asserts.true(env, input_rlib[0].extension == "rmeta", "expected libfirst dependency to be rmeta, found " + input_rlib[0].path) + + return analysistest.end(env) + +def _bin_test_impl(ctx): + env = analysistest.begin(ctx) + tut = analysistest.target_under_test(env) + bin_action = [act for act in tut.actions if act.mnemonic == "Rustc"][0] + + # Check that no inputs to this binary are .rmeta files. + metadata_inputs = [i.path for i in bin_action.inputs.to_list() if i.path.endswith(".rmeta")] + asserts.false(env, metadata_inputs, "expected no metadata inputs, found " + str(metadata_inputs)) + + return analysistest.end(env) + +ENABLE_PIPELINING = { + "@//rust/settings:pipelined_compilation": True, +} +bin_test = analysistest.make(_bin_test_impl, config_settings = ENABLE_PIPELINING) +second_lib_test = analysistest.make(_second_lib_test_impl, config_settings = ENABLE_PIPELINING) + +def _pipelined_compilation_test(): + rust_proc_macro( + name = "my_macro", + edition = "2021", + srcs = ["my_macro.rs"], + ) + + rust_library( + name = "first", + edition = "2021", + srcs = ["first.rs"], + ) + + rust_library( + name = "second", + edition = "2021", + srcs = ["second.rs"], + deps = [":first"], + proc_macro_deps = [":my_macro"], + ) + + rust_binary( + name = "bin", + edition = "2021", + srcs = ["bin.rs"], + deps = [":second"], + ) + + NOT_WINDOWS = [ + "@platforms//os:linux", + "@platforms//os:macos", + ] + 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 pipelined_compilation_test_suite(name): + """Entry-point macro called from the BUILD file. + + Args: + name: Name of the macro. + """ + _pipelined_compilation_test() + + native.test_suite( + name = name, + tests = [ + ":bin_test", + ":second_lib_test", + ], + ) diff --git a/test/unit/pipelined_compilation/second.rs b/test/unit/pipelined_compilation/second.rs new file mode 100644 index 0000000000..b42e0b47a8 --- /dev/null +++ b/test/unit/pipelined_compilation/second.rs @@ -0,0 +1,7 @@ +use first::first_fun; +use my_macro::noop; + +#[noop] +pub fn fun() { + println!("{}", first_fun()) +} diff --git a/util/process_wrapper/main.rs b/util/process_wrapper/main.rs index a90bf4c3b8..0499ca9cfa 100644 --- a/util/process_wrapper/main.rs +++ b/util/process_wrapper/main.rs @@ -40,6 +40,7 @@ fn status_code(status: ExitStatus, was_killed: bool) -> i32 { fn status_code(status: ExitStatus, was_killed: bool) -> i32 { // On unix, if code is None it means that the process was killed by a signal. // https://doc.rust-lang.org/std/process/struct.ExitStatus.html#method.success + // eprintln!("{:?} {}", status, was_killed); match status.code() { Some(code) => code, // If we killed the process, we expect None here @@ -55,19 +56,6 @@ fn main() { Ok(v) => v, }; - let stderr: Box = if let Some(stderr_file) = opts.stderr_file { - Box::new( - OpenOptions::new() - .create(true) - .truncate(true) - .write(true) - .open(stderr_file) - .expect("process wrapper error: unable to open stderr file"), - ) - } else { - Box::new(io::stderr()) - }; - let mut child = Command::new(opts.executable) .args(opts.child_arguments) .env_clear() @@ -87,25 +75,44 @@ fn main() { .spawn() .expect("process wrapper error: failed to spawn child process"); - let child_stderr = Box::new(child.stderr.take().unwrap()); + let mut stderr: Box = if let Some(stderr_file) = opts.stderr_file { + Box::new( + OpenOptions::new() + .create(true) + .truncate(true) + .write(true) + .open(stderr_file) + .expect("process wrapper error: unable to open stderr file"), + ) + } else { + Box::new(io::stderr()) + }; + + let mut child_stderr = child.stderr.take().unwrap(); let mut was_killed = false; - let result = if !opts.rustc_quit_on_rmeta { - // Process output normally by forwarding stderr - process_output(child_stderr, stderr, LineOutput::Message) - } else { - let format = opts.rustc_output_format; - let mut kill = false; - let result = process_output(child_stderr, stderr, |line| { - rustc::stop_on_rmeta_completion(line, format, &mut kill) + let result = if let Some(format) = opts.rustc_output_format { + let quit_on_rmeta = opts.rustc_quit_on_rmeta; + // Process json rustc output and kill the subprocess when we get a signal + // that we emitted a metadata file. + let mut metadata_emitted = false; + let result = process_output(&mut child_stderr, stderr.as_mut(), move |line| { + if quit_on_rmeta { + rustc::stop_on_rmeta_completion(line, format, &mut metadata_emitted) + } else { + rustc::process_json(line, format) + } }); - if kill { + if metadata_emitted { // If recv returns Ok(), a signal was sent in this channel so we should terminate the child process. // We can safely ignore the Result from kill() as we don't care if the process already terminated. let _ = child.kill(); was_killed = true; } result + } else { + // Process output normally by forwarding stderr + process_output(&mut child_stderr, stderr.as_mut(), LineOutput::Message) }; result.expect("process wrapper error: failed to process stderr"); diff --git a/util/process_wrapper/options.rs b/util/process_wrapper/options.rs index fdd60b4acc..869b5c32b5 100644 --- a/util/process_wrapper/options.rs +++ b/util/process_wrapper/options.rs @@ -44,7 +44,7 @@ pub(crate) struct Options { pub(crate) rustc_quit_on_rmeta: bool, // If rustc_quit_on_rmeta is set to true, this controls the // output format of rustc messages. - pub(crate) rustc_output_format: rustc::ErrorFormat, + pub(crate) rustc_output_format: Option, } pub(crate) fn options() -> Result { @@ -173,8 +173,7 @@ pub(crate) fn options() -> Result { v ))), }) - .transpose()? - .unwrap_or_default(); + .transpose()?; // Prepare the environment variables, unifying those read from files with the ones // of the current process. diff --git a/util/process_wrapper/output.rs b/util/process_wrapper/output.rs index 049090c7fa..84d61d9d75 100644 --- a/util/process_wrapper/output.rs +++ b/util/process_wrapper/output.rs @@ -31,8 +31,8 @@ pub(crate) enum LineOutput { /// Depending on the result of process_line, the modified message may be written /// to write_end. pub(crate) fn process_output( - read_end: Box, - write_end: Box, + read_end: &mut dyn Read, + write_end: &mut dyn Write, mut process_line: F, ) -> io::Result<()> where diff --git a/util/process_wrapper/rustc.rs b/util/process_wrapper/rustc.rs index e5667279b1..ca796806cc 100644 --- a/util/process_wrapper/rustc.rs +++ b/util/process_wrapper/rustc.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::convert::{TryFrom, TryInto}; + use tinyjson::JsonValue; use crate::output::LineOutput; @@ -40,9 +42,46 @@ fn get_key(value: &JsonValue, key: &str) -> Option { } } -/// stop_on_rmeta_completion takes an output line from rustc configured with +#[derive(Debug)] +enum RustcMessage { + Emit(String), + Message(String), +} + +impl TryFrom for RustcMessage { + type Error = (); + fn try_from(val: JsonValue) -> Result { + if let Some(emit) = get_key(&val, "emit") { + return Ok(Self::Emit(emit)); + } + if let Some(rendered) = get_key(&val, "rendered") { + return Ok(Self::Message(rendered)); + } + Err(()) + } +} + +/// process_rustc_json takes an output line from rustc configured with /// --error-format=json, parses the json and returns the appropriate output -/// according to the original --error-format supplied to rustc. +/// according to the original --error-format supplied. +/// Only messages are returned, emits are ignored. +pub(crate) fn process_json(line: String, error_format: ErrorFormat) -> LineOutput { + let parsed: JsonValue = line + .parse() + .expect("process wrapper error: expected json messages in pipeline mode"); + match parsed.try_into() { + Ok(RustcMessage::Message(msg)) => match error_format { + // If the output should be json, we just forward the messages as-is + // using `line`. + ErrorFormat::Json => LineOutput::Message(line), + // Otherwise we return the rendered field. + _ => LineOutput::Message(msg), + }, + _ => LineOutput::Skip, + } +} + +/// stop_on_rmeta_completion parses the json output of rustc in the same way process_rustc_json does. /// In addition, it will signal to stop when metadata is emitted /// so the compiler can be terminated. /// This is used to implement pipelining in rules_rust, please see @@ -55,24 +94,19 @@ pub(crate) fn stop_on_rmeta_completion( let parsed: JsonValue = line .parse() .expect("process wrapper error: expected json messages in pipeline mode"); - if let Some(emit) = get_key(&parsed, "emit") { - // We don't want to print emit messages. - // If the emit messages is "metadata" we can signal the process to quit - return if emit == "metadata" { + + match parsed.try_into() { + Ok(RustcMessage::Emit(emit)) if emit == "metadata" => { *kill = true; LineOutput::Terminate - } else { - LineOutput::Skip - }; - }; - - match error_format { - // If the output should be json, we just forward the messages as-is - ErrorFormat::Json => LineOutput::Message(line), - // Otherwise we extract the "rendered" attribute. - // If we don't find it we skip the line. - _ => get_key(&parsed, "rendered") - .map(LineOutput::Message) - .unwrap_or(LineOutput::Skip), + } + Ok(RustcMessage::Message(msg)) => match error_format { + // If the output should be json, we just forward the messages as-is + // using `line`. + ErrorFormat::Json => LineOutput::Message(line), + // Otherwise we return the rendered field. + _ => LineOutput::Message(msg), + }, + _ => LineOutput::Skip, } } From bb7e9b03bd5b3f2b986b912f709d65f6c282e153 Mon Sep 17 00:00:00 2001 From: Roberto Bampi Date: Wed, 27 Jul 2022 17:24:08 +0200 Subject: [PATCH 2/7] Addressing review comments (1/2) --- rust/private/providers.bzl | 2 +- rust/private/rustc.bzl | 59 ++++++------------- rust/private/rustdoc.bzl | 2 +- .../pipelined_compilation_test.bzl | 9 +-- util/process_wrapper/main.rs | 1 - 5 files changed, 24 insertions(+), 49 deletions(-) diff --git a/rust/private/providers.bzl b/rust/private/providers.bzl index 7e28244e83..9600718b2a 100644 --- a/rust/private/providers.bzl +++ b/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 crate metadata outputs.", + "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_noncrates": "depset[LinkerInput]: All transitive dependencies that aren't crates.", }, ) diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index 49622ee834..58ff91264d 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -237,20 +237,16 @@ def collect_deps( if not crate_info.metadata: depend_on = crate_info.output + # If this dependency is a proc_macro, it still can be used for lib crates + # that produce metadata. + # In that case, we don't depend on its metadata dependencies. transitive_metadata_outputs.append( depset( [depend_on], - transitive = [dep_info.transitive_metadata_outputs], + transitive = [] if _is_proc_macro(crate_info) else [dep_info.transitive_metadata_outputs], ), ) - # If this dependency is a proc_macro, it still can be used for lib crates - # that produce metadata. - if _is_proc_macro(crate_info): - transitive_metadata_outputs.append( - depset([crate_info.output]), - ) - transitive_crate_outputs.append( depset( [crate_info.output], @@ -922,10 +918,10 @@ def construct_arguments( _add_native_link_flags(rustc_flags, dep_info, linkstamp_outs, ambiguous_libs, crate_info.type, toolchain, cc_toolchain, feature_configuration) - link_to_metadata = _depend_on_metadata(crate_info, force_link_to_objects) + use_metadata = _depend_on_metadata(crate_info, force_link_to_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, link_to_metadata = link_to_metadata) + add_crate_link_flags(rustc_flags, dep_info, force_all_deps_direct, use_metadata) needs_extern_proc_macro_flag = _is_proc_macro(crate_info) and crate_info.edition != "2015" if needs_extern_proc_macro_flag: @@ -1448,7 +1444,7 @@ def _get_dir_names(files): dirs[f.dirname] = None return dirs.keys() -def add_crate_link_flags(args, dep_info, force_all_deps_direct = False, link_to_metadata = False): +def add_crate_link_flags(args, dep_info, force_all_deps_direct = False, use_metadata = False): """Adds link flags to an Args object reference Args: @@ -1456,39 +1452,18 @@ def add_crate_link_flags(args, dep_info, force_all_deps_direct = False, link_to_ dep_info (DepInfo): The current target's dependency info force_all_deps_direct (bool, optional): Whether to pass the transitive rlibs with --extern to the commandline as opposed to -L. - link_to_metadata: + use_metadata (bool, optional): Build command line arugments using metadata for crates that provide it. """ - # print(" link_to_metadata ", link_to_metadata) - if not link_to_metadata: - if force_all_deps_direct: - args.add_all( - depset( - transitive = [ - dep_info.direct_crates, - dep_info.transitive_crates, - ], - ), - uniquify = True, - map_each = _crate_to_link_flag, - ) - else: - # nb. Direct crates are linked via --extern regardless of their crate_type - args.add_all(dep_info.direct_crates, map_each = _crate_to_link_flag) - elif force_all_deps_direct: - args.add_all( - depset( - transitive = [ - dep_info.direct_crates, - dep_info.transitive_crates, - ], - ), - uniquify = True, - map_each = _crate_to_link_flag_metadata, - ) - else: - # nb. Direct crates are linked via --extern regardless of their crate_type - args.add_all(dep_info.direct_crates, map_each = _crate_to_link_flag_metadata) + direct_crates = depset( + transitive = [ + dep_info.direct_crates, + dep_info.transitive_crates, + ], + ) if force_all_deps_direct else dep_info.direct_crates + + crate_to_link_flags = _crate_to_link_flag_metadata if use_metadata else _crate_to_link_flag + args.add_all(direct_crates, uniquify = True, map_each = crate_to_link_flags) args.add_all( dep_info.transitive_crates, diff --git a/rust/private/rustdoc.bzl b/rust/private/rustdoc.bzl index 6314da22bf..2660794d96 100644 --- a/rust/private/rustdoc.bzl +++ b/rust/private/rustdoc.bzl @@ -37,7 +37,7 @@ def _strip_crate_info_output(crate_info): aliases = crate_info.aliases, # This crate info should have no output output = None, - metadata = crate_info.metadata, + metadata = None, edition = crate_info.edition, rustc_env = crate_info.rustc_env, is_test = crate_info.is_test, diff --git a/test/unit/pipelined_compilation/pipelined_compilation_test.bzl b/test/unit/pipelined_compilation/pipelined_compilation_test.bzl index 1ccb3c7be5..403271e1d3 100644 --- a/test/unit/pipelined_compilation/pipelined_compilation_test.bzl +++ b/test/unit/pipelined_compilation/pipelined_compilation_test.bzl @@ -101,10 +101,11 @@ def _pipelined_compilation_test(): deps = [":second"], ) - NOT_WINDOWS = [ - "@platforms//os:linux", - "@platforms//os:macos", - ] + NOT_WINDOWS = select({ + "@platforms//os:linux": [], + "@platforms//os:macos": [], + "//conditions:default": ["@platforms//:incompatible"], + }) 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) diff --git a/util/process_wrapper/main.rs b/util/process_wrapper/main.rs index 0499ca9cfa..49c7e24ff1 100644 --- a/util/process_wrapper/main.rs +++ b/util/process_wrapper/main.rs @@ -40,7 +40,6 @@ fn status_code(status: ExitStatus, was_killed: bool) -> i32 { fn status_code(status: ExitStatus, was_killed: bool) -> i32 { // On unix, if code is None it means that the process was killed by a signal. // https://doc.rust-lang.org/std/process/struct.ExitStatus.html#method.success - // eprintln!("{:?} {}", status, was_killed); match status.code() { Some(code) => code, // If we killed the process, we expect None here From 1fb9a132c9f374609c37475339bc33066eec4ebb Mon Sep 17 00:00:00 2001 From: Roberto Bampi Date: Thu, 28 Jul 2022 14:13:40 +0200 Subject: [PATCH 3/7] Implement leak test for libs --- test/unit/proc_macro/leaks_deps/lib/a.rs | 5 ++ test/unit/proc_macro/leaks_deps/lib/b.rs | 3 + .../proc_macro/leaks_deps/lib/my_macro.rs | 7 ++ .../proc_macro_does_not_leak_deps.bzl | 70 ++++++++++++++++++- 4 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 test/unit/proc_macro/leaks_deps/lib/a.rs create mode 100644 test/unit/proc_macro/leaks_deps/lib/b.rs create mode 100644 test/unit/proc_macro/leaks_deps/lib/my_macro.rs diff --git a/test/unit/proc_macro/leaks_deps/lib/a.rs b/test/unit/proc_macro/leaks_deps/lib/a.rs new file mode 100644 index 0000000000..7d1a54eb75 --- /dev/null +++ b/test/unit/proc_macro/leaks_deps/lib/a.rs @@ -0,0 +1,5 @@ +use my_macro::greet; + +pub fn use_macro() -> &'static str { + greet!() +} diff --git a/test/unit/proc_macro/leaks_deps/lib/b.rs b/test/unit/proc_macro/leaks_deps/lib/b.rs new file mode 100644 index 0000000000..8fd0518ede --- /dev/null +++ b/test/unit/proc_macro/leaks_deps/lib/b.rs @@ -0,0 +1,3 @@ +pub fn hello() -> &'static str { + "hello" +} diff --git a/test/unit/proc_macro/leaks_deps/lib/my_macro.rs b/test/unit/proc_macro/leaks_deps/lib/my_macro.rs new file mode 100644 index 0000000000..f8b6f2a6be --- /dev/null +++ b/test/unit/proc_macro/leaks_deps/lib/my_macro.rs @@ -0,0 +1,7 @@ +use b::hello; +use proc_macro::{Literal, TokenStream, TokenTree}; + +#[proc_macro] +pub fn greet(_item: TokenStream) -> TokenStream { + TokenTree::Literal(Literal::string(hello())).into() +} diff --git a/test/unit/proc_macro/leaks_deps/proc_macro_does_not_leak_deps.bzl b/test/unit/proc_macro/leaks_deps/proc_macro_does_not_leak_deps.bzl index 40c2e485cc..db86506305 100644 --- a/test/unit/proc_macro/leaks_deps/proc_macro_does_not_leak_deps.bzl +++ b/test/unit/proc_macro/leaks_deps/proc_macro_does_not_leak_deps.bzl @@ -1,7 +1,7 @@ """Unittest to verify proc-macro targets""" load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") -load("//rust:defs.bzl", "rust_proc_macro", "rust_test") +load("//rust:defs.bzl", "rust_library", "rust_proc_macro", "rust_test") def _proc_macro_does_not_leak_deps_impl(ctx): env = analysistest.begin(ctx) @@ -30,8 +30,6 @@ def _proc_macro_does_not_leak_deps_impl(ctx): return analysistest.end(env) -proc_macro_does_not_leak_deps_test = analysistest.make(_proc_macro_does_not_leak_deps_impl) - def _proc_macro_does_not_leak_deps_test(): rust_proc_macro( name = "proc_macro_definition", @@ -68,6 +66,70 @@ def _proc_macro_does_not_leak_deps_test(): target_under_test = ":deps_not_leaked", ) +proc_macro_does_not_leak_deps_test = analysistest.make(_proc_macro_does_not_leak_deps_impl) + +# Tests that a lib_a -> proc_macro -> lib_b does not propagate lib_b to the inputs of lib_a +def _proc_macro_does_not_leak_lib_deps_impl(ctx): + env = analysistest.begin(ctx) + actions = analysistest.target_under_test(env).actions + rustc_actions = [] + for action in actions: + if action.mnemonic == "Rustc" or action.mnemonic == "RustcMetadata": + rustc_actions.append(action) + + # We should have a RustcMetadata and a Rustc action. + asserts.true(env, len(rustc_actions) == 2, "expected 2 actions, got %d" % len(rustc_actions)) + + for rustc_action in rustc_actions: + # lib :a has a dependency on :my_macro via a rust_proc_macro target. + # lib :b (which is a dependency of :my_macro) should not appear in the inputs of :a + b_inputs = [i for i in rustc_action.inputs.to_list() if "libb" in i.path] + b_args = [arg for arg in rustc_action.argv if "libb" in arg] + + asserts.equals(env, 0, len(b_inputs)) + asserts.equals(env, 0, len(b_args)) + + return analysistest.end(env) + +def _proc_macro_does_not_leak_lib_deps_test(): + rust_library( + name = "b", + srcs = ["leaks_deps/lib/b.rs"], + edition = "2018", + ) + + rust_proc_macro( + name = "my_macro", + srcs = ["leaks_deps/lib/my_macro.rs"], + edition = "2018", + deps = [ + ":b", + ], + ) + + rust_library( + name = "a", + srcs = ["leaks_deps/lib/a.rs"], + edition = "2018", + proc_macro_deps = [ + ":my_macro", + ], + ) + + NOT_WINDOWS = select({ + "@platforms//os:linux": [], + "@platforms//os:macos": [], + "//conditions:default": ["@platforms//:incompatible"], + }) + + proc_macro_does_not_leak_lib_deps_test( + name = "proc_macro_does_not_leak_lib_deps_test", + target_under_test = ":a", + target_compatible_with = NOT_WINDOWS, + ) + +proc_macro_does_not_leak_lib_deps_test = analysistest.make(_proc_macro_does_not_leak_lib_deps_impl, config_settings = {"@//rust/settings:pipelined_compilation": True}) + def proc_macro_does_not_leak_deps_test_suite(name): """Entry-point macro called from the BUILD file. @@ -75,10 +137,12 @@ def proc_macro_does_not_leak_deps_test_suite(name): name: Name of the macro. """ _proc_macro_does_not_leak_deps_test() + _proc_macro_does_not_leak_lib_deps_test() native.test_suite( name = name, tests = [ ":proc_macro_does_not_leak_deps_test", + ":proc_macro_does_not_leak_lib_deps_test", ], ) From 886dc32cea178c834e93b7e0237144b83fa5e697 Mon Sep 17 00:00:00 2001 From: Roberto Bampi Date: Thu, 28 Jul 2022 14:42:41 +0200 Subject: [PATCH 4/7] Regenerate documentation --- docs/flatten.md | 2 +- docs/providers.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/flatten.md b/docs/flatten.md index d70d2e2ddf..20eb643fe6 100644 --- a/docs/flatten.md +++ b/docs/flatten.md @@ -1401,7 +1401,7 @@ A provider containing information about a Crate's dependencies. | 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 crate metadata outputs. | +| 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_noncrates | depset[LinkerInput]: All transitive dependencies that aren't crates. | diff --git a/docs/providers.md b/docs/providers.md index fa5a1cef5d..93fcda7d2d 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -61,7 +61,7 @@ A provider containing information about a Crate's dependencies. | 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 crate metadata outputs. | +| 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_noncrates | depset[LinkerInput]: All transitive dependencies that aren't crates. | From 4e01fa0ff880e5e198e398ff5520634e46422c7b Mon Sep 17 00:00:00 2001 From: Roberto Bampi Date: Tue, 2 Aug 2022 19:36:15 +0200 Subject: [PATCH 5/7] process_wrapper: properly kill the process without waiting for it to exit --- util/process_wrapper/main.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/util/process_wrapper/main.rs b/util/process_wrapper/main.rs index 49c7e24ff1..6d985b34af 100644 --- a/util/process_wrapper/main.rs +++ b/util/process_wrapper/main.rs @@ -94,15 +94,16 @@ fn main() { let quit_on_rmeta = opts.rustc_quit_on_rmeta; // Process json rustc output and kill the subprocess when we get a signal // that we emitted a metadata file. - let mut metadata_emitted = false; + let mut me = false; + let metadata_emitted = &mut me; let result = process_output(&mut child_stderr, stderr.as_mut(), move |line| { if quit_on_rmeta { - rustc::stop_on_rmeta_completion(line, format, &mut metadata_emitted) + rustc::stop_on_rmeta_completion(line, format, metadata_emitted) } else { rustc::process_json(line, format) } }); - if metadata_emitted { + if me { // If recv returns Ok(), a signal was sent in this channel so we should terminate the child process. // We can safely ignore the Result from kill() as we don't care if the process already terminated. let _ = child.kill(); From d32726b900efad48ca6cb63ae5e4bb9b4d027419 Mon Sep 17 00:00:00 2001 From: Roberto Bampi Date: Wed, 3 Aug 2022 10:08:02 +0200 Subject: [PATCH 6/7] Added tests for pipelining with custom rules --- test/unit/force_all_deps_direct/generator.bzl | 13 --- .../custom_rule_test/to_wrap.rs | 3 + .../custom_rule_test/uses_wrapper.rs | 5 + .../pipelined_compilation_test.bzl | 85 ++++++++++++-- test/unit/pipelined_compilation/wrap.bzl | 106 ++++++++++++++++++ 5 files changed, 191 insertions(+), 21 deletions(-) create mode 100644 test/unit/pipelined_compilation/custom_rule_test/to_wrap.rs create mode 100644 test/unit/pipelined_compilation/custom_rule_test/uses_wrapper.rs create mode 100644 test/unit/pipelined_compilation/wrap.bzl diff --git a/test/unit/force_all_deps_direct/generator.bzl b/test/unit/force_all_deps_direct/generator.bzl index 370606b511..2ca319587d 100644 --- a/test/unit/force_all_deps_direct/generator.bzl +++ b/test/unit/force_all_deps_direct/generator.bzl @@ -9,9 +9,6 @@ load("//rust/private:providers.bzl", "BuildInfo", "CrateInfo", "DepInfo", "DepVa # buildifier: disable=bzl-visibility load("//rust/private:rustc.bzl", "rustc_compile_action") -# buildifier: disable=bzl-visibility -load("//rust/private:utils.bzl", "can_build_metadata") - def _generator_impl(ctx): rs_file = ctx.actions.declare_file(ctx.label.name + "_generated.rs") ctx.actions.run_shell( @@ -42,12 +39,6 @@ EOF lib_hash = output_hash, extension = ".rlib", ) - rust_metadata_name = "{prefix}{name}-{lib_hash}{extension}".format( - prefix = "lib", - name = crate_name, - lib_hash = output_hash, - extension = ".rmeta", - ) deps = [DepVariantInfo( crate_info = dep[CrateInfo] if CrateInfo in dep else None, @@ -57,9 +48,6 @@ EOF ) for dep in ctx.attr.deps] rust_lib = ctx.actions.declare_file(rust_lib_name) - rust_metadata = None - if can_build_metadata(toolchain, ctx, crate_type): - rust_metadata = ctx.actions.declare_file(rust_metadata_name) return rustc_compile_action( ctx = ctx, attr = ctx.attr, @@ -73,7 +61,6 @@ EOF proc_macro_deps = depset([]), aliases = {}, output = rust_lib, - metadata = rust_metadata, owner = ctx.label, edition = "2018", compile_data = depset([]), diff --git a/test/unit/pipelined_compilation/custom_rule_test/to_wrap.rs b/test/unit/pipelined_compilation/custom_rule_test/to_wrap.rs new file mode 100644 index 0000000000..5fee30b2b9 --- /dev/null +++ b/test/unit/pipelined_compilation/custom_rule_test/to_wrap.rs @@ -0,0 +1,3 @@ +pub fn to_wrap() { + eprintln!("something"); +} diff --git a/test/unit/pipelined_compilation/custom_rule_test/uses_wrapper.rs b/test/unit/pipelined_compilation/custom_rule_test/uses_wrapper.rs new file mode 100644 index 0000000000..d932467b27 --- /dev/null +++ b/test/unit/pipelined_compilation/custom_rule_test/uses_wrapper.rs @@ -0,0 +1,5 @@ +use wrapper::wrap; + +pub fn calls_wrap() { + wrap(); +} diff --git a/test/unit/pipelined_compilation/pipelined_compilation_test.bzl b/test/unit/pipelined_compilation/pipelined_compilation_test.bzl index 403271e1d3..99115beafb 100644 --- a/test/unit/pipelined_compilation/pipelined_compilation_test.bzl +++ b/test/unit/pipelined_compilation/pipelined_compilation_test.bzl @@ -3,6 +3,17 @@ load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") load("//rust:defs.bzl", "rust_binary", "rust_library", "rust_proc_macro") load("//test/unit:common.bzl", "assert_argv_contains", "assert_list_contains_adjacent_elements", "assert_list_contains_adjacent_elements_not") +load(":wrap.bzl", "wrap") + +NOT_WINDOWS = select({ + "@platforms//os:linux": [], + "@platforms//os:macos": [], + "//conditions:default": ["@platforms//:incompatible"], +}) + +ENABLE_PIPELINING = { + "@//rust/settings:pipelined_compilation": True, +} def _second_lib_test_impl(ctx): env = analysistest.begin(ctx) @@ -67,9 +78,6 @@ def _bin_test_impl(ctx): return analysistest.end(env) -ENABLE_PIPELINING = { - "@//rust/settings:pipelined_compilation": True, -} bin_test = analysistest.make(_bin_test_impl, config_settings = ENABLE_PIPELINING) second_lib_test = analysistest.make(_second_lib_test_impl, config_settings = ENABLE_PIPELINING) @@ -101,14 +109,71 @@ def _pipelined_compilation_test(): deps = [":second"], ) - NOT_WINDOWS = select({ - "@platforms//os:linux": [], - "@platforms//os:macos": [], - "//conditions:default": ["@platforms//:incompatible"], - }) 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): + env = analysistest.begin(ctx) + tut = analysistest.target_under_test(env) + + # This is the metadata-generating action. It should depend on metadata for the library and, if generate_metadata is set + # also depend on metadata for 'wrapper'. + rust_action = [act for act in tut.actions if act.mnemonic == "RustcMetadata"][0] + + metadata_inputs = [i for i in rust_action.inputs.to_list() if i.path.endswith(".rmeta")] + rlib_inputs = [i for i in rust_action.inputs.to_list() if i.path.endswith(".rlib")] + + seen_wrapper_metadata = False + seen_to_wrap = 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_wrapper_rlib = True + for ri in rlib_inputs: + if "libwrapper" in ri.path: + seen_wrapper_rlib = True + + if ctx.attr.generate_metadata: + asserts.true(env, seen_wrapper_metadata, "expected dependency on metadata for 'wrapper' but not found") + else: + asserts.true(env, seen_wrapper_rlib, "expected dependency on rlib for 'wrapper' but not found") + + asserts.true(env, seen_to_wrap, "expected dependency on metadata for 'to_wrap' but not found") + + return analysistest.end(env) + +custom_rule_test = analysistest.make(_custom_rule_test_impl, attrs = {"generate_metadata": attr.bool()}, config_settings = ENABLE_PIPELINING) + +def _custom_rule_test(generate_metadata, prefix): + rust_library( + name = "to_wrap" + prefix, + crate_name = "to_wrap", + srcs = ["custom_rule_test/to_wrap.rs"], + edition = "2021", + ) + wrap( + name = "wrapper" + prefix, + crate_name = "wrapper", + target = ":to_wrap" + prefix, + generate_metadata = generate_metadata, + ) + rust_library( + name = "uses_wrapper" + prefix, + srcs = ["custom_rule_test/uses_wrapper.rs"], + deps = [":wrapper" + prefix], + edition = "2021", + ) + + custom_rule_test( + name = "custom_rule_test" + prefix, + generate_metadata = generate_metadata, + target_compatible_with = NOT_WINDOWS, + target_under_test = ":uses_wrapper" + prefix, + ) + def pipelined_compilation_test_suite(name): """Entry-point macro called from the BUILD file. @@ -116,11 +181,15 @@ 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") native.test_suite( name = name, tests = [ ":bin_test", ":second_lib_test", + ":custom_rule_test_with_metadata", + ":custom_rule_test_without_metadata", ], ) diff --git a/test/unit/pipelined_compilation/wrap.bzl b/test/unit/pipelined_compilation/wrap.bzl new file mode 100644 index 0000000000..36055d0500 --- /dev/null +++ b/test/unit/pipelined_compilation/wrap.bzl @@ -0,0 +1,106 @@ +"""A custom rule that wraps a crate called to_wrap.""" + +# buildifier: disable=bzl-visibility +load("//rust/private:common.bzl", "rust_common") + +# buildifier: disable=bzl-visibility +load("//rust/private:providers.bzl", "BuildInfo", "CrateInfo", "DepInfo", "DepVariantInfo") + +# buildifier: disable=bzl-visibility +load("//rust/private:rustc.bzl", "rustc_compile_action") + +def _wrap_impl(ctx): + rs_file = ctx.actions.declare_file(ctx.label.name + "_wrapped.rs") + crate_name = ctx.attr.crate_name if ctx.attr.crate_name else ctx.label.name + ctx.actions.run_shell( + outputs = [rs_file], + command = """cat < {} +// crate_name: {} +use to_wrap::to_wrap; + +pub fn wrap() {{ + to_wrap(); +}} +EOF +""".format(rs_file.path, crate_name), + mnemonic = "WriteWrapperRsFile", + ) + + toolchain = ctx.toolchains[Label("//rust:toolchain")] + + # Determine unique hash for this rlib + output_hash = repr(hash(rs_file.path)) + crate_type = "rlib" + + rust_lib_name = "{prefix}{name}-{lib_hash}{extension}".format( + prefix = "lib", + name = crate_name, + lib_hash = output_hash, + extension = ".rlib", + ) + rust_metadata_name = "{prefix}{name}-{lib_hash}{extension}".format( + prefix = "lib", + name = crate_name, + lib_hash = output_hash, + extension = ".rmeta", + ) + + tgt = ctx.attr.target + deps = [DepVariantInfo( + crate_info = tgt[CrateInfo] if CrateInfo in tgt else None, + dep_info = tgt[DepInfo] if DepInfo in tgt else None, + build_info = tgt[BuildInfo] if BuildInfo in tgt else None, + cc_info = tgt[CcInfo] if CcInfo in tgt else None, + )] + + rust_lib = ctx.actions.declare_file(rust_lib_name) + rust_metadata = None + if ctx.attr.generate_metadata: + rust_metadata = ctx.actions.declare_file(rust_metadata_name) + return rustc_compile_action( + ctx = ctx, + attr = ctx.attr, + toolchain = toolchain, + crate_info = rust_common.create_crate_info( + name = crate_name, + type = crate_type, + root = rs_file, + srcs = depset([rs_file]), + deps = depset(deps), + proc_macro_deps = depset([]), + aliases = {}, + output = rust_lib, + metadata = rust_metadata, + owner = ctx.label, + edition = "2018", + compile_data = depset([]), + rustc_env = {}, + is_test = False, + ), + output_hash = output_hash, + force_all_deps_direct = True, + ) + +wrap = rule( + implementation = _wrap_impl, + attrs = { + "crate_name": attr.string(), + "generate_metadata": attr.bool(default = False), + "target": attr.label(), + "_cc_toolchain": attr.label( + default = Label("@bazel_tools//tools/cpp:current_cc_toolchain"), + ), + "_error_format": attr.label( + default = Label("//:error_format"), + ), + "_process_wrapper": attr.label( + default = Label("//util/process_wrapper"), + executable = True, + allow_single_file = True, + cfg = "exec", + ), + }, + toolchains = ["@rules_rust//rust:toolchain", "@bazel_tools//tools/cpp:toolchain_type"], + incompatible_use_toolchain_transition = True, + fragments = ["cpp"], +) From 0794e2815f33fd4e048a064606413c244aa1de48 Mon Sep 17 00:00:00 2001 From: Roberto Bampi Date: Thu, 4 Aug 2022 15:00:43 +0200 Subject: [PATCH 7/7] Address review comments --- docs/flatten.md | 2 +- docs/providers.md | 2 +- rust/private/providers.bzl | 2 +- rust/private/rustc.bzl | 16 ++-- rust/private/rustdoc.bzl | 2 +- .../pipelined_compilation_test.bzl | 76 ++++++++++++++----- test/unit/pipelined_compilation/wrap.bzl | 1 - 7 files changed, 67 insertions(+), 34 deletions(-) diff --git a/docs/flatten.md b/docs/flatten.md index 20eb643fe6..87af5c50b8 100644 --- a/docs/flatten.md +++ b/docs/flatten.md @@ -1401,7 +1401,7 @@ A provider containing information about a Crate's dependencies. | 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. | diff --git a/docs/providers.md b/docs/providers.md index 93fcda7d2d..775dbd9608 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -61,7 +61,7 @@ A provider containing information about a Crate's dependencies. | 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. | diff --git a/rust/private/providers.bzl b/rust/private/providers.bzl index 9600718b2a..7533349e66 100644 --- a/rust/private/providers.bzl +++ b/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.", }, ) diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index 58ff91264d..0fe072b8e1 100644 --- a/rust/private/rustc.bzl +++ b/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) diff --git a/rust/private/rustdoc.bzl b/rust/private/rustdoc.bzl index 2660794d96..6874717dc7 100644 --- a/rust/private/rustdoc.bzl +++ b/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 diff --git a/test/unit/pipelined_compilation/pipelined_compilation_test.bzl b/test/unit/pipelined_compilation/pipelined_compilation_test.bzl index 99115beafb..93382c56b9 100644 --- a/test/unit/pipelined_compilation/pipelined_compilation_test.bzl +++ b/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", ], ) diff --git a/test/unit/pipelined_compilation/wrap.bzl b/test/unit/pipelined_compilation/wrap.bzl index 36055d0500..11b84808dc 100644 --- a/test/unit/pipelined_compilation/wrap.bzl +++ b/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(