From 42bb66dd5a2ad189611facf3d243d9eb03da38ef Mon Sep 17 00:00:00 2001 From: Armando Montanez Date: Mon, 8 Dec 2025 14:50:08 -0800 Subject: [PATCH] Do not forward ARFLAGS This is a partial revert of #3704. It does NOT revert logic that properly finds the hermetic archiver (binary and associated tests). cc-rs needs full ownership of flag handling for the archiver. This is in part due to `ar` having very specific positional flag behaviors, and partially due to some special logic in how cc-rs constructs static libraries. The change to enable ARFLAGS was causing many toolchains that pass flags to the static archiver to error out due to conflicting expectations for flag handling. Example bad `ar` invocation, where `cq` is treated as the destination archive instead of the `ar` tool mode: ar rcsD cq dest.a src1.o src2.o This drops forwarding of `ARFLAGS` to fix the common case. --- cargo/private/cargo_build_script.bzl | 15 ++++----- .../cc_args_and_env/BUILD.bazel | 3 -- .../cc_args_and_env/cc_args_and_env_test.bzl | 33 +++++++++++-------- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_script.bzl index 13dd45027a..5fdde2e0af 100644 --- a/cargo/private/cargo_build_script.bzl +++ b/cargo/private/cargo_build_script.bzl @@ -131,7 +131,6 @@ def get_cc_compile_args_and_env(cc_toolchain, feature_configuration): tuple: A tuple of the following items: - (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( @@ -148,17 +147,12 @@ 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_ar_args, cc_env + return cc_c_args, cc_cxx_args, cc_env def _pwd_flags_sysroot(args): """Prefix execroot-relative paths of known arguments with ${pwd}. @@ -429,12 +423,13 @@ def _cargo_build_script_impl(ctx): env["CC"] = "${{pwd}}/{}".format(ctx.executable._fallback_cc.path) env["CXX"] = "${{pwd}}/{}".format(ctx.executable._fallback_cxx.path) env["AR"] = "${{pwd}}/{}".format(ctx.executable._fallback_ar.path) + env["ARFLAGS"] = "" env["CFLAGS"] = "" env["CXXFLAGS"] = "" if cc_toolchain: # MSVC requires INCLUDE to be set - cc_c_args, cc_cxx_args, cc_ar_args, cc_env = get_cc_compile_args_and_env(cc_toolchain, feature_configuration) + cc_c_args, cc_cxx_args, cc_env = get_cc_compile_args_and_env(cc_toolchain, feature_configuration) include = cc_env.get("INCLUDE") if include: env["INCLUDE"] = include @@ -464,7 +459,9 @@ def _cargo_build_script_impl(ctx): # 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)) + # It may be tempting to forward ARFLAGS, but cc-rs is opinionated enough + # that doing so is more likely to hurt than help. If you need to change + # ARFLAGS, make changes to cc-rs. # Inform build scripts of rustc flags # https://github.com/rust-lang/cargo/issues/9600 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 5915540557..7c35d65640 100644 --- a/test/cargo_build_script/cc_args_and_env/BUILD.bazel +++ b/test/cargo_build_script/cc_args_and_env/BUILD.bazel @@ -1,6 +1,5 @@ load( "cc_args_and_env_test.bzl", - "ar_flags_test", "bindir_absolute_test", "bindir_relative_test", "fsanitize_ignorelist_absolute_test", @@ -31,6 +30,4 @@ 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 2c7cc8b0f7..512cc2d545 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 @@ -54,6 +54,16 @@ def _test_cc_config_impl(ctx): name = "default_ar_flags", enabled = True, flag_sets = [ + flag_set( + actions = [ACTION_NAMES.cpp_link_static_library], + flag_groups = ([ + flag_group( + # Simulate a case where a toolchain always passes + # rcsD to `ar`. + flags = ["rcsD"], + ), + ]), + ), flag_set( actions = [ACTION_NAMES.cpp_link_static_library], flag_groups = ([ @@ -207,7 +217,6 @@ def _cc_args_and_env_analysis_test_impl(ctx): ) _ENV_VAR_TO_EXPECTED_ARGS = { - "ARFLAGS": ctx.attr.expected_arflags, "CFLAGS": ctx.attr.expected_cflags, "CXXFLAGS": ctx.attr.expected_cxxflags, } @@ -221,13 +230,22 @@ def _cc_args_and_env_analysis_test_impl(ctx): "error: expected '{}' to be in cargo {}: '{}'".format(flag, env_var, actual_flags), ) + arflags = cargo_action.env["ARFLAGS"] + asserts.equals( + env, + [], + arflags.split(" ") if arflags else [], + "ARFLAGS is intentionally always empty. cc-rs tightly controls the " + + "archiver flags in such a way that forwarding standard flags as " + + "set up by most Bazel C/C++ toolchains is extremely error-prone.", + ) + 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_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), @@ -394,17 +412,6 @@ def fsanitize_ignorelist_absolute_test(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,