From 7d8769a9d5024200da8a34999e5934b23f441e90 Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Thu, 1 Dec 2022 09:20:27 -0800 Subject: [PATCH] Add back uses of `-disable-autolink-framework` for static frameworks (#941) This was removed a while back because it appeared to be unused. Since then this rules_apple change: https://github.com/bazelbuild/rules_apple/commit/8d841342c238457896cd7596cc29b2d06c9a75f0 causes it to be slightly necessary again. The situation is that we compile Swift files with the framework search paths, but then we now link by passing the static framework binary directly to the linker, as opposed to using `-F` / `-framework`, which means any `LC_LINKER_OPTION` `-framework` uses pointing to these static frameworks are invalid. Invalid `LC_LINKER_OPTION`s are ignored by ld64 _unless_ the entire link fails, but lld and mold do not implement this workaround, so it fails the link. This also potentially helps with cases of including undeclared dependencies. Ideally we could disable autolinking all together to ensure dependencies were strict, but that requires us to manually link the various Swift support dylibs. This discovers framework names based on their search paths similar to how bazel did before the rules_apple change https://github.com/bazelbuild/bazel/blob/8bd2d97fd143b4b8a6fc96ce300820e6e422c03a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java#L689-L697 We cannot revert the rules_apple change since theoretically that field or provider will be removed entirely in the future. This reverts commit b37a7173dab6a33fea89d79e60592594927b7931. --- swift/internal/compiling.bzl | 41 ++++++++++++++++++++++++ test/BUILD | 4 +-- test/fixtures/linking/fake_framework.bzl | 5 +-- test/imported_framework_tests.bzl | 34 ++++++++++++++++++++ test/linking_tests.bzl | 25 --------------- 5 files changed, 80 insertions(+), 29 deletions(-) create mode 100644 test/imported_framework_tests.bzl delete mode 100644 test/linking_tests.bzl diff --git a/swift/internal/compiling.bzl b/swift/internal/compiling.bzl index 533c9d410..781fece34 100644 --- a/swift/internal/compiling.bzl +++ b/swift/internal/compiling.bzl @@ -14,6 +14,7 @@ """Implementation of compilation logic for Swift.""" +load("@bazel_skylib//lib:collections.bzl", "collections") load("@bazel_skylib//lib:partial.bzl", "partial") load("@bazel_skylib//lib:paths.bzl", "paths") load("@bazel_skylib//lib:sets.bzl", "sets") @@ -1111,6 +1112,12 @@ def compile_action_configs( ], configurators = [_conditional_compilation_flag_configurator], ), + + # Disable auto-linking for prebuilt static frameworks. + swift_toolchain_config.action_config( + actions = [swift_action_names.COMPILE], + configurators = [_static_frameworks_disable_autolink_configurator], + ), ] # NOTE: The positions of these action configs in the list are important, @@ -1607,6 +1614,19 @@ def _framework_search_paths_configurator(prerequisites, args, is_swift): before_each = "-Xcc", ) +def _static_frameworks_disable_autolink_configurator(prerequisites, args): + """Add flags to disable auto-linking for static prebuilt frameworks. + + This disables the `LC_LINKER_OPTION` load commands for auto-linking when + importing a static framework. This is needed to avoid potential linker + errors since when linking the framework it will be passed directly as a + library. + """ + args.add_all( + prerequisites.objc_info.imported_library, + map_each = _disable_autolink_framework_copts, + ) + def _dependencies_swiftmodules_configurator(prerequisites, args): """Adds `.swiftmodule` files from deps to search paths and action inputs.""" args.add_all( @@ -3011,6 +3031,27 @@ def _swift_module_search_path_map_fn(module): else: return None +def _disable_autolink_framework_copts(library_path): + """A `map_each` helper that potentially disables autolinking for the given library. + + Args: + library_path: The path to an imported library that is potentially a static framework. + + Returns: + The list of `swiftc` flags needed to disable autolinking for the given + framework. + """ + if not library_path.dirname.endswith(".framework"): + return [] + + return collections.before_each( + "-Xfrontend", + [ + "-disable-autolink-framework", + library_path.basename, + ], + ) + def _find_num_threads_flag_value(user_compile_flags): """Finds the value of the `-num-threads` flag. diff --git a/test/BUILD b/test/BUILD index 270ba1c13..dc5c7d55b 100644 --- a/test/BUILD +++ b/test/BUILD @@ -4,7 +4,7 @@ load(":coverage_settings_tests.bzl", "coverage_settings_test_suite") load(":debug_settings_tests.bzl", "debug_settings_test_suite") load(":features_tests.bzl", "features_test_suite") load(":generated_header_tests.bzl", "generated_header_test_suite") -load(":linking_tests.bzl", "linking_test_suite") +load(":imported_framework_tests.bzl", "imported_framework_test_suite") load(":mainattr_tests.bzl", "mainattr_test_suite") load(":module_cache_settings_tests.bzl", "module_cache_settings_test_suite") load(":output_file_map_tests.bzl", "output_file_map_test_suite") @@ -27,7 +27,7 @@ features_test_suite(name = "features") generated_header_test_suite(name = "generated_header") -linking_test_suite(name = "linking") +imported_framework_test_suite(name = "imported_framework") mainattr_test_suite(name = "mainattr") diff --git a/test/fixtures/linking/fake_framework.bzl b/test/fixtures/linking/fake_framework.bzl index 4039524cf..b26dab8f6 100644 --- a/test/fixtures/linking/fake_framework.bzl +++ b/test/fixtures/linking/fake_framework.bzl @@ -1,12 +1,13 @@ """Simple rule to emulate apple_static_framework_import""" def _impl(ctx): - binary1 = ctx.actions.declare_file("framework1.framework/framework") + binary1 = ctx.actions.declare_file("framework1.framework/framework1") ctx.actions.write(binary1, "empty") - binary2 = ctx.actions.declare_file("framework2.framework/framework") + binary2 = ctx.actions.declare_file("framework2.framework/framework2") ctx.actions.write(binary2, "empty") return apple_common.new_objc_provider( static_framework_file = depset([binary1, binary2]), + imported_library = depset([binary1, binary2]), ) fake_framework = rule( diff --git a/test/imported_framework_tests.bzl b/test/imported_framework_tests.bzl new file mode 100644 index 000000000..0ecd356ac --- /dev/null +++ b/test/imported_framework_tests.bzl @@ -0,0 +1,34 @@ +"""Tests for validating linking behavior""" + +load( + "@build_bazel_rules_swift//test/rules:action_command_line_test.bzl", + "action_command_line_test", +) + +def imported_framework_test_suite(name): + action_command_line_test( + name = "{}_disable_autolink_framework_test".format(name), + expected_argv = [ + "-Xfrontend -disable-autolink-framework -Xfrontend framework1", + "-Xfrontend -disable-autolink-framework -Xfrontend framework2", + ], + mnemonic = "SwiftCompile", + tags = [name], + target_under_test = "@build_bazel_rules_swift//test/fixtures/linking:bin", + ) + + action_command_line_test( + name = "{}_duplicate_linking_args".format(name), + expected_argv = [ + "-framework framework1", + "-framework framework2", + ], + mnemonic = "CppLink", + tags = [name], + target_under_test = "@build_bazel_rules_swift//test/fixtures/linking:bin", + ) + + native.test_suite( + name = name, + tags = [name], + ) diff --git a/test/linking_tests.bzl b/test/linking_tests.bzl deleted file mode 100644 index dd380c992..000000000 --- a/test/linking_tests.bzl +++ /dev/null @@ -1,25 +0,0 @@ -"""Tests for validating linking behavior""" - -load( - "@build_bazel_rules_swift//test/rules:action_command_line_test.bzl", - "make_action_command_line_test_rule", -) - -linking_test = make_action_command_line_test_rule() - -def linking_test_suite(name): - linking_test( - name = "{}_duplicate_linking_args".format(name), - expected_argv = [ - "-framework framework1", - "-framework framework2", - ], - mnemonic = "CppLink", - tags = [name], - target_under_test = "@build_bazel_rules_swift//test/fixtures/linking:bin", - ) - - native.test_suite( - name = name, - tags = [name], - )