Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update DEFAULT_IOS_CPU for M1 arm64 simulator support #13873

Conversation

keith
Copy link
Member

@keith keith commented Aug 19, 2021

Now that bazel supports ios_sim_arm64 we can prefer this if no other iOS
CPU is passed so that developers can build the simulator builds of their
apps / tests without having to pass a flag depending on the host.

Now that bazel supports ios_sim_arm64 we can prefer this if no other iOS
CPU is passed so that developers can build the simulator builds of their
apps / tests without having to pass a flag depending on the host.
@keith keith requested a review from lberki as a code owner August 19, 2021 00:27
@google-cla google-cla bot added the cla: yes label Aug 19, 2021
@keith
Copy link
Member Author

keith commented Aug 19, 2021

Depends on #13870 to actually work but I wanted to discuss separately

@keith
Copy link
Member Author

keith commented Aug 19, 2021

My big open question here is how will platforms work for defaults like this? It would be ideally if they matched this behavior, but if not maybe we shouldn't merge this just to regress later

@keith keith marked this pull request as draft August 19, 2021 03:32
@keith keith marked this pull request as ready for review August 19, 2021 04:12
@keith
Copy link
Member Author

keith commented Aug 19, 2021

So I think this change is good, but I'm not actually sure what builds read this value. It seems like this on its own does nothing for our build (but potentially that's just because we're not using many rules from bazel core) and ours is falling through to https://github.com/bazelbuild/rules_apple/blob/dd47da8154045255e2fa19ef004a518efd2304a7/apple/internal/transition_support.bzl#L43 which will have to be dealt with separately somehow

@lberki lberki requested review from comius and removed request for lberki August 19, 2021 06:35
@lberki
Copy link
Contributor

lberki commented Aug 19, 2021

/cc @allevato

@allevato
Copy link
Member

Deferring to @kaylathar for anything involving M1 support

@kaylathar
Copy link
Member

kaylathar commented Aug 19, 2021

This changes a default that could cause breakages for folks that have build fleets made up of heterogenous systems. While I think this is probably going to resolve local build problems, it is likely going to cause regressions/breaks for some folks. Given this I would be hesitant to move forward at this point, and suggest not merging this for now.

For platforms, you will actually specify a target platform or set of platforms, and the various parameters will be resolved from this, and then that will be used in toolchain selection via the constraints on what toolchains can support which parameters. E.g. if you select simulator:arm64 you would end up with something specifying a CPU constraint of arm64, with a target environment of simulator.

Insofar as how this specific PR will interact with platforms, it will depend on some details on how we move in that direction, but I imagine the new 1:* transition will set the target platform from the target platform set provided via the new option created in a62f78f - so if you fail to pass a set of target platforms, we'd fall back to the default platform, which would set this. Given this, what we change this to right now does not seem as important, as the eventual platforms solution will instead have a default target platform that ends up superseding/replacing the default CPU.

@keith
Copy link
Member Author

keith commented Aug 19, 2021

made up of heterogenous systems

are you saying folks who already have M1 machines in their systems and want to maintain building for Intel by default? I think if folks aren't passing the archs they expect today, and are hoping it builds for a different arch than the host, while true in the past since bazel didn't support that configuration, is unexpected in general.

I would recommend folks specify the expected arch the expect to build, and then we still flip the default for the common case, which I think will be more inline with what people expect. Also since this would be able to make it in to 5.x where technically the breakage would be allowed AFAIUI?

Sounds like for platforms a similar change will make sense, and the default can be simulator + host arch, so doing this now would still pave the way to that behavior change later.

@kaylathar
Copy link
Member

I am not against that change in principle but would require internal changes before landing as this behavior is relied on in some places.

@brentleyjones
Copy link
Contributor

Hi @kaylathar can we get this in before the 5.0 cut?

@brentleyjones brentleyjones mentioned this pull request Oct 19, 2021
9 tasks
@kaylathar
Copy link
Member

This LGTM for merge.

@brentleyjones
Copy link
Contributor

Thanks! @oquenchil can you import it?

@kaylathar
Copy link
Member

@susinmotion Mind pulling this in please?

@comius
Copy link
Contributor

comius commented Oct 25, 2021

@susinmotion @kaylathar I pulled it in.

@keith
Copy link
Member Author

keith commented Oct 25, 2021

@comius thanks, I don't see it on master yet, is something stuck?

@bazel-io bazel-io closed this in d7628e1 Oct 26, 2021
@keith keith deleted the ks/update-default_ios_cpu-for-m1-arm64-simulator-support branch October 26, 2021 19:10
keith added a commit to keith/bazel that referenced this pull request Oct 27, 2021
Now that bazel supports ios_sim_arm64 we can prefer this if no other iOS
CPU is passed so that developers can build the simulator builds of their
apps / tests without having to pass a flag depending on the host.

Closes bazelbuild#13873.

PiperOrigin-RevId: 405661296
(cherry picked from commit d7628e1)
Wyverald pushed a commit that referenced this pull request Oct 28, 2021
Now that bazel supports ios_sim_arm64 we can prefer this if no other iOS
CPU is passed so that developers can build the simulator builds of their
apps / tests without having to pass a flag depending on the host.

Closes #13873.

PiperOrigin-RevId: 405661296
(cherry picked from commit d7628e1)
chiragramani pushed a commit to uber-common/bazel that referenced this pull request Feb 2, 2022
Now that bazel supports ios_sim_arm64 we can prefer this if no other iOS
CPU is passed so that developers can build the simulator builds of their
apps / tests without having to pass a flag depending on the host.

Closes bazelbuild#13873.

PiperOrigin-RevId: 405661296
(cherry picked from commit d7628e1)
keith added a commit to keith/bazel that referenced this pull request Feb 14, 2022
Since M1 support was added we started defaulting the CPU for macOS and
iOS builds to the host CPU in the case another CPU wasn't passed. This
change mirrors bazelbuild#13873,
bazelbuild#13440 and
https://github.com/bazelbuild/rules_apple//commit/99e5b631bf060358241a8eaabd285be5c120490f
in the newer starlark transition.

Without this you end up with architecture mismatches for builds. Fixes bazelbuild#14803
bazel-io pushed a commit that referenced this pull request Mar 1, 2022
Since M1 support was added we started defaulting the CPU for macOS and
iOS builds to the host CPU in the case another CPU wasn't passed. This
change mirrors #13873, #13440 and https://github.com/bazelbuild/rules_apple//commit/99e5b631bf060358241a8eaabd285be5c120490f
in the newer starlark transition.

Without this you end up with architecture mismatches for builds. Fixes #14803

Closes #14816.

PiperOrigin-RevId: 431641312
brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Mar 1, 2022
Since M1 support was added we started defaulting the CPU for macOS and
iOS builds to the host CPU in the case another CPU wasn't passed. This
change mirrors bazelbuild#13873, bazelbuild#13440 and https://github.com/bazelbuild/rules_apple//commit/99e5b631bf060358241a8eaabd285be5c120490f
in the newer starlark transition.

Without this you end up with architecture mismatches for builds. Fixes bazelbuild#14803

Closes bazelbuild#14816.

PiperOrigin-RevId: 431641312
(cherry picked from commit 1ec1068)
Wyverald pushed a commit that referenced this pull request Mar 2, 2022
Since M1 support was added we started defaulting the CPU for macOS and
iOS builds to the host CPU in the case another CPU wasn't passed. This
change mirrors #13873, #13440 and https://github.com/bazelbuild/rules_apple//commit/99e5b631bf060358241a8eaabd285be5c120490f
in the newer starlark transition.

Without this you end up with architecture mismatches for builds. Fixes #14803

Closes #14816.

PiperOrigin-RevId: 431641312
(cherry picked from commit 1ec1068)

Co-authored-by: Keith Smiley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants