From 84e98e4d2fb5545057c1802fe45e8a5fe29d48c4 Mon Sep 17 00:00:00 2001 From: Krasimir Georgiev Date: Thu, 24 Mar 2022 15:43:41 +0100 Subject: [PATCH] don't emit --codegen={metadata,extra-filename} for rust_static_library and rust_shared_library (#1222) Extracted from https://github.com/bazelbuild/rules_rust/pull/1219/files, addressing this part: * currently we don't include the output hash for cdylib, but we do for staticlib. It's more accurate to not include the output hash for staticlib as well. Related to, but does not address https://github.com/bazelbuild/rules_rust/issues/1063. @scentini suggested to not even emit --codegen flags in these cases. Co-authored-by: Marcel Hlopko --- rust/private/rust.bzl | 12 +++---- rust/private/rustc.bzl | 16 +++++++--- test/unit/crate_name/crate_name_test.bzl | 40 ++++++++++++++++++++++-- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/rust/private/rust.bzl b/rust/private/rust.bzl index 3b17d6205e..af11c9fe1f 100644 --- a/rust/private/rust.bzl +++ b/rust/private/rust.bzl @@ -245,14 +245,14 @@ def _rust_library_common(ctx, crate_type): toolchain = find_toolchain(ctx) # Determine unique hash for this rlib. - # Note that we don't include a hash for `cdylib` since they are meant to be consumed externally and having a - # deterministic name is important since it ends up embedded in the executable. This is problematic when one needs - # to include the library with a specific filename into a larger application. + # Note that we don't include a hash for `cdylib` and `staticlib` since they are meant to be consumed externally + # and having a deterministic name is important since it ends up embedded in the executable. This is problematic + # when one needs to include the library with a specific filename into a larger application. # (see https://github.com/bazelbuild/rules_rust/issues/405#issuecomment-993089889 for more details) - if crate_type != "cdylib": - output_hash = determine_output_hash(crate_root, ctx.label) - else: + if crate_type in ["cdylib", "staticlib"]: output_hash = None + else: + output_hash = determine_output_hash(crate_root, ctx.label) crate_name = compute_crate_name(ctx.workspace_name, ctx.label, toolchain, ctx.attr.crate_name) rust_lib_name = _determine_lib_name( diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index dc8ed83141..22f0f190b9 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -714,12 +714,20 @@ def construct_arguments( if hasattr(attr, "_error_format"): rustc_flags.add("--error-format=" + attr._error_format[ErrorFormatInfo].error_format) - # Mangle symbols to disambiguate crates with the same name - extra_filename = "-" + output_hash if output_hash else "" - rustc_flags.add("--codegen=metadata=" + extra_filename) + # Mangle symbols to disambiguate crates with the same name. This could + # happen only for non-final artifacts where we compute an output_hash, + # e.g., rust_library. + # + # For "final" artifacts and ones intended for distribution outside of + # Bazel, such as rust_binary, rust_static_library and rust_shared_library, + # where output_hash is None we don't need to add these flags. + if output_hash: + extra_filename = "-" + output_hash + rustc_flags.add("--codegen=metadata=" + extra_filename) + rustc_flags.add("--codegen=extra-filename=" + extra_filename) + if output_dir: rustc_flags.add("--out-dir=" + output_dir) - rustc_flags.add("--codegen=extra-filename=" + extra_filename) compilation_mode = get_compilation_mode_opts(ctx, toolchain) rustc_flags.add("--codegen=opt-level=" + compilation_mode.opt_level) diff --git a/test/unit/crate_name/crate_name_test.bzl b/test/unit/crate_name/crate_name_test.bzl index ccc7a18794..43a9c1b6c2 100644 --- a/test/unit/crate_name/crate_name_test.bzl +++ b/test/unit/crate_name/crate_name_test.bzl @@ -1,8 +1,8 @@ """Unit tests for crate names.""" load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") -load("//rust:defs.bzl", "rust_binary", "rust_library", "rust_test") -load("//test/unit:common.bzl", "assert_argv_contains") +load("//rust:defs.bzl", "rust_binary", "rust_library", "rust_shared_library", "rust_static_library", "rust_test") +load("//test/unit:common.bzl", "assert_argv_contains", "assert_argv_contains_prefix_not") def _default_crate_name_library_test_impl(ctx): env = analysistest.begin(ctx) @@ -68,9 +68,22 @@ def _slib_library_name_test_impl(ctx): """ env = analysistest.begin(ctx) tut = analysistest.target_under_test(env) + assert_argv_contains(env, tut.actions[0], "--codegen=metadata=-2102077805") assert_argv_contains(env, tut.actions[0], "--codegen=extra-filename=-2102077805") return analysistest.end(env) +def _no_extra_filename_test_impl(ctx): + """Check that no extra filename is used. + + Args: + ctx: rule context. + """ + env = analysistest.begin(ctx) + tut = analysistest.target_under_test(env) + assert_argv_contains_prefix_not(env, tut.actions[0], "--codegen=metadata=") + assert_argv_contains_prefix_not(env, tut.actions[0], "--codegen=extra-filename=") + return analysistest.end(env) + default_crate_name_library_test = analysistest.make( _default_crate_name_library_test_impl, ) @@ -100,6 +113,9 @@ invalid_custom_crate_name_test = analysistest.make( slib_library_name_test = analysistest.make( _slib_library_name_test_impl, ) +no_extra_filename_test = analysistest.make( + _no_extra_filename_test_impl, +) def _crate_name_test(): rust_library( @@ -153,6 +169,16 @@ def _crate_name_test(): srcs = ["slib.rs"], ) + rust_shared_library( + name = "shared_lib", + srcs = ["lib.rs"], + ) + + rust_static_library( + name = "static_lib", + srcs = ["lib.rs"], + ) + slib_library_name_test( name = "slib_library_name_test", target_under_test = ":slib", @@ -198,6 +224,16 @@ def _crate_name_test(): target_under_test = ":invalid-custom-crate-name", ) + no_extra_filename_test( + name = "no_extra_filename_for_shared_library_test", + target_under_test = ":shared_lib", + ) + + no_extra_filename_test( + name = "no_extra_filename_for_static_library_test", + target_under_test = ":static_lib", + ) + def crate_name_test_suite(name): """Entry-point macro called from the BUILD file.