pw_toolchain_bazel: Use llvm-libtool-darwin on macOS

Fixes Pigweed's compile helper to be compatible with llvm-libtool-darwin. Fixes: b/297924955 Bug: b/297413805 Change-Id: If6a864671ca39d3e90e8693750c9dbc17ad7b01b Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/190896 Pigweed-Auto-Submit: Armando Montanez <[email protected]> Commit-Queue: Auto-Submit <[email protected]> Presubmit-Verified: CQ Bot Account <[email protected]> Reviewed-by: Ted Pudlik <[email protected]> Reviewed-by: Matt Stark <[email protected]>
google · Feb 12, 2024 · 9909bd4 · 9909bd4
1 parent 6b4f24a
commit 9909bd4
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 111 deletions.
diff --git a/pw_build/bazel_internal/pigweed_internal.bzl b/pw_build/bazel_internal/pigweed_internal.bzl
@@ -40,6 +40,13 @@ print_platform = aspect(
     """,
 )
 
+# TODO: https://github.com/bazelbuild/bazel/issues/16546 - Use
+# cc_helper.is_compilation_outputs_empty() if/when it's available for
+# end-users.
+def _is_compilation_outputs_empty(compilation_outputs):
+    return (len(compilation_outputs.pic_objects) == 0 and
+            len(compilation_outputs.objects) == 0)
+
 def compile_cc(
         ctx,
         srcs,
@@ -88,25 +95,35 @@ def compile_cc(
     )
 
     linking_contexts = [dep[CcInfo].linking_context for dep in deps]
-    linking_context, _ = cc_common.create_linking_context_from_compilation_outputs(
-        actions = ctx.actions,
-        feature_configuration = feature_configuration,
-        cc_toolchain = cc_toolchain,
-        compilation_outputs = compilation_outputs,
-        linking_contexts = linking_contexts,
-        disallow_dynamic_library = True,
-        name = ctx.label.name,
-    )
 
-    transitive_output_files = [dep[DefaultInfo].files for dep in deps]
-    output_files = depset(
-        compilation_outputs.pic_objects + compilation_outputs.objects,
-        transitive = transitive_output_files,
-    )
-    return [DefaultInfo(files = output_files), CcInfo(
-        compilation_context = compilation_context,
-        linking_context = linking_context,
-    )]
+    # If there's no compiled artifacts (i.e. the library is header-only), don't
+    # try and link a library.
+    #
+    # TODO: https://github.com/bazelbuild/bazel/issues/18095 - Remove this
+    # if create_linking_context_from_compilation_outputs() is changed to no
+    # longer require this workaround.
+    if not _is_compilation_outputs_empty(compilation_outputs):
+        linking_context, link_outputs = cc_common.create_linking_context_from_compilation_outputs(
+            actions = ctx.actions,
+            feature_configuration = feature_configuration,
+            cc_toolchain = cc_toolchain,
+            compilation_outputs = compilation_outputs,
+            linking_contexts = linking_contexts,
+            disallow_dynamic_library = True,
+            name = ctx.label.name,
+        )
+
+        if link_outputs.library_to_link != None:
+            linking_contexts.append(linking_context)
+
+    return [
+        CcInfo(
+            compilation_context = compilation_context,
+            linking_context = cc_common.merge_linking_contexts(
+                linking_contexts = linking_contexts,
+            ),
+        ),
+    ]
 
 # From cc_helper.bzl. Feature names for static/dynamic linking.
 linker_mode = struct(

diff --git a/pw_build/pigweed.bzl b/pw_build/pigweed.bzl
@@ -284,7 +284,7 @@ pw_cc_blob_library = rule(
 )
 
 def _pw_cc_binary_with_map_impl(ctx):
-    [_, cc_info] = _compile_cc(
+    [cc_info] = _compile_cc(
         ctx,
         ctx.files.srcs,
         [],

diff --git a/pw_toolchain/host_clang/BUILD.bazel b/pw_toolchain/host_clang/BUILD.bazel
@@ -46,6 +46,12 @@ pw_cc_flag_set(
     target_compatible_with = ["@platforms//os:linux"],
 )
 
+pw_cc_flag_set(
+    name = "libtool_darwin_flags",
+    actions = ["@pw_toolchain//actions:cpp_link_static_library"],
+    flags = ["-no_warning_for_no_symbols"],
+)
+
 # Although we use similar warnings for clang and arm_gcc, we don't have one
 # centralized list, since we might want to use different warnings based on the
 # compiler in the future.
@@ -132,6 +138,7 @@ pw_cc_toolchain(
         "@platforms//os:macos": [
             "@pw_toolchain//flag_sets:no_default_cpp_stdlib",
             ":macos_stdlib",
+            ":libtool_darwin_flags",
         ],
         "@platforms//os:linux": [
             ":linux_link_libs",

diff --git a/pw_toolchain_bazel/build_external/gcc_arm_none_eabi.BUILD b/pw_toolchain_bazel/build_external/gcc_arm_none_eabi.BUILD
@@ -43,6 +43,10 @@ pw_cc_tool(
 pw_cc_action_config(
     name = "arm-none-eabi-ar",
     action_names = ["@pw_toolchain//actions:all_ar_actions"],
+    # Unlike most legacy features required to compile code, these features
+    # aren't enabled by default, and are instead only implied by the built-in
+    # action configs. We imply the features here to match the behavior of the
+    # built-in action configs so flags are actually passed to `ar`.
     implies = [
         "@pw_toolchain//features/legacy:archiver_flags",
         "@pw_toolchain//features/legacy:linker_param_file",

diff --git a/pw_toolchain_bazel/build_external/llvm_clang.BUILD b/pw_toolchain_bazel/build_external/llvm_clang.BUILD
@@ -37,14 +37,26 @@ pw_cc_tool(
     tool = "//:bin/llvm-ar",
 )
 
+pw_cc_tool(
+    name = "llvm_libtool_darwin_tool",
+    tool = "//:bin/llvm-libtool-darwin",
+)
+
 pw_cc_action_config(
     name = "ar",
     action_names = ["@pw_toolchain//actions:all_ar_actions"],
+    # Unlike most legacy features required to compile code, these features
+    # aren't enabled by default, and are instead only implied by the built-in
+    # action configs. We imply the features here to match the behavior of the
+    # built-in action configs so flags are actually passed to `ar`.
     implies = [
         "@pw_toolchain//features/legacy:archiver_flags",
         "@pw_toolchain//features/legacy:linker_param_file",
     ],
-    tools = [":ar_tool"],
+    tools = select({
+        "@platforms//os:macos": [":llvm_libtool_darwin_tool"],
+        "//conditions:default": [":ar_tool"],
+    }),
 )
 
 pw_cc_tool(

diff --git a/pw_toolchain_bazel/cc_toolchain/private/cc_toolchain.bzl b/pw_toolchain_bazel/cc_toolchain/private/cc_toolchain.bzl
@@ -13,14 +13,6 @@
 # the License.
 """Implementation of the pw_cc_toolchain rule."""
 
-load("@bazel_tools//tools/build_defs/cc:action_names.bzl", "ACTION_NAMES")
-load(
-    "@rules_cc//cc:cc_toolchain_config_lib.bzl",
-    "feature",
-    "flag_group",
-    "flag_set",
-    "variable_with_value",
-)
 load("//features:builtin_features.bzl", "BUILTIN_FEATURES")
 load(
     ":providers.bzl",
@@ -80,86 +72,6 @@ PW_CC_TOOLCHAIN_BLOCKED_ATTRS = {
     "make_variables": "pw_cc_toolchain does not yet support make variables",
 }
 
-def _archiver_flags_feature(is_mac):
-    """Returns our implementation of the legacy archiver_flags feature.
-
-    We provide our own implementation of the archiver_flags.  The default
-    implementation of this legacy feature at
-    https://github.com/bazelbuild/bazel/blob/252d36384b8b630d77d21fac0d2c5608632aa393/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java#L620-L660
-    contains a bug that prevents it from working with llvm-libtool-darwin only
-    fixed in
-    https://github.com/bazelbuild/bazel/commit/ae7cfa59461b2c694226be689662d387e9c38427,
-    which has not yet been released.
-
-    However, we don't merely fix the bug. Part of the Pigweed build involves
-    linking some empty libraries (with no object files). This leads to invoking
-    the archiving tool with no input files. Such an invocation is considered a
-    success by llvm-ar, but not by llvm-libtool-darwin. So for now, we use
-    flags appropriate for llvm-ar here, even on MacOS.
-
-    Args:
-        is_mac: Does the toolchain this feature will be included in target MacOS?
-
-    Returns:
-        The archiver_flags feature.
-    """
-
-    # TODO: b/297413805 - Remove this implementation.
-    return feature(
-        name = "archiver_flags",
-        flag_sets = [
-            flag_set(
-                actions = [
-                    ACTION_NAMES.cpp_link_static_library,
-                ],
-                flag_groups = [
-                    flag_group(
-                        flags = _archiver_flags(is_mac),
-                    ),
-                    flag_group(
-                        expand_if_available = "output_execpath",
-                        flags = ["%{output_execpath}"],
-                    ),
-                ],
-            ),
-            flag_set(
-                actions = [
-                    ACTION_NAMES.cpp_link_static_library,
-                ],
-                flag_groups = [
-                    flag_group(
-                        expand_if_available = "libraries_to_link",
-                        iterate_over = "libraries_to_link",
-                        flag_groups = [
-                            flag_group(
-                                expand_if_equal = variable_with_value(
-                                    name = "libraries_to_link.type",
-                                    value = "object_file",
-                                ),
-                                flags = ["%{libraries_to_link.name}"],
-                            ),
-                            flag_group(
-                                expand_if_equal = variable_with_value(
-                                    name = "libraries_to_link.type",
-                                    value = "object_file_group",
-                                ),
-                                flags = ["%{libraries_to_link.object_files}"],
-                                iterate_over = "libraries_to_link.object_files",
-                            ),
-                        ],
-                    ),
-                ],
-            ),
-        ],
-    )
-
-def _archiver_flags(is_mac):
-    """Returns flags for llvm-ar."""
-    if is_mac:
-        return ["--format=darwin", "rcs"]
-    else:
-        return ["rcsD"]
-
 def _pw_cc_toolchain_config_impl(ctx):
     """Rule that provides a CcToolchainConfigInfo.
 
@@ -193,9 +105,6 @@ def _pw_cc_toolchain_config_impl(ctx):
     flag_sets = [fs[PwFlagSetInfo] for fs in ctx.attr.unconditional_flag_sets]
     out = to_untyped_config(feature_set, action_config_set, flag_sets, extra_action_files)
 
-    # TODO: b/297413805 - This could be externalized.
-    out.features.append(_archiver_flags_feature(ctx.attr.target_libc == "macosx"))
-
     extra = []
     return [
         cc_common.create_cc_toolchain_config_info(