Move -ObjC into a separate feature (#210)

This extracts the -ObjC flag into a separate crosstool feature, and
makes it apply to all CC linking actions. Previously this only applied
to objc-executable actions, but as this gets unified into the cc linking
behavior it makes sense to apply it to the cc actions as well. This has
the potential downside that if you had a cc_binary target that had
Objective-C in the dependency tree that it did not use, previously it
would not have been loaded and now it would. If that becomes a problem
this flag can be removed on thew binary in question by disabling this
feature. This also allows us to remove this flag from rules_swift's
logic since it was there for the same reason.

7 files changed, 239 insertions, 2 deletions

diff --git a/crosstool/cc_toolchain_config.bzl b/crosstool/cc_toolchain_config.bzl
index 6aed695..f2e3a67 100644
--- a/crosstool/cc_toolchain_config.bzl
+++ b/crosstool/cc_toolchain_config.bzl
@@ -186,6 +186,18 @@ def _impl(ctx):
         ],
     )
 
+    objc_link_flag_feature = feature(
+        name = "objc_link_flag",
+        enabled = True,
+        flag_sets = [
+            flag_set(
+                actions = _DYNAMIC_LINK_ACTIONS,
+                flag_groups = [flag_group(flags = ["-ObjC"])],
+                with_features = [with_feature_set(not_features = ["kernel_extension"])],
+            ),
+        ],
+    )
+
     objcpp_executable_action = action_config(
         action_name = _OBJCPP_EXECUTABLE_ACTION_NAME,
         flag_sets = [
@@ -197,7 +209,6 @@ def _impl(ctx):
                             "-objc_abi_version",
                             "-Xlinker",
                             "2",
-                            "-ObjC",
                         ],
                     ),
                 ],
@@ -457,7 +468,6 @@ def _impl(ctx):
                             "-objc_abi_version",
                             "-Xlinker",
                             "2",
-                            "-ObjC",
                         ],
                     ),
                 ],
@@ -2530,6 +2540,7 @@ def _impl(ctx):
         archiver_flags_feature,
         runtime_root_flags_feature,
         input_param_flags_feature,
+        objc_link_flag_feature,
         force_pic_flags_feature,
         pch_feature,
         apply_default_warnings_feature,
diff --git a/test/BUILD b/test/BUILD
index d17cab0..56eaab2 100644
--- a/test/BUILD
+++ b/test/BUILD
@@ -5,6 +5,7 @@ load(":apple_support_test.bzl", "apple_support_test")
 load(":universal_binary_test.bzl", "universal_binary_test")
 load(":xcode_support_test.bzl", "xcode_support_test")
 load(":starlark_apple_binary.bzl", "starlark_apple_binary")
+load(":linking_tests.bzl", "linking_test_suite")
 
 licenses(["notice"])
 
@@ -13,6 +14,8 @@ apple_support_test(name = "apple_support_test")
 
 xcode_support_test(name = "xcode_support_test")
 
+linking_test_suite(name = "linking")
+
 # Test to ensure the environment variable contract of apple_genrule.
 sh_test(
     name = "apple_genrule_test",
diff --git a/test/fixtures/linking/BUILD b/test/fixtures/linking/BUILD
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/fixtures/linking/BUILD
diff --git a/test/linking_tests.bzl b/test/linking_tests.bzl
new file mode 100644
index 0000000..c95ce87
--- /dev/null
+++ b/test/linking_tests.bzl
@@ -0,0 +1,61 @@
+"""Tests for linking behavior."""
+
+load(
+    "//test/rules:action_command_line_test.bzl",
+    "make_action_command_line_test_rule",
+)
+
+default_test = make_action_command_line_test_rule()
+
+disable_objc_test = make_action_command_line_test_rule(
+    config_settings = {
+        "//command_line_option:features": [
+            "-objc_link_flag",
+        ],
+    },
+)
+
+def linking_test_suite(name):
+    default_test(
+        name = "{}_default_link_test".format(name),
+        tags = [name],
+        expected_argv = ["-ObjC"],
+        mnemonic = "CppLink",
+        target_under_test = "//test/test_data:cc_test_binary",
+    )
+
+    disable_objc_test(
+        name = "{}_disable_objc_link_test".format(name),
+        tags = [name],
+        not_expected_argv = ["-ObjC"],
+        mnemonic = "CppLink",
+        target_under_test = "//test/test_data:cc_test_binary",
+    )
+
+    default_test(
+        name = "{}_default_apple_link_test".format(name),
+        tags = [name],
+        expected_argv = [
+            "-Xlinker",
+            "-objc_abi_version",
+            "-Xlinker",
+            "2",
+            "-ObjC",
+        ],
+        mnemonic = "ObjcLink",
+        target_under_test = "//test/test_data:apple_binary",
+    )
+
+    disable_objc_test(
+        name = "{}_disable_objc_apple_link_test".format(name),
+        tags = [name],
+        expected_argv = [
+            "-Xlinker",
+            "-objc_abi_version",
+            "-Xlinker",
+            "2",
+        ],
+        not_expected_argv = ["-ObjC"],
+        mnemonic = "ObjcLink",
+        target_under_test = "//test/test_data:apple_binary",
+    )
diff --git a/test/rules/BUILD b/test/rules/BUILD
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/rules/BUILD
diff --git a/test/rules/action_command_line_test.bzl b/test/rules/action_command_line_test.bzl
new file mode 100644
index 0000000..83852cd
--- /dev/null
+++ b/test/rules/action_command_line_test.bzl
@@ -0,0 +1,149 @@
+# Copyright 2020 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Rules for testing the contents of action command lines."""
+
+load("@bazel_skylib//lib:collections.bzl", "collections")
+load("@bazel_skylib//lib:unittest.bzl", "analysistest", "unittest")
+
+def _action_command_line_test_impl(ctx):
+    env = analysistest.begin(ctx)
+    target_under_test = analysistest.target_under_test(env)
+
+    # Find the desired action and verify that there is exactly one.
+    actions = analysistest.target_actions(env)
+    mnemonic = ctx.attr.mnemonic
+    matching_actions = [
+        action
+        for action in actions
+        if action.mnemonic == mnemonic
+    ]
+    if not matching_actions:
+        actual_mnemonics = collections.uniq(
+            [action.mnemonic for action in actions],
+        )
+        unittest.fail(
+            env,
+            ("Target '{}' registered no actions with the mnemonic '{}' " +
+             "(it had {}).").format(
+                str(target_under_test.label),
+                mnemonic,
+                actual_mnemonics,
+            ),
+        )
+        return analysistest.end(env)
+    if len(matching_actions) != 1:
+        # This is a hack to avoid CppLink meaning binary linking and static
+        # library archiving https://github.com/bazelbuild/bazel/pull/15060
+        if mnemonic == "CppLink" and len(matching_actions) == 2:
+            matching_actions = [
+                action
+                for action in matching_actions
+                if action.argv[0] not in [
+                    "/usr/bin/ar",
+                    "external/local_config_cc/libtool",
+                    "external/local_config_apple_cc/libtool",
+                ]
+            ]
+        if len(matching_actions) != 1:
+            unittest.fail(
+                env,
+                ("Expected exactly one action with the mnemonic '{}', " +
+                 "but found {}.").format(
+                    mnemonic,
+                    len(matching_actions),
+                ),
+            )
+            return analysistest.end(env)
+
+    action = matching_actions[0]
+    message_prefix = "In {} action for target '{}', ".format(
+        mnemonic,
+        str(target_under_test.label),
+    )
+
+    # Concatenate the arguments into a single string so that we can easily look
+    # for subsequences of arguments. Note that we append an extra space to the
+    # end and look for arguments followed by a trailing space so that having
+    # `-foo` in the expected list doesn't match `-foobar`, for example.
+    concatenated_args = " ".join(action.argv) + " "
+    bin_dir = analysistest.target_bin_dir_path(env)
+    for expected in ctx.attr.expected_argv:
+        expected = expected.replace("$(BIN_DIR)", bin_dir).replace("$(WORKSPACE_NAME)", ctx.workspace_name)
+        if expected + " " not in concatenated_args and expected + "=" not in concatenated_args:
+            unittest.fail(
+                env,
+                "{}expected argv to contain '{}', but it did not: {}".format(
+                    message_prefix,
+                    expected,
+                    concatenated_args,
+                ),
+            )
+    for not_expected in ctx.attr.not_expected_argv:
+        not_expected = not_expected.replace("$(BIN_DIR)", bin_dir).replace("$(WORKSPACE_NAME)", ctx.workspace_name)
+        if not_expected + " " in concatenated_args or not_expected + "=" in concatenated_args:
+            unittest.fail(
+                env,
+                "{}expected argv to not contain '{}', but it did: {}".format(
+                    message_prefix,
+                    not_expected,
+                    concatenated_args,
+                ),
+            )
+
+    return analysistest.end(env)
+
+def make_action_command_line_test_rule(config_settings = {}):
+    """Returns a new `action_command_line_test`-like rule with custom configs.
+
+    Args:
+        config_settings: A dictionary of configuration settings and their values
+            that should be applied during tests.
+
+    Returns:
+        A rule returned by `analysistest.make` that has the
+        `action_command_line_test` interface and the given config settings.
+    """
+    return analysistest.make(
+        _action_command_line_test_impl,
+        attrs = {
+            "expected_argv": attr.string_list(
+                mandatory = False,
+                doc = """\
+A list of strings representing substrings expected to appear in the action
+command line, after concatenating all command line arguments into a single
+space-delimited string.
+""",
+            ),
+            "not_expected_argv": attr.string_list(
+                mandatory = False,
+                doc = """\
+A list of strings representing substrings expected not to appear in the action
+command line, after concatenating all command line arguments into a single
+space-delimited string.
+""",
+            ),
+            "mnemonic": attr.string(
+                mandatory = True,
+                doc = """\
+The mnemonic of the action to be inspected on the target under test. It is
+expected that there will be exactly one of these.
+""",
+            ),
+        },
+        config_settings = config_settings,
+    )
+
+# A default instantiation of the rule when no custom config settings are needed.
+action_command_line_test = make_action_command_line_test_rule()
diff --git a/test/test_data/BUILD b/test/test_data/BUILD
index 4864f5b..bdc4b86 100644
--- a/test/test_data/BUILD
+++ b/test/test_data/BUILD
@@ -2,6 +2,7 @@ load(
     "//rules:universal_binary.bzl",
     "universal_binary",
 )
+load("//test:starlark_apple_binary.bzl", "starlark_apple_binary")
 
 package(
     default_testonly = 1,
@@ -19,8 +20,20 @@ cc_binary(
     tags = TARGETS_UNDER_TEST_TAGS,
 )
 
+cc_library(
+    name = "lib",
+    srcs = ["main.cc"],
+    tags = TARGETS_UNDER_TEST_TAGS,
+)
+
 universal_binary(
     name = "multi_arch_cc_binary",
     binary = ":cc_test_binary",
     tags = TARGETS_UNDER_TEST_TAGS,
 )
+
+starlark_apple_binary(
+    name = "apple_binary",
+    tags = TARGETS_UNDER_TEST_TAGS,
+    deps = [":lib"],
+)