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

[apple] support tvos_sim_arm64 in toolchain #14439

Closed
wants to merge 2 commits into from

Conversation

dflems
Copy link
Contributor

@dflems dflems commented Dec 15, 2021

No description provided.

@dflems dflems requested a review from lberki as a code owner December 15, 2021 22:55
@thii
Copy link
Member

thii commented Dec 15, 2021

This needs to be updated too

switch (applePlatformType) {
case IOS:
{
String cpu = Iterables.getFirst(appleCpus.iosMultiCpus(), appleCpus.iosCpu());
if (removeSimPrefix && cpu.startsWith(IOS_FORCED_SIMULATOR_CPU_PREFIX)) {
cpu = cpu.substring(IOS_FORCED_SIMULATOR_CPU_PREFIX.length());
}
return cpu;
}
case WATCHOS:
return appleCpus.watchosCpus().get(0);
case TVOS:
return appleCpus.tvosCpus().get(0);

@keith
Copy link
Member

keith commented Dec 15, 2021

ah yea be sure to grep for all IOS_FORCED_SIMULATOR_CPU_PREFIX and ios_sim in general

@brentleyjones
Copy link
Contributor

@Wyverald This would be really nice to get into 5.0 if we are creating another RC (which is looks like we might be?).

@dflems
Copy link
Contributor Author

dflems commented Dec 16, 2021

Updated! Now it just checks for the prefix on all CPUs and chops it off if removeSimPrefix is true

@brentleyjones
Copy link
Contributor

@kaylathar Can we get this reviewed and imported? I think this should be included in 5.0.

cc: @Wyverald.

@brentleyjones brentleyjones mentioned this pull request Jan 3, 2022
9 tasks
@Wyverald
Copy link
Member

Wyverald commented Jan 4, 2022

Unless there's a strong argument that this is crucial, I'm reluctant to cherrypick any non-trivial changes at this point, for fear of delaying the release even further. We can look at cherrypicking this into a 5.1 release after 5.0 gets out.

@dflems
Copy link
Contributor Author

dflems commented Jan 4, 2022

@Wyverald This would be really useful for us as we ship AppleTV and Watch apps. iOS and macOS aren't the only M1-compatible platforms. For the watch especially, we've been having problems running it under rosetta.

@brentleyjones
Copy link
Contributor

brentleyjones commented Jan 4, 2022

This would fall under OS/Xcode support in my mind, which is a key feature of the LTS releases.

@kaylathar
Copy link
Member

I have concerns on some portions of this change, in particular tvOS related change. The watchOS related change I will review more closely shortly but I have less concern on. The tvOS change adds more technical debt by adding an additional "sim" CPU as we are moving to platforms and will require internal changes in some cases as well. I will evaluate more fully later today though.

@dflems
Copy link
Contributor Author

dflems commented Jan 4, 2022

@kaylathar I can break these up into two PRs if need be. The watch integration is more pressing for us in the short term as the TV app can be built for intel and run under rosetta in the simulator. We're having problems doing that for the watch app.

@keith
Copy link
Member

keith commented Jan 4, 2022

It seems to me that this tech debt is pretty localized, and users don't have another option if they need to build for this simulator in the meantime. So can we land that piece as well knowing it's something we'll have to cleanup when the ideal way to implement this lands? I think the worst case scenario here is the ideal solution continues to lag and folks are blocked in the meantime

bazel-io pushed a commit that referenced this pull request Jan 7, 2022
The watch-only changes from #14439

Closes #14512.

PiperOrigin-RevId: 420296580
brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Jan 7, 2022
The watch-only changes from bazelbuild#14439

Closes bazelbuild#14512.

PiperOrigin-RevId: 420296580
(cherry picked from commit b341802)
Wyverald pushed a commit that referenced this pull request Jan 8, 2022
The watch-only changes from #14439

Closes #14512.

PiperOrigin-RevId: 420296580
(cherry picked from commit b341802)

Co-authored-by: Dan Fleming <dflems@spotify.com>
@gregestren gregestren self-requested a review January 21, 2022 22:46
@gregestren
Copy link
Contributor

gregestren commented Feb 2, 2022

@kaylathar is there any reference material coming up on how the architectures map differently for platforms? For example, what about sim adds more complication than adding another non-sim CPU? Or for that matter how is the concept of sim expressed in the new API?

I'm interested both to understand your intentions but also to support the possibility of collaborative improvements. i.e. if you're working with a fixed assumption of what the platforms API can support, possibly at the expense of a clean Apple API, my team might be able to find optimizations to support cleaner Apple APIs. I understand Apple rules particularly struggle with their complex configuration needs.

@dflems dflems changed the title [apple] support tvos_sim_arm64 and watchos_arm64 in toolchain [apple] support tvos_sim_arm64 in toolchain Feb 2, 2022
@dflems
Copy link
Contributor Author

dflems commented Feb 2, 2022

@kaylathar Updated this old PR to fix the conflicts. Still think tvos_sim_arm64 is a necessary evil until we have proper platform support. Otherwise it's impossible to build for the native tvOS simulator on M1 macs.

@kaylathar
Copy link
Member

LGTM - I'm fine landing this change given the time it's going to take to get platforms functional.

@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 7, 2022
@Wyverald
Copy link
Member

Wyverald commented Feb 9, 2022

@bazel-io fork 5.1

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 9, 2022
@bazel-io bazel-io closed this in 207c31b Feb 10, 2022
brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Feb 10, 2022
Closes bazelbuild#14439.

PiperOrigin-RevId: 427721738
(cherry picked from commit 207c31b)
Wyverald pushed a commit that referenced this pull request Feb 10, 2022
Closes #14439.

PiperOrigin-RevId: 427721738
(cherry picked from commit 207c31b)

Co-authored-by: Dan Fleming <dflems@spotify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants