[follow-up] Use RedHat code detection from Microsoft.DotNet.PlatformAbstractions #24531

krwq · 2017-10-10T00:31:36Z

This PR is a follow up on my PR against release/2.0.0 which cleaned up bunch of code related to RedHat: https://github.com/dotnet/corefx/pull/24340/files

This also addresses feedback from @eerhardt about reusing core-setup code.

Changes included here:

rename/join PlatformDetection properties:

IsRedHat => IsRedHatFamily
IsRedHat69 => IsRedHatFamily6
IsNotRedHat69 => IsNotRedHatFamily6
IsCentos7 => IsRedHatFamily7
IsNotFedoraOrRedHatOrCentos => IsNotFedoraOrRedHatFamily

replace parsing code for distro detection so that it calls into https://github.com/dotnet/core-setup/blob/master/src/managed/Microsoft.DotNet.PlatformAbstractions/RuntimeEnvironment.cs

tarekgh · 2017-10-10T00:50:34Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.Unix.cs

        {
-            Debug.Assert(RuntimeInformation.IsOSPlatform(OSPlatform.Linux));
+            Id = Microsoft.DotNet.PlatformAbstractions.RuntimeEnvironment.OperatingSystem,
+            VersionId = Microsoft.DotNet.PlatformAbstractions.RuntimeEnvironment.OperatingSystemVersion


VersionId = M [](start = 11, length = 14)

can we change this to Version struct instead of string? that will make it easier to use in the code and in comparison too,

@tarekgh, @eerhardt sorry for late response here, I had to suspend this work for a while - I've just tried converting the code to use Version class but there is one detail which may cause us some problems and might be worth mentioning: Version class requires that we pass at minimum 2 arguments (major, minor) and does not allow negative minor version - in some cases we might expect something to work on any 6.X version (like with RedHat or CentOS case) and in some cases to work only on 6.0 but not on other 6.X versions - this might be problematic. I was thinking that example code comparing version could look something like following:

// NOTE: this does not work as expected private static bool VersionEquivalentWith(Version expectedVersionId, Version actualVersionId) { return expectedVersionId.Major == actualVersionId.Major && expectedVersionId.Minor == actualVersionId.Minor && (expectedVersionId.Build == -1 || expectedVersionId.Build == actualVersionId.Build) && (expectedVersionId.Revision == -1 || expectedVersionId.Revision == actualVersionId.Revision); }

but this is causing problems as mentioned above (i.e. expecting RHEL6, got 6.9 => condition failed). We might approach working around this limitation in different ways:

use some placeholder number as "minor not defined" i.e. 9999999

compare ranges - that sounds fairly non-intuitive to define, i.e. checking for any RHEL6 would be something along the lines of: X >= V(6) && X < V(7), where more intuitive X == V(6) will not work as expected

define expected version in terms of 4 numbers and actual as Version class - that could work but not sure if that would make the code cleaner as all of these variable version parts would have to be handled

leave string comparison

any opinions? Perhaps there is some way to simplify this overall.

I think when we don't have a minor version we just use 0 which should work for any checks use this version. what would be wrong with that?

@tarekgh that was the first thing I've tried. Example problem: You want condition like:
IsCentOS7 -> you have to compare with new Version(7,0) -> let's assume you're on CentOS 7.2 - should this comparison return true or false?
Other case: IsCentOS70, you check for new Version(7,0) -> no matter what you answered before this is a problematic situation

Ok, talked with @tarekgh offline we will go with:

parse to Version class in such a way that when minor is not provided interpret it as minor = 0

each function which compares versions takes 4 integers and compares them with Version handling minor == -1

tarekgh · 2017-10-10T00:52:05Z

src/System.Drawing.Common/tests/Helpers.cs

-            // CentOS 7, RHEL 7 and Ubuntu 14.04, as well as Fedora 25 and OpenSUSE 4.22 are running outdated versions of libgdiplus
-            if (PlatformDetection.IsCentos7 || PlatformDetection.IsRedHat || PlatformDetection.IsUbuntu1404 || PlatformDetection.IsFedora || PlatformDetection.IsOpenSUSE)
+            // RedHat and Ubuntu 14.04, as well as Fedora 25 and OpenSUSE 4.22 are running outdated versions of libgdiplus
+            if (PlatformDetection.IsRedHatFamily || PlatformDetection.IsUbuntu1404 || PlatformDetection.IsFedora || PlatformDetection.IsOpenSUSE)


PlatformDetection.IsRedHatFamily [](start = 16, length = 32)

we should check version 7 here.

interesting, I am seeing the original code was checking for redhat without the version. If you are not getting any problem with your change here, then I think your change would be ok.

tarekgh · 2017-10-10T00:52:53Z

src/System.Drawing.Common/tests/Helpers.cs

-            // CentOS 7, RHEL 7 and Ubuntu 14.04 are running outdated versions of libgdiplus
-            if (PlatformDetection.IsCentos7 || PlatformDetection.IsRedHat || PlatformDetection.IsUbuntu1404)
+            // RedHat and Ubuntu 14.04 are running outdated versions of libgdiplus
+            if (PlatformDetection.IsRedHatFamily || PlatformDetection.IsUbuntu1404)


(PlatformDetection.IsRedHatFamily [](start = 15, length = 33)

same

tarekgh · 2017-10-10T00:57:35Z

System/PlatformDetection.Unix.cs(95,17): error CS0246: The type or namespace name 'IdVersionPair' could not be found (are you missing a using directive or an assembly reference?) [/opt/corefx/src/CoreFx.Private.TestUtilities/src/CoreFx.Private.TestUtilities.csproj]

eerhardt · 2017-10-10T02:04:40Z

external/test-runtime/XUnit.Runtime.depproj

@@ -50,6 +50,9 @@
    <PackageReference Include="Microsoft.3rdpartytools.MarkdownLog">
      <Version>0.10.0-alpha-experimental</Version>
    </PackageReference>
+    <PackageReference Include="Microsoft.DotNet.PlatformAbstractions">
+      <Version>2.1.0-preview1-25806-02</Version>


It would be nice to either reuse this property:

corefx/dependencies.props

Line 34 in e273e7f

<MicrosoftNETCoreAppPackageVersion>2.1.0-preview1-25806-02</MicrosoftNETCoreAppPackageVersion>

Or even better, define a new MicrosoftDotNetPlatformAbstractionsPackageVersion property in dependencies.props. That way this version automatically gets updated when a new version comes out.

eerhardt · 2017-10-10T02:05:53Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.Unix.cs

-                                i++;
+        private static bool VersionEqual(string expectedVersionId, string actualVersionId)
+        {
+            if (expectedVersionId == null)


Would it make sense to parse these into Version structs and use the comparisons that come with it?

eerhardt · 2017-10-10T02:10:10Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.Unix.cs

                }
            }

-            return ret;
+            return true;
        }

        private static string RemoveQuotes(string s)


I don't think this method is used anymore.

eerhardt · 2017-10-10T02:13:02Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.Unix.cs


-            return "Distro=" + v.Id + " VersionId=" + v.VersionId + " Pretty=" + v.PrettyName + " Version=" + v.Version;
+            return "Distro=" + v.Id + " VersionId=" + v.VersionId;
        }

        private static readonly Version s_osxProductVersion = GetOSXProductVersion();


GetOSXProductVersion() should be able to be removed as well and just use RuntimeEnvironment.

Sorry, I meant GetOSXKernelVersion(). That is the method that appears to be duplicated with PlatformAbstractions.

RuntimeEnvironment.OperatingSystemVersion will give you the same value as what GetOSXKernelVersion() will return.

sorry for delay - had to suspend this PR. The value was not identical since they adjust version a little bit (10.<old major version - 4>.<rest of version>) but there were only two usages of that so I've changed those

krwq · 2017-11-03T17:33:27Z

sorry for delay, had to suspend this work for a while. PTAL

krwq · 2017-11-03T20:56:46Z

src/System.Net.Security/tests/FunctionalTests/NegotiateStreamTestForUnix.cs

@@ -82,6 +82,7 @@ public bool CheckAndClearCredentials(ITestOutputHelper output)
                output.WriteLine("kdestroy returned {0}", clearCreds.ExitCode);
                return (clearCreds.ExitCode == 0);
            }
+


need to remove this

eerhardt · 2017-11-03T21:12:31Z

src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.Unix.cs


-        public static Version OSXKernelVersion { get; } = GetOSXKernelVersion();
+        public static Version OSXKernelVersion { get; } = ToVersion(Microsoft.DotNet.PlatformAbstractions.RuntimeEnvironment.OperatingSystemVersion);


(nit) Does it work to using Microsoft.DotNet.PlatformAbstractions;, or is there some type collision?

@eerhardt - type collision - class RuntimeEnvironment unfortunatelly also exists in corefx :-)

eerhardt

Looks good! Thanks for following up on this @krwq.

krwq · 2017-11-03T21:30:40Z

Thanks @eerhardt! Merging since previous build was green and I only changed whitespace

tarekgh · 2017-11-03T21:49:17Z

@krwq did you see my question above?

krwq · 2017-11-03T21:51:02Z

@tarekgh not seeing any question, could you resend?

tarekgh · 2017-11-03T22:00:22Z

[‎11/‎3/‎2017 2:52 PM]
why the version number changed here? if 10.12 is not really the kernel version, we should rename the property OSXKernelVersion to OSXVersion
regarding the line
- if (PlatformDetection.IsOSX && PlatformDetection.OSXKernelVersion < new Version(16, 0)) + if (PlatformDetection.IsOSX && PlatformDetection.OSXKernelVersion < new Version(10, 12))

krwq · 2017-11-03T22:07:20Z

@tarekgh I've addressed this in #25057

…et/corefx#24531) Commit migrated from dotnet/corefx@4bf9019

krwq requested a review from tarekgh October 10, 2017 00:31

dnfclas added the cla-already-signed label Oct 10, 2017

tarekgh reviewed Oct 10, 2017

View reviewed changes

eerhardt reviewed Oct 10, 2017

View reviewed changes

karelz assigned krwq Oct 11, 2017

karelz added the area-Infrastructure-libraries label Oct 11, 2017

karelz added this to the 2.1.0 milestone Oct 28, 2017

krwq force-pushed the redhat_followup branch from d843278 to 3d3933e Compare November 3, 2017 17:29

krwq commented Nov 3, 2017

View reviewed changes

Use distro detection from Microsoft.DotNet.PlatformAbstractions

e27681a

krwq force-pushed the redhat_followup branch from 3d3933e to e27681a Compare November 3, 2017 20:58

eerhardt reviewed Nov 3, 2017

View reviewed changes

eerhardt approved these changes Nov 3, 2017

View reviewed changes

krwq merged commit 4bf9019 into dotnet:master Nov 3, 2017

krwq mentioned this pull request Nov 3, 2017

Rename PlatformDetection.OSXKernelVersion to PlatformDetection.OSXVersion #25057

Merged

krwq mentioned this pull request Jan 31, 2020

Port back improvements made to PlatformDetection related to version detection dotnet/runtime#24043

Closed

weshaggard mentioned this pull request Jan 31, 2020

CoreFx test utility depends on Microsoft.DotNet.PlatformAbstractions dotnet/runtime#25681

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Use distro detection from Microsoft.DotNet.PlatformAbstractions (dotn…

f3f741e

…et/corefx#24531) Commit migrated from dotnet/corefx@4bf9019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[follow-up] Use RedHat code detection from Microsoft.DotNet.PlatformAbstractions #24531

[follow-up] Use RedHat code detection from Microsoft.DotNet.PlatformAbstractions #24531

krwq commented Oct 10, 2017

tarekgh Oct 10, 2017

krwq Nov 2, 2017 •

edited

Loading

tarekgh Nov 2, 2017

krwq Nov 2, 2017

krwq Nov 2, 2017

tarekgh Oct 10, 2017

tarekgh Oct 10, 2017

tarekgh Oct 10, 2017

tarekgh commented Oct 10, 2017

eerhardt Oct 10, 2017

eerhardt Oct 10, 2017

eerhardt Oct 10, 2017

eerhardt Oct 10, 2017

eerhardt Oct 10, 2017

krwq Nov 3, 2017

krwq commented Nov 3, 2017

krwq Nov 3, 2017

eerhardt Nov 3, 2017

krwq Nov 3, 2017

eerhardt left a comment

krwq commented Nov 3, 2017

tarekgh commented Nov 3, 2017

krwq commented Nov 3, 2017

tarekgh commented Nov 3, 2017

krwq commented Nov 3, 2017


		public static Version OSXKernelVersion { get; } = GetOSXKernelVersion();
		public static Version OSXKernelVersion { get; } = ToVersion(Microsoft.DotNet.PlatformAbstractions.RuntimeEnvironment.OperatingSystemVersion);

[follow-up] Use RedHat code detection from Microsoft.DotNet.PlatformAbstractions #24531

[follow-up] Use RedHat code detection from Microsoft.DotNet.PlatformAbstractions #24531

Conversation

krwq commented Oct 10, 2017

Choose a reason for hiding this comment

krwq Nov 2, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Oct 10, 2017

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

Choose a reason for hiding this comment

krwq commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

krwq commented Nov 3, 2017

tarekgh commented Nov 3, 2017

krwq commented Nov 3, 2017

tarekgh commented Nov 3, 2017

krwq commented Nov 3, 2017

krwq Nov 2, 2017 •

edited

Loading