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

Add IsMacCatalyst() and IsMacCatalystVersionAtLeast() to System.Runtime #47768

Closed
directhex opened this issue Feb 2, 2021 · 7 comments · Fixed by #50990
Closed

Add IsMacCatalyst() and IsMacCatalystVersionAtLeast() to System.Runtime #47768

directhex opened this issue Feb 2, 2021 · 7 comments · Fixed by #50990
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@directhex
Copy link
Contributor

Background and Motivation

As per the header, all changes to src/libraries/System.Runtime/ref/System.Runtime.cs are subject to API review. This is a minimal change to class OperatingSystem to add checks for Mac Catalyst, directly copying the existing behaviour of IsTvOS() and IsTvOSVersionAtLeast

Roughly speaking, Mac Catalyst exposes the iOS API subset for use on macOS, allowing for source (but not binary) compatibility with iOS/iPad apps. A number of the default apps in recent macOS are actually Catalyst apps.

Ref: #47517

Proposed API

diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs
index 67894c17773..8ed0532b930 100644
--- a/src/libraries/System.Runtime/ref/System.Runtime.cs
+++ b/src/libraries/System.Runtime/ref/System.Runtime.cs
@@ -3099,6 +3099,8 @@ public sealed partial class OperatingSystem : System.ICloneable, System.Runtime.
         public static bool IsIOSVersionAtLeast(int major, int minor = 0, int build = 0) { throw null; }
         public static bool IsMacOS() { throw null; }
         public static bool IsMacOSVersionAtLeast(int major, int minor = 0, int build = 0) { throw null; }
+        public static bool IsMacCatalyst() { throw null; }
+        public static bool IsMacCatalystVersionAtLeast(int major, int minor = 0, int build = 0) { throw null; }
         public static bool IsTvOS() { throw null; }
         public static bool IsTvOSVersionAtLeast(int major, int minor = 0, int build = 0) { throw null; }
         public static bool IsWatchOS() { throw null; }

Usage Examples

Right now, my main usage is in correctly gating which tests are run specifically against the correct platform, i.e.

+        [Fact, PlatformSpecific(TestPlatforms.MacCatalyst)]
+        public static void TestIsOSPlatform_MacCatalyst() => TestIsOSPlatform("MacCatalyst", OperatingSystem.IsMacCatalyst);

Alternative Designs

You could argue that adding one bool-return function per OS isn't very scalable, but that ship already sailed a long time ago I think.

@directhex directhex added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 2, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 2, 2021
directhex added a commit to directhex/runtime that referenced this issue Feb 2, 2021
@marek-safar marek-safar added this to the 6.0.0 milestone Feb 2, 2021
@rolfbjarne
Copy link
Member

rolfbjarne commented Feb 4, 2021

Versioning on Mac Catalyst is somewhat complex, because there are two versions in effect: the macOS version the app is running (or targeting), and the equivalent iOS version of the Mac Catalyst API.

Unfortunately Apple decided to use the macOS version in some places, and the iOS version in other places.

For the Xamarin parts of Mac Catalyst (API bindings), we decided to use the iOS version for API version checks (xamarin/xamarin-macios#10453), because this is the version that Apple uses in Objective-C/Swift, and the version they use in their documentation.

Unfortunately it's not the version their version API ([NSProcessInfo operatingSystemVersionString], [UIDevice systemVersion]) returns.

The end result is that we had to special-case the API we expose for version checks to handle Mac Catalyst versions (rolfbjarne/xamarin-macios@9314bb5).

I believe IsMacCatalystVersionAtLeast should handle versions the same way.

@directhex
Copy link
Contributor Author

@rolfbjarne That's valuable info RE the implementation details. Does it change anything about the API added to the System.Runtime reference assembly (i.e. IsMacCatalystVersionAtLeast()), in your opinion?

@directhex
Copy link
Contributor Author

src/libraries/Common/src/Interop/OSX/Interop.libobjc.cs is where the actual implementation is happening, I'd welcome suggestions there

@rolfbjarne
Copy link
Member

Does it change anything about the API added to the System.Runtime reference assembly (i.e. IsMacCatalystVersionAtLeast()), in your opinion?

No, it doesn't change the public API in any way, it's an implementation detail. Should I open a new issue about it?

@directhex
Copy link
Contributor Author

Does it change anything about the API added to the System.Runtime reference assembly (i.e. IsMacCatalystVersionAtLeast()), in your opinion?

No, it doesn't change the public API in any way, it's an implementation detail. Should I open a new issue about it?

Maybe just move the discussion to the PR for now? Unless it gets merged with "wrong" version detection, then a new issue is absolutely correct

@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 5, 2021
@terrajobst
Copy link
Member

  • Looks good as proposed
namespace System.Diagnostics
{
    public partial class OperatingSystem
    {
        public static bool IsMacCatalyst();
        public static bool IsMacCatalystVersionAtLeast(int major, int minor = 0, int build = 0);
    }
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 9, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants