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

Mac Catalyst: should we have Runtime.Arch? If so, what should it return? #10312

Closed
Tracked by #44736
rolfbjarne opened this issue Dec 17, 2020 · 8 comments · Fixed by #13562
Closed
Tracked by #44736

Mac Catalyst: should we have Runtime.Arch? If so, what should it return? #10312

rolfbjarne opened this issue Dec 17, 2020 · 8 comments · Fixed by #13562
Labels
dotnet-pri3 .NET 6: wishlist for stable release estimate-4w Mac Catalyst Issues affecting Mac Catalyst request-for-comments The issue is a suggested idea seeking feedback
Milestone

Comments

@rolfbjarne
Copy link
Member

rolfbjarne commented Dec 17, 2020

Question:

  • What do we do with regards to the Runtime.Arch property for Mac Catalyst?

Possibilities:

  1. Remove it.
    Con: existing code won't compile / increases binary incompatibility with iOS.
  2. Keep it, and return:
    2.1) Simulator
    2.2) Device
    2.3) A third value (MacCatalyst?)

Time estimate (for Sebastien's proposal here): 4 weeks

@rolfbjarne rolfbjarne added iOS Issues affecting iOS request-for-comments The issue is a suggested idea seeking feedback labels Dec 17, 2020
@rolfbjarne rolfbjarne added this to the Mac Catalyst milestone Dec 17, 2020
@dalexsoto
Copy link
Member

I am thinking the solution should be between 2.2 and 2.3, 1 is definitely a no go for me, I think it would be useful to tell that we are running in the MacCatalyst context but OTOH we are running on a Device, definitely not a simulator heh

My vote would lean to 2.3, MacCatalyst

@spouliot
Copy link
Contributor

A new value, like MacCatalyst, means code changes on every call sites :(

$ git grep Runtime.Arch | wc -l
     170
$ git grep Runtime.Arch | grep SIMULATOR | wc -l
      95
$ git grep Runtime.Arch | grep DEVICE | wc -l
      68

and then it's also used inside generated binding code...

if (Runtime.Arch == Arch.DEVICE) {

but AFAIK it's always (generated) for DEVICE which in fact is more a Intel vs ARM (ABI) decision.

@rolfbjarne were you planning to change the generated code for Catalyst ?

Maybe we could add Intel and Arm with the existing (SIMULATOR and DEVICE respectively) values ?

@rolfbjarne
Copy link
Member Author

were you planning to change the generated code for Catalyst ?

It's easy enough to change the generated code for Catalyst if we wanted to. It shouldn't affect existing binding NuGets for Xamarin.iOS, because those will have to be rebuilt anyway.

A new value, like MacCatalyst, means code changes on every call sites :(

We could do something like this:

public enum Arch {
	DEVICE,
	SIMULATOR,
	MACCATALYST = SIMULATOR,
}

@spouliot
Copy link
Contributor

MACCATALYST = SIMULATOR,

and what will happen when we run this natively on a M1 CPU ?

@rolfbjarne
Copy link
Member Author

Unfortunately Arch.SIMULATOR and Arch.DEVICE has been used with different meanings, which comes back and bites us now 😒

As far as I can tell, some call sites will have to be updated no matter which solution we choose. We'll just have to pick the best poison.

@spouliot
Copy link
Contributor

Indeed, the thing is DEVICE and SIMULATOR are not Arch.

Up to this summer they were based on different architectures so that worked just fine. But it was effectively a case of badly named members (or property).

We'll just have to pick the best poison.

I'd rather not pick a poison 💀. It's ☢️ waste so let's pretend it does not exists.

I suggest we don't add MACCATALYST (of any casing variation). That would only be another step in the wrong direction - it's a dead-end.

Instead let's start a new road where we:

  • Obsolete Runtime.Arch and it's SHOOTING enum members
  • Add methods/properties to Runtime to
    • give us the ABI, minimally families (Intel vs ARM) and, if needed, down to arm64, arm64e (and I think we might already have some for stret right ?)
    • tell us if we're running under the simulator, e.g. Runtime.RunningUnderSimulator
  • update the generator to use the correct methods/properties (largely, if not all, ABI related)
  • update non-generated source code to use the correct API based on the context, i.e. don't guess the ABI by checking for simulator or device
  • update our linker Arch optimizations to map the new API

@rolfbjarne rolfbjarne added Mac Catalyst Issues affecting Mac Catalyst dotnet-pri0 .NET 6: required for stable release and removed iOS Issues affecting iOS labels Feb 9, 2021
@rolfbjarne rolfbjarne modified the milestones: Mac Catalyst, .NET 6 Feb 9, 2021
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Jul 15, 2021
…properly on ARM-based simulators.

* Use the Apple-provided TARGET_OS_SIMULATOR define to determine if we're
  running in a simulator, instead of checking the current architecture. This
  way we properly detect ARM64-based simulators (and it'll work correctly in
  the future).

* Always set Runtime.Arch = SIMULATOR for Mac Catalyst. The final value for
  Runtime.Arch for Mac Catalyst is tracked in xamarin#10312, but this is a stop-gap
  measure to make sure we have the same value between X64 and ARM64 on Mac
  Catalyst, and until now we've had Runtime.Arch = SIMULATOR for X64, so just
  go with that for now.
rolfbjarne added a commit that referenced this issue Jul 16, 2021
…properly on ARM-based simulators. (#12125)

* Use the Apple-provided TARGET_OS_SIMULATOR define to determine if we're
  running in a simulator, instead of checking the current architecture. This
  way we properly detect ARM64-based simulators (and it'll work correctly in
  the future).

* Always set Runtime.Arch = SIMULATOR for Mac Catalyst. The final value for
  Runtime.Arch for Mac Catalyst is tracked in #10312, but this is a stop-gap
  measure to make sure we have the same value between X64 and ARM64 on Mac
  Catalyst, and until now we've had Runtime.Arch = SIMULATOR for X64, so just
  go with that for now.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Jul 16, 2021
…properly on ARM-based simulators. (xamarin#12125)

* Use the Apple-provided TARGET_OS_SIMULATOR define to determine if we're
  running in a simulator, instead of checking the current architecture. This
  way we properly detect ARM64-based simulators (and it'll work correctly in
  the future).

* Always set Runtime.Arch = SIMULATOR for Mac Catalyst. The final value for
  Runtime.Arch for Mac Catalyst is tracked in xamarin#10312, but this is a stop-gap
  measure to make sure we have the same value between X64 and ARM64 on Mac
  Catalyst, and until now we've had Runtime.Arch = SIMULATOR for X64, so just
  go with that for now.
@rolfbjarne rolfbjarne added dotnet-pri3 .NET 6: wishlist for stable release and removed dotnet-pri0 .NET 6: required for stable release labels Aug 17, 2021
@rolfbjarne
Copy link
Member Author

Downgrading priority since the current solution works (although it's ugly), and we can implement this at a later stage (we're not going to add any breaking changes either now or later anyway).

@rolfbjarne
Copy link
Member Author

Downgrading priority since the current solution works (although it's ugly), and we can implement this at a later stage (we're not going to add any breaking changes either now or later anyway).

Now that we've decided to do breaking changes for .NET 6, and Mac Catalyst binaries won't be compatible with iOS binaries anymore, I think we can reconsider this and just remove the Runtime.Arch API for Mac Catalyst.

rolfbjarne added a commit that referenced this issue Dec 15, 2021
…ixes #10312. (#13562)

Remove Runtime.Arch and ObjCRuntime.Arch from Mac Catalyst, because they don't
apply for a Mac Catalyst app (which is neither a simulator environment, nor a
device environment).

This means that code using these APIs will have to be re-evaluated to
determine what's the correct behavior for Mac Catalyst.

Also update our tests accordingly.

Fixes #10312.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dotnet-pri3 .NET 6: wishlist for stable release estimate-4w Mac Catalyst Issues affecting Mac Catalyst request-for-comments The issue is a suggested idea seeking feedback
Projects
None yet
3 participants