From 9909bd4146807564e6a8a48cd316d7fa62ed6e33 Mon Sep 17 00:00:00 2001 From: Armando Montanez Date: Mon, 12 Feb 2024 19:52:43 +0000 Subject: [PATCH] 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 Commit-Queue: Auto-Submit Presubmit-Verified: CQ Bot Account Reviewed-by: Ted Pudlik Reviewed-by: Matt Stark --- pw_build/bazel_internal/pigweed_internal.bzl | 53 +++++++---- pw_build/pigweed.bzl | 2 +- pw_toolchain/host_clang/BUILD.bazel | 7 ++ .../build_external/gcc_arm_none_eabi.BUILD | 4 + .../build_external/llvm_clang.BUILD | 14 ++- .../cc_toolchain/private/cc_toolchain.bzl | 91 ------------------- 6 files changed, 60 insertions(+), 111 deletions(-) diff --git a/pw_build/bazel_internal/pigweed_internal.bzl b/pw_build/bazel_internal/pigweed_internal.bzl index be3756c778..0c49114a1d 100644 --- 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 index 49b23727ae..ae621eca60 100644 --- 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 index 75777f1fd4..7c3e934410 100644 --- 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 index 3131336053..0f4c2231a8 100644 --- 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 index 87353c3fc2..4083b3e796 100644 --- 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 index 17cc95bd6b..349dedd61c 100644 --- 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(