Use platforms inside Apple rules transitions

[comius]

This makes C++ toolchain selection work without using platform mapping for Apple rules.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[comius]

The Bazel crash in CI was fixed in bazelbuild/bazel@e3e2448

Caused by: java.lang.IllegalStateException: Optional.get() cannot be called on an absent value
	at com.google.common.base.Absent.get(Absent.java:44)
	at com.google.devtools.build.lib.rules.objc.MultiArchBinarySupport.getSplitTargetTripletFromCtads(MultiArchBinarySupport.java:620)

[comius]

For the second problem on CI, I did some investigation, but I don't have a machine (Mac/C++toolchain/swiftc) where I could test it.

It seems as a problem in transitions/flags used in the tests. (Same tests are passing internally, but I don't see where's the difference).

ERROR: file 'test/starlark_tests/targets_under_test/apple/static_library/example_library_oldest_supported_ios-x86_64-apple-ios11.0-simulator-fl.a' is generated by these conflicting actions:
Label: //test/starlark_tests/targets_under_test/apple/static_library:example_library_oldest_supported_ios

[brentleyjones]

We can't take this. We can't assume what platforms the user has set. They might have different remote execution platform properties. Or other things. So we need to use platform mapping until proper multi-platform APIs are available for us to transition to.

[brentleyjones]

Some context:

• Multiple top-level platforms? bazel#18209 (comment)
• Multiple top-level platforms? bazel#18209 (comment)
• Multiple top-level platforms? bazel#18209 (reply in thread)

[brentleyjones]

cc @katre

[keith]

would it be possible for this to work alongside platform_mappings? as in if the user wanted to use custom platforms their platform_mappings would take precedence over this?

[keith]

The Bazel crash in CI was fixed in bazelbuild/bazel@e3e2448

was this backported to 6.x? seems like otherwise we might not be able to take this?

[keith]

I'm not sure on that second issue but I suppose it's just something around us not being fully compatible with platforms.

[comius]

Hey @keith, this is a replacement for platform_mappings. Caveat, is that you need to specify --platforms on the command line, whenever you passed --cpu flag.

What can be helpful here is to define standard build in .bazelrc. For example:

build:darwin_common --crosstool_top=//tools/osx/crosstool:crosstool
build:darwin_common --host_crosstool_top=//tools/cpp:toolchain
build:darwin_common --apple_platform_type=macos

# Flags for --config=darwin_x86_64
build:darwin_x86_64 --config=darwin_common
build:darwin_x86_64 --cpu=darwin_x86_64
build:darwin_x86_64 --platforms=@apple_support//platforms:darwin_x86_64

I've made this change, because platform_mappings have sometimes weird behaviours. Mapping can be applied twice, to plaforms and back. So if the mapping is not bijective, you get flag changes that are hard to explain.

Your platform_mapping is good, and can support C++ toolchains. (I'm planning to flip that on by default soon, and now I have the last fix, so that also rules_apple are not broken).

[keith]

hrm but I guess I assume if this was 1:1 to what we had before, that it wouldn't be failing here? we're not explicitly passing platforms or CPUs today, so that should continue to work I assume?

[comius]

hrm but I guess I assume if this was 1:1 to what we had before, that it wouldn't be failing here? we're not explicitly passing platforms or CPUs today, so that should continue to work I assume?

I spent quite some time investigating this, but haven't found a fix. Findings are:

Simplest reproduction:
bazel test //test/starlark_tests:ios_static_framework_swift_x86_64_builds_using_ios_multi_cpus //test/starlark_tests:ios_static_framework_swift_x86_64_builds_using_apple_platforms

The tests won't fail running one at a time.

The conflict happens in //test/starlark_tests/targets_under_test/ios:swift_ios_static_framework.

Notice there are different ST hashes in the config. If you grep bazel config for them and compare the configurations you get:

bazel config 0a5545 3a083b
INFO: Displaying diff between configs 0a5545 and 3a083b
Displaying diff between configs 0a5545 and 3a083b
FragmentOptions com.google.devtools.build.lib.analysis.PlatformOptions {
  platforms: [@build_bazel_apple_support//platforms:ios_x86_64], [@build_bazel_apple_support//platforms:ios_sim_arm64]
}
FragmentOptions com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions {
  apple_platforms: [@build_bazel_apple_support//platforms:ios_x86_64, @build_bazel_apple_support//platforms:ios_sim_arm64], [@build_bazel_apple_support//platforms:ios_sim_arm64, @build_bazel_apple_support//platforms:ios_x86_64]
}

Different orders of apple_platforms seemed suspicious, but I can't find anything wrong with that.

The cause seems more likely to be in ios_static_framework rule. Maybe there's a missing transition for one of the outputs? Or there are too many?

Comparing google vs main branch, in ios_static_framework the difference is

 rule_attrs.common_bundle_attrs(
+            deps_cfg = transition_support.apple_platform_split_transition,
        ),

I couldn't locate how this difference happend or why.

[keith]

that diff fixes a bug that resources aren't affected by the expected transition

rules_apple/apple/internal/rule_attrs.bzl

Line 602 in 11471c5

cfg = deps_cfg,

you can remove that and it doesn't affect this issue.

I tried removing apple_platforms from the transition output, and the ST hashes are more similar, but it still hits the issue with just this diff:

FragmentOptions com.google.devtools.build.lib.analysis.PlatformOptions {
  platforms: [@build_bazel_apple_support//platforms:ios_sim_arm64], [@build_bazel_apple_support//platforms:ios_x86_64]
}

Now that I look at the specific test targets I think I'm not surprised by this, because it's building the same target with 2 different transitions. The default here:

rules_apple/test/starlark_tests/ios_static_framework_tests.bzl

Lines 57 to 59 in 11471c5

	cpus = {
	"ios_multi_cpus": ["x86_64", "sim_arm64"],
	},

and forcing platforms here:

rules_apple/test/starlark_tests/ios_static_framework_tests.bzl

Lines 97 to 100 in 11471c5

	apple_platforms = [
	"@build_bazel_apple_support//platforms:ios_sim_arm64",
	"@build_bazel_apple_support//platforms:ios_x86_64",
	],

[brentleyjones]

If we can get CI passing, I have an idea on how to land a version of this PR that supports custom platforms as well. Also, this change is probably needed to unblock rules_apple at Bazel HEAD, right?

[comius]

If we can get CI passing, I have an idea on how to land a version of this PR that supports custom platforms as well. Also, this change is probably needed to unblock rules_apple at Bazel HEAD, right?

No, it's not needed. We had Apple rules working with:

1. platform mappings
2. no platform mappings + this PR

[brentleyjones]

Okay, so strike that part of the need for it. Either way, once this is passing CI I plan to add a way to customize the platforms, so people can still use custom platforms without the platform mapping file.

[brentleyjones]

@keith Same as the other one, once this is green, we can merge. I'll get the customization change in after.

[keith]

@comius any idea how we could potentially work around this crash with bazel 6.x?

[keith]

I merged #2256 which hacks around 6.x and only applies this new behavior for 7.x, but I'd love to hear any workaround ideas to improve that and be able to drop platform_mappings for 6.x as well

[keith]

closing since i merged that other one, still open to bazel 6.x compat ideas though!