diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_script.bzl index ae1b4c0fd6..3572675a0c 100644 --- a/cargo/private/cargo_build_script.bzl +++ b/cargo/private/cargo_build_script.bzl @@ -130,9 +130,10 @@ def get_cc_compile_args_and_env(cc_toolchain, feature_configuration): Returns: tuple: A tuple of the following items: - - (sequence): A flattened C command line flags for given action. - - (sequence): A flattened CXX command line flags for given action. - - (dict): C environment variables to be set for given action. + - (sequence): A flattened list of C command line flags. + - (sequence): A flattened list of CXX command line flags. + - (sequence): A flattened list of AR command line flags. + - (dict): C environment variables to be set for this configuration. """ compile_variables = cc_common.create_compile_variables( feature_configuration = feature_configuration, @@ -148,12 +149,17 @@ def get_cc_compile_args_and_env(cc_toolchain, feature_configuration): action_name = ACTION_NAMES.cpp_compile, variables = compile_variables, ) + cc_ar_args = cc_common.get_memory_inefficient_command_line( + feature_configuration = feature_configuration, + action_name = ACTION_NAMES.cpp_link_static_library, + variables = compile_variables, + ) cc_env = cc_common.get_environment_variables( feature_configuration = feature_configuration, action_name = ACTION_NAMES.c_compile, variables = compile_variables, ) - return cc_c_args, cc_cxx_args, cc_env + return cc_c_args, cc_cxx_args, cc_ar_args, cc_env def _pwd_flags_sysroot(args): """Prefix execroot-relative paths of known arguments with ${pwd}. @@ -423,7 +429,7 @@ def _cargo_build_script_impl(ctx): env["LDFLAGS"] = " ".join(_pwd_flags(link_args)) # MSVC requires INCLUDE to be set - cc_c_args, cc_cxx_args, cc_env = get_cc_compile_args_and_env(cc_toolchain, feature_configuration) + cc_c_args, cc_cxx_args, cc_ar_args, cc_env = get_cc_compile_args_and_env(cc_toolchain, feature_configuration) include = cc_env.get("INCLUDE") if include: env["INCLUDE"] = include @@ -444,11 +450,17 @@ def _cargo_build_script_impl(ctx): action_name = ACTION_NAMES.cpp_link_static_library, ) + # Many C/C++ toolchains are missing an action_config for AR because + # one was never included in the unix_cc_toolchain_config. + if not env["AR"]: + env["AR"] = cc_toolchain.ar_executable + # Populate CFLAGS and CXXFLAGS that cc-rs relies on when building from source, in particular # to determine the deployment target when building for apple platforms (`macosx-version-min` # for example, itself derived from the `macos_minimum_os` Bazel argument). env["CFLAGS"] = " ".join(_pwd_flags(cc_c_args)) env["CXXFLAGS"] = " ".join(_pwd_flags(cc_cxx_args)) + env["ARFLAGS"] = " ".join(_pwd_flags(cc_ar_args)) # Inform build scripts of rustc flags # https://github.com/rust-lang/cargo/issues/9600 diff --git a/cargo/private/cargo_build_script_runner/bin.rs b/cargo/private/cargo_build_script_runner/bin.rs index 51a52fc26c..27f5848edc 100644 --- a/cargo/private/cargo_build_script_runner/bin.rs +++ b/cargo/private/cargo_build_script_runner/bin.rs @@ -131,8 +131,13 @@ fn run_buildrs() -> Result<(), String> { // The default OSX toolchain uses libtool as ar_executable not ar. // This doesn't work when used as $AR, so simply don't set it - tools will probably fall back to // /usr/bin/ar which is probably good enough. - if Path::new(&ar_path).file_name() == Some("libtool".as_ref()) { + let file_name = Path::new(&ar_path) + .file_name() + .ok_or_else(|| "Failed while getting file name".to_string())? + .to_string_lossy(); + if file_name.contains("libtool") { command.env_remove("AR"); + command.env_remove("ARFLAGS"); } else { command.env("AR", exec_root.join(ar_path)); } diff --git a/test/cargo_build_script/cc_args_and_env/BUILD.bazel b/test/cargo_build_script/cc_args_and_env/BUILD.bazel index e2620705c3..5915540557 100644 --- a/test/cargo_build_script/cc_args_and_env/BUILD.bazel +++ b/test/cargo_build_script/cc_args_and_env/BUILD.bazel @@ -1,11 +1,13 @@ load( "cc_args_and_env_test.bzl", + "ar_flags_test", "bindir_absolute_test", "bindir_relative_test", "fsanitize_ignorelist_absolute_test", "fsanitize_ignorelist_relative_test", "isystem_absolute_test", "isystem_relative_test", + "legacy_cc_toolchain_test", "sysroot_absolute_test", "sysroot_next_absolute_test", "sysroot_relative_test", @@ -28,3 +30,7 @@ bindir_absolute_test(name = "bindir_absolute_test") fsanitize_ignorelist_absolute_test(name = "fsanitize_ignorelist_absolute_test") fsanitize_ignorelist_relative_test(name = "fsanitize_ignorelist_relative_test") + +ar_flags_test(name = "ar_flags_test") + +legacy_cc_toolchain_test(name = "legacy_cc_toolchain_test") diff --git a/test/cargo_build_script/cc_args_and_env/cc_args_and_env_test.bzl b/test/cargo_build_script/cc_args_and_env/cc_args_and_env_test.bzl index b8ac5a74bf..2c7cc8b0f7 100644 --- a/test/cargo_build_script/cc_args_and_env/cc_args_and_env_test.bzl +++ b/test/cargo_build_script/cc_args_and_env/cc_args_and_env_test.bzl @@ -8,12 +8,18 @@ To verify the processed cargo cc_args, we use cc_args_and_env_analysis_test(). """ load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") -load("@rules_cc//cc:action_names.bzl", "ACTION_NAME_GROUPS") -load("@rules_cc//cc:cc_toolchain_config_lib.bzl", "feature", "flag_group", "flag_set") +load("@rules_cc//cc:action_names.bzl", "ACTION_NAMES", "ACTION_NAME_GROUPS") +load("@rules_cc//cc:cc_toolchain_config_lib.bzl", "action_config", "feature", "flag_group", "flag_set", "tool", "tool_path") load("@rules_cc//cc:defs.bzl", "cc_toolchain") load("@rules_cc//cc/common:cc_common.bzl", "cc_common") load("//cargo:defs.bzl", "cargo_build_script") +_EXPECTED_CC_TOOLCHAIN_TOOLS = { + "AR": "/usr/fake/ar", + "CC": "/usr/fake/gcc", + "CXX": "/usr/fake/g++", +} + def _test_cc_config_impl(ctx): features = [ feature( @@ -30,9 +36,96 @@ def _test_cc_config_impl(ctx): ), ], ), + feature( + name = "default_cpp_flags", + enabled = True, + flag_sets = [ + flag_set( + actions = ACTION_NAME_GROUPS.all_cpp_compile_actions, + flag_groups = ([ + flag_group( + flags = ctx.attr.extra_cxx_compile_flags, + ), + ]), + ), + ], + ), + feature( + name = "default_ar_flags", + enabled = True, + flag_sets = [ + flag_set( + actions = [ACTION_NAMES.cpp_link_static_library], + flag_groups = ([ + flag_group( + flags = ctx.attr.extra_ar_flags, + ), + ]), + ), + ], + ), ] + + tool_paths = [] + action_configs = [] + if ctx.attr.legacy_cc_toolchain: + tool_paths = [ + tool_path( + name = "gcc", + path = _EXPECTED_CC_TOOLCHAIN_TOOLS["CC"], + ), + tool_path( + name = "cpp", + path = _EXPECTED_CC_TOOLCHAIN_TOOLS["CXX"], + ), + tool_path( + name = "ar", + path = _EXPECTED_CC_TOOLCHAIN_TOOLS["AR"], + ), + # These need to be set to something to create a toolchain, but + # is not tested here. + tool_path( + name = "ld", + path = "/usr/ignored/false", + ), + tool_path( + name = "nm", + path = "/usr/ignored/false", + ), + tool_path( + name = "objdump", + path = "/usr/ignored/false", + ), + tool_path( + name = "strip", + path = "/usr/ignored/false", + ), + ] + else: + action_configs = ( + action_config( + action_name = ACTION_NAMES.c_compile, + tools = [ + tool(path = _EXPECTED_CC_TOOLCHAIN_TOOLS["CC"]), + ], + ), + action_config( + action_name = ACTION_NAMES.cpp_compile, + tools = [ + tool(path = _EXPECTED_CC_TOOLCHAIN_TOOLS["CXX"]), + ], + ), + action_config( + action_name = ACTION_NAMES.cpp_link_static_library, + tools = [ + tool(path = _EXPECTED_CC_TOOLCHAIN_TOOLS["AR"]), + ], + ), + ) + config_info = cc_common.create_cc_toolchain_config_info( ctx = ctx, + action_configs = action_configs, toolchain_identifier = "test-cc-toolchain", host_system_name = "unknown", target_system_name = "unknown", @@ -42,13 +135,17 @@ def _test_cc_config_impl(ctx): abi_version = "unknown", abi_libc_version = "unknown", features = features, + tool_paths = tool_paths, ) return config_info test_cc_config = rule( implementation = _test_cc_config_impl, attrs = { + "extra_ar_flags": attr.string_list(), "extra_cc_compile_flags": attr.string_list(), + "extra_cxx_compile_flags": attr.string_list(), + "legacy_cc_toolchain": attr.bool(default = False), }, provides = [CcToolchainConfigInfo], ) @@ -84,27 +181,66 @@ def _cc_args_and_env_analysis_test_impl(ctx): env = analysistest.begin(ctx) tut = analysistest.target_under_test(env) cargo_action = tut[DepActionsInfo].actions[0] - cflags = cargo_action.env["CFLAGS"].split(" ") - for flag in ctx.attr.expected_cflags: - asserts.true( + + for env_var, expected_path in _EXPECTED_CC_TOOLCHAIN_TOOLS.items(): + if ctx.attr.legacy_cc_toolchain and env_var == "CXX": + # When using the legacy tool_path toolchain configuration approach, + # the CXX tool is forced to be the same as the the CC tool. + # See: https://github.com/bazelbuild/bazel/blob/14840856986f21b54330e352b83d687825648889/src/main/starlark/builtins_bzl/common/cc/toolchain_config/legacy_features.bzl#L1296-L1304 + expected_path = _EXPECTED_CC_TOOLCHAIN_TOOLS["CC"] + + actual_path = cargo_action.env.get(env_var) + asserts.false( + env, + actual_path == None, + "error: '{}' tool unset".format(env_var), + ) + asserts.equals( env, - flag in cflags, - "error: expected '{}' to be in cargo CFLAGS: '{}'".format(flag, cflags), + expected_path, + actual_path, + "error: '{}' tool path '{}' does not match expected '{}'".format( + env_var, + actual_path, + expected_path, + ), ) + _ENV_VAR_TO_EXPECTED_ARGS = { + "ARFLAGS": ctx.attr.expected_arflags, + "CFLAGS": ctx.attr.expected_cflags, + "CXXFLAGS": ctx.attr.expected_cxxflags, + } + + for env_var, expected_flags in _ENV_VAR_TO_EXPECTED_ARGS.items(): + actual_flags = cargo_action.env[env_var].split(" ") + for flag in expected_flags: + asserts.true( + env, + flag in actual_flags, + "error: expected '{}' to be in cargo {}: '{}'".format(flag, env_var, actual_flags), + ) + return analysistest.end(env) cc_args_and_env_analysis_test = analysistest.make( impl = _cc_args_and_env_analysis_test_impl, doc = """An analysistest to examine the custom cargo flags of an cargo_build_script target.""", attrs = { - "expected_cflags": attr.string_list(), + "expected_arflags": attr.string_list(default = ["-x"]), + "expected_cflags": attr.string_list(default = ["-Wall"]), + "expected_cxxflags": attr.string_list(default = ["-fno-rtti"]), + "legacy_cc_toolchain": attr.bool(default = False), }, ) def cargo_build_script_with_extra_cc_compile_flags( + *, name, - extra_cc_compile_flags): + extra_cc_compile_flags = ["-Wall"], + extra_cxx_compile_flags = ["-fno-rtti"], + extra_ar_flags = ["-x"], + legacy_cc_toolchain = False): """Produces a test cargo_build_script target that's set up to use a custom cc_toolchain with the extra_cc_compile_flags. This is achieved by creating a cascade of targets: @@ -115,12 +251,19 @@ def cargo_build_script_with_extra_cc_compile_flags( Args: name: The name of the test target. - extra_cc_compile_flags: Extra args for the cc_toolchain. + extra_cc_compile_flags: Extra C/C++ args for the cc_toolchain. + extra_cxx_compile_flags: Extra C++-specific args for the cc_toolchain. + extra_ar_flags: Extra archiver args for the cc_toolchain. + legacy_cc_toolchain: Enables legacy tool_path configuration of the cc + cc toolchain. """ test_cc_config( name = "%s/cc_toolchain_config" % name, extra_cc_compile_flags = extra_cc_compile_flags, + extra_cxx_compile_flags = extra_cxx_compile_flags, + extra_ar_flags = extra_ar_flags, + legacy_cc_toolchain = legacy_cc_toolchain, ) cc_toolchain( name = "%s/test_cc_toolchain_impl" % name, @@ -250,3 +393,25 @@ def fsanitize_ignorelist_absolute_test(name): target_under_test = "%s/cargo_build_script" % name, expected_cflags = ["-fsanitize-ignorelist=/test/absolute/path"], ) + +def ar_flags_test(name): + cargo_build_script_with_extra_cc_compile_flags( + name = "%s/cargo_build_script" % name, + extra_ar_flags = ["-static"], + ) + cc_args_and_env_analysis_test( + name = name, + target_under_test = "%s/cargo_build_script" % name, + expected_arflags = ["-static"], + ) + +def legacy_cc_toolchain_test(name): + cargo_build_script_with_extra_cc_compile_flags( + name = "%s/cargo_build_script" % name, + legacy_cc_toolchain = True, + ) + cc_args_and_env_analysis_test( + name = name, + target_under_test = "%s/cargo_build_script" % name, + legacy_cc_toolchain = True, + )