Do not forward ARFLAGS (#3763)

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.

Author: armandomontanez

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

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")

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,