add OSPlatform.macOS, make OSPlatform.Equal use OrdinalIgnoreCaseComparison #39064

adamsitnik · 2020-07-10T08:47:48Z

Contributes to #33331

…sons

Dotnet-GitSync-Bot · 2020-07-10T08:47:51Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

akoeplinger · 2020-07-10T09:28:08Z

hey @adamsitnik I just merged my PR #36704 which also implemented most of these new additions, so please rebase this one :)

adamsitnik · 2020-07-10T09:46:36Z

I just merged my PR #36704 which also implemented most of these new additions, so please rebase this one :)

@akoeplinger thanks for letting me know!

jeffhandley · 2020-07-10T10:06:39Z

I think there was a spoken comment during the review that we could have macOS return “OSX”. If we did that, would it fee more or less hacky to you, @adamsitnik?

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs

adamsitnik · 2020-07-10T10:12:32Z

I think there was a spoken comment during the review that we could have macOS return “OSX”. If we did that, would it fee more or less hacky to you

it would make the following lines work:

Assert.Equal(OSPlatform.OSX, OSPlatform.macOS);

but not:

Assert.Equal(OSPlatform.macOS, OSPlatform.Create("macOS"));
Assert.Equal(OSPlatform.OSX, OSPlatform.Create("macOS"));

I like @akoeplinger idea and I am going to apply it

adamsitnik · 2020-07-10T10:25:36Z

@akoeplinger @jeffhandley thanks for great suggestions, the code looks much better now!

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs

src/libraries/System.Runtime.InteropServices.RuntimeInformation/tests/CheckPlatformTests.cs

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs

marek-safar · 2020-07-10T11:18:03Z

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs

+        public static OSPlatform macOS { get; } = new OSPlatform("MACOS");
+
+        [EditorBrowsable(EditorBrowsableState.Never)] // https://github.com/dotnet/runtime/issues/33331#issuecomment-650326500
+        public static OSPlatform OSX { get; } = macOS;


This is a breaking change.

For example

void Foo (OSPlatform osp) { if (osp.ToString() == "OSX") return; }

ok, then we should add the macOS/OSX magic to ToString() too

I do not think we should be changing the string identifier for OSX/macOS. It should stay as OSX. OSX is hardcoded in many places that are hard/impossible to fix, like NuGet RIDs. We are not gaining much by changing it ad-hoc in subset of places. If we want to change it, it should be done everywhere and it should be a feature on its own.

We discussed this during the Design Review meeting today and got a direction on how to address this breaking change risk while still handling equivalence. The comments are posted on #33331, but at a high-level, OSPlatform should not try to handle equivalence between platforms itself; instead, the RuntimeInformation.IsOSPlatform should handle it.

adamsitnik · 2020-07-10T14:23:11Z

I've been trying to wrap my head around the macOS and OSX thing, and I got to the following conclusions:

Based on https://en.wikipedia.org/w/index.php?title=Macintosh_operating_systems&section=4#Releases_2

    Mac OS X 10.0 – code name "Cheetah", released to end users on Saturday, March 24, 2001
    Mac OS X 10.1 – code name "Puma", released to end users on Tuesday, September 25, 2001
    Mac OS X 10.2 – also marketed as "Jaguar", released to end users on Friday, August 23, 2002
    Mac OS X Panther – version 10.3, released to end users on Friday, October 24, 2003
    Mac OS X Tiger – version 10.4, released to end users on Friday, April 29, 2005
    Mac OS X Leopard – version 10.5, released to end users on Friday, October 26, 2007
    Mac OS X Snow Leopard – version 10.6, publicly unveiled on Monday, June 8, 2009
    Mac OS X Lion – version 10.7, released to end users on Wednesday, July 20, 2011
    OS X Mountain Lion – version 10.8, released to end users on Wednesday, July 25, 2012
    OS X Mavericks – version 10.9, released to end users on Tuesday, October 22, 2013
    OS X Yosemite – version 10.10, released to end users on Thursday, October 16, 2014
    OS X El Capitan – version 10.11, released to end users on Wednesday, September 30, 2015
    macOS Sierra – version 10.12, released to end users on Tuesday, September 20, 2016
    macOS High Sierra – version 10.13, released to end users on Monday, September 25, 2017
    macOS Mojave – version 10.14, released to end users on Monday, September 24, 2018
    macOS Catalina – version 10.15, released to end users on Monday, October 7, 2019
    macOS Big Sur - version 11.0, announced at WWDC 2020 on Monday, June 22, 2020 and currently in development

The value returned by Interop.Sys.GetUnixName() should be:

OSX for versions 10.0-10.11
macOS for versions 10.12-11.00

But .NET 5 officially supports only macOS 10.13+ so Interop.Sys.GetUnixName() should just return macOS

Moreover it's not just about the naming, so: OSPlatform.OSX != OSPlatform.macOS

Assert.NotEqual(OSPlatform.OSX, OSPlatform.macOS);

But for the sake of backward compatibility and usability:

RuntimeInformation.IsOSPlatform(OSPlatform.OSX) // should return true for every mac OS and .NET

And for .NET 5:

RuntimeInformation.IsOSPlatform(OSPlatform.macOS) // should return true only on .NET 5 on macOS 10.13+

Any comments on that before I apply the changes?

akoeplinger · 2020-07-10T14:28:35Z

@adamsitnik the OSX -> macOS change was purely a marketing/branding change so it'd be very weird if OSPlatform.OSX != OSPlatform.macOS. IMHO we should treat them as the same, except the ToString() case.

jkotas · 2020-07-10T17:42:39Z

....InteropServices.RuntimeInformation/ref/System.Runtime.InteropServices.RuntimeInformation.cs

        public static System.Runtime.InteropServices.OSPlatform Linux { get { throw null; } }
+        [System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] // https://github.com/dotnet/runtime/issues/33331#issuecomment-650326500


ref files should not use comments like this. They will disappear next time the file is reformatted using auto-generator.

jkotas · 2020-07-10T17:44:04Z

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs

-        public static OSPlatform OSX { get; } = new OSPlatform("OSX");
+        public static OSPlatform macOS { get; } = new OSPlatform("MACOS");
+
+        [EditorBrowsable(EditorBrowsableState.Never)] // https://github.com/dotnet/runtime/issues/33331#issuecomment-650326500


Is this link really useful for context? It points to several screens of API review notes, and it does not say much useful about the OSX.

Should this rather say "// superseded by macOS" ?

jkotas · 2020-07-10T17:49:02Z

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs

        }

        public override bool Equals(object? obj)
        {
-            return obj is OSPlatform && Equals((OSPlatform)obj);
+            Debug.Fail("The non generic method should not be used by BCL");


We want the debug libraries to be fully usable. This should be removed.

If you would like to check that this method is not called by the BCL, it is a job for the analyzer.

jkotas · 2020-07-10T17:52:23Z

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs

-        internal bool Equals(string? other)
-        {
-            return string.Equals(_osPlatform, other, StringComparison.Ordinal);
+            return string.Equals(_osPlatform, other._osPlatform, StringComparison.OrdinalIgnoreCase);


What is motivating this change? This will make this API quite a bit more expensive.

jkotas · 2020-07-10T18:14:18Z

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs

+        public static OSPlatform macOS { get; } = new OSPlatform("MACOS");
+
+        [EditorBrowsable(EditorBrowsableState.Never)] // https://github.com/dotnet/runtime/issues/33331#issuecomment-650326500
+        public static OSPlatform OSX { get; } = macOS;


Suggested change

public static OSPlatform OSX { get; } = macOS;

public static OSPlatform OSX => macOS;

adamsitnik · 2020-07-13T17:05:21Z

I've sent a new PR: #39209

adamsitnik added 4 commits July 10, 2020 09:21

introduce new OSPlatforms

5909ffe

use OrdinalIgnoreCase instead of Ordinal for os platform name compari…

d586408

…sons

hide OSX, make sure OSX == macOS

76783a6

add more tests

2d2ed3e

Dotnet-GitSync-Bot added area-System.Runtime.InteropServices new-api-needs-documentation labels Jul 10, 2020

Merge remote-tracking branch 'upstream/master' into 33331newOsPlatforms

f388f03

adamsitnik changed the title ~~add new OSPlatforms (Android, iOS, macOS, tvOS, watchOS)~~ add OSPlatform.macOS, make OSPlatform.Equal use OrdinalIgnoreCaseComparison Jul 10, 2020

akoeplinger reviewed Jul 10, 2020

View reviewed changes

adamsitnik added 2 commits July 10, 2020 12:16

simplify the code based on ideas from code review

7a8f548

add more test cases

8a3277e

jeffhandley approved these changes Jul 10, 2020

View reviewed changes

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs Show resolved Hide resolved

src/libraries/System.Runtime.InteropServices.RuntimeInformation/tests/CheckPlatformTests.cs Show resolved Hide resolved

adamsitnik commented Jul 10, 2020

View reviewed changes

src/libraries/System.Runtime.InteropServices.RuntimeInformation/tests/CheckPlatformTests.cs Show resolved Hide resolved

add suggested test cases

d3e751d

akoeplinger approved these changes Jul 10, 2020

View reviewed changes

src/libraries/System.Runtime.InteropServices.RuntimeInformation/tests/CheckPlatformTests.cs Show resolved Hide resolved

marek-safar reviewed Jul 10, 2020

View reviewed changes

...vices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/OSPlatform.cs Show resolved Hide resolved

marek-safar reviewed Jul 10, 2020

View reviewed changes

adamsitnik added 3 commits July 10, 2020 14:14

address code review feedback

8ae72cf

fix a bug that I've introduced

f3c439a

use StringComparison.OrdinalIgnoreCase in GetHashCode as well

84d959a

jeffhandley mentioned this pull request Jul 10, 2020

add IsOSPlatformOrLater and IsOSPlatformEarlierThan methods #39005

Merged

jeffhandley requested a review from buyaa-n July 10, 2020 17:34

jkotas reviewed Jul 10, 2020

View reviewed changes

adamsitnik closed this Jul 13, 2020

adamsitnik deleted the 33331newOsPlatforms branch July 13, 2020 17:05

karelz added this to the 5.0.0 milestone Aug 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add OSPlatform.macOS, make OSPlatform.Equal use OrdinalIgnoreCaseComparison #39064

add OSPlatform.macOS, make OSPlatform.Equal use OrdinalIgnoreCaseComparison #39064

adamsitnik commented Jul 10, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Jul 10, 2020

akoeplinger commented Jul 10, 2020

adamsitnik commented Jul 10, 2020

jeffhandley commented Jul 10, 2020

adamsitnik commented Jul 10, 2020

adamsitnik commented Jul 10, 2020

marek-safar Jul 10, 2020

akoeplinger Jul 10, 2020

marek-safar Jul 10, 2020

akoeplinger Jul 10, 2020

jkotas Jul 10, 2020 •

edited

Loading

jeffhandley Jul 10, 2020

adamsitnik commented Jul 10, 2020

akoeplinger commented Jul 10, 2020 •

edited

Loading

jkotas Jul 10, 2020

jkotas Jul 10, 2020

jkotas Jul 10, 2020

jkotas Jul 10, 2020

jkotas Jul 10, 2020

adamsitnik commented Jul 13, 2020

		public static System.Runtime.InteropServices.OSPlatform Linux { get { throw null; } }
		[System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] // https://github.com/dotnet/runtime/issues/33331#issuecomment-650326500

	public static OSPlatform OSX { get; } = macOS;
	public static OSPlatform OSX => macOS;

add OSPlatform.macOS, make OSPlatform.Equal use OrdinalIgnoreCaseComparison #39064

add OSPlatform.macOS, make OSPlatform.Equal use OrdinalIgnoreCaseComparison #39064

Conversation

adamsitnik commented Jul 10, 2020 • edited Loading

Dotnet-GitSync-Bot commented Jul 10, 2020

akoeplinger commented Jul 10, 2020

adamsitnik commented Jul 10, 2020

jeffhandley commented Jul 10, 2020

adamsitnik commented Jul 10, 2020

adamsitnik commented Jul 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik commented Jul 10, 2020

akoeplinger commented Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik commented Jul 13, 2020

adamsitnik commented Jul 10, 2020 •

edited

Loading

jkotas Jul 10, 2020 •

edited

Loading

akoeplinger commented Jul 10, 2020 •

edited

Loading