bazelbuild / rules_apple

Bazel rules to build apps for Apple platforms.
Apache License 2.0
511 stars 268 forks source link

Platforms aren't propagated in transitions #1650

Open gferon opened 2 years ago

gferon commented 2 years ago

We have a mixed language build and spent a bit of time trying to understand why platforms aren't propagated (well, ignored) when building a ios_application target, but worked fine when building a swift_library one (the core library which the app depends on). When building the ios_application target, --platforms are ignored and the build fails with symbols from mixed platforms (some built with the host target and the final linking phase with the right one).

After a bit of investigating, we realized that adding the //:command_line_options:platforms to the transition outputs and setting it to the platforms declared in @build_bazel_apple_support//platforms fixes our issue.

When using only --platforms with the transition platform_mappings API, this works exactly as intended. A bit more logic change was involved to fix apple_xcframework when using --platforms which we should probably send a patch for separately.

I think the patch is rather straightforward, but I have a bunch of questions before I open a PR to discuss it further:

 apple/internal/transition_support.bzl | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/apple/internal/transition_support.bzl b/apple/internal/transition_support.bzl
index 4f841f76..f651e034 100644
--- a/apple/internal/transition_support.bzl
+++ b/apple/internal/transition_support.bzl
@@ -126,16 +126,23 @@ def _command_line_options(
         A dictionary of `"//command_line_option"`s defined for the current target.
     """

+    # Derive platform label defined in @build_bazel_apple_support//platforms.
+    #
+    # Doing this transition allows to set number of constrains defined in @build_bazel_apple_support
+    # used by other rules (e.g. rules_rust).
+    cpu_string = _cpu_string(
+        cpu = cpu,
+        platform_type = platform_type,
+        settings = settings,
+    )
+
     output_dictionary = {
         "//command_line_option:apple configuration distinguisher": "applebin_" + platform_type,
         "//command_line_option:apple_platform_type": platform_type,
         "//command_line_option:apple_split_cpu": cpu if cpu else "",
         "//command_line_option:compiler": settings["//command_line_option:apple_compiler"],
-        "//command_line_option:cpu": _cpu_string(
-            cpu = cpu,
-            platform_type = platform_type,
-            settings = settings,
-        ),
+        "//command_line_option:cpu": cpu_string,
+        "//command_line_option:platforms": "@build_bazel_apple_support//platforms:{}".format(cpu_string),
         "//command_line_option:crosstool_top": (
             settings["//command_line_option:apple_crosstool_top"]
         ),
@@ -300,6 +307,7 @@ _apple_rule_base_transition_outputs = [
     "//command_line_option:macos_minimum_os",
     "//command_line_option:tvos_minimum_os",
     "//command_line_option:watchos_minimum_os",
+    "//command_line_option:platforms",
 ]
 _apple_universal_binary_rule_transition_outputs = _apple_rule_base_transition_outputs + [
     "//command_line_option:ios_multi_cpus",
@@ -415,7 +423,6 @@ _xcframework_transition = transition(
     inputs = _apple_rule_common_transition_inputs,
     outputs = _apple_rule_base_transition_outputs + [
         "@build_bazel_rules_swift//swift:emit_swiftinterface",
-        "//command_line_option:platforms",
     ],
 )

-- 
2.32.1 (Apple Git-133)

cc @boxdot

keith commented 2 years ago

Up to this point the apple rules don't support platforms in general, which is why this case isn't covered. it likely works with swift_library directly because there are no custom transitions inherently with that rule, so bazel handles it.

I'm not sure what other implications adding this to the transition would have, part of the issue today is we have to satisfy bazel internals with some of the apple specific attributes you see in this transition, so I'm not sure what adding platforms to this would break.

Would you still accept such contribution on the bazel/5.x branch

yep sure, we just need to land in master first

Do you know if it is expected behaviour that Bazel completely ignores any invalid values of --platforms= if a target doesn't make use of them/apply transitions based on this flag. This sounds like a bug to me.

I'm not sure, does sound surprising

gferon commented 2 years ago

Thanks for the quick and detailed answer! The only problem I have right now is that master already has something that could work for us, but won't work with Bazel v5.3 because it transitions on --incompatible_enable_apple_toolchain_resolution.

keith commented 2 years ago

You can submit directly to the 5.x branch if absolutely necessary but the problem is if you don't submit to master first we might miss your fix once you do want to upgrade