Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fix/disable RedHat System.Globalization, System.IO.Pipes, System.IO.FileSystem failures #24340

Merged
merged 9 commits into from
Oct 9, 2017
139 changes: 124 additions & 15 deletions src/Common/tests/System/PlatformDetection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,15 @@ public static bool IsWinRT
public static bool IsWindowsSubsystemForLinux => m_isWindowsSubsystemForLinux.Value;
public static bool IsNotWindowsSubsystemForLinux => !IsWindowsSubsystemForLinux;

public static bool IsNotFedoraOrRedHatOrCentos => !IsDistroAndVersion("fedora") && !IsDistroAndVersion("rhel") && !IsDistroAndVersion("centos");

public static bool IsFedora => IsDistroAndVersion("fedora");

// RedHat family covers RedHat and CentOS
public static bool IsRedHatFamily => IsRedHatFamilyAndVersion();
public static bool IsRedHatFamily7 => IsRedHatFamilyAndVersion("7");
public static bool IsRedHatFamily69 => IsRedHatFamilyAndVersion("6.9");
public static bool IsNotRedHatFamily69 => !IsRedHatFamily69;
public static bool IsNotFedoraOrRedHatFamily => !IsFedora && !IsRedHatFamily;

private static bool GetIsWindowsSubsystemForLinux()
{
// https://github.com/Microsoft/BashOnWindows/issues/423#issuecomment-221627364
Expand All @@ -186,7 +191,6 @@ private static bool GetIsWindowsSubsystemForLinux()
public static bool IsDebian => IsDistroAndVersion("debian");
public static bool IsDebian8 => IsDistroAndVersion("debian", "8");
public static bool IsUbuntu1404 => IsDistroAndVersion("ubuntu", "14.04");
public static bool IsCentos7 => IsDistroAndVersion("centos", "7");

private static readonly Version s_osxProductVersion = GetOSXProductVersion();

Expand Down Expand Up @@ -264,6 +268,48 @@ private static bool IsDistroAndVersion(string distroId, string versionId = null)
return false;
}

private static bool IsRedHatFamilyAndVersion(string versionId = null)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
IdVersionPair v = ParseOsReleaseFile();

// RedHat includes minor version. We need to account for that when comparing
if ((v.Id == "rhel" || v.Id == "centos") && VersionEqual(versionId, v.VersionId))
{
return true;
}
}

return false;
}

private static bool VersionEqual(string expectedVersionId, string actualVersionId)
{
if (expectedVersionId == null)
{
return true;
}

string[] expected = expectedVersionId.Split('.');
string[] actual = actualVersionId.Split('.');

if (expected.Length > actual.Length)
{
return false;
}

for (int i = 0; i < expected.Length; i++)
{
if (expected[i] != actual[i])
{
return false;
}
}

return true;
}

private static IdVersionPair ParseOsReleaseFile()
{
Debug.Assert(RuntimeInformation.IsOSPlatform(OSPlatform.Linux));
Expand All @@ -278,26 +324,77 @@ private static IdVersionPair ParseOsReleaseFile()
{
if (line.StartsWith("ID=", System.StringComparison.Ordinal))
{
ret.Id = line.Substring("ID=".Length);
ret.Id = RemoveQuotes(line.Substring("ID=".Length));
}
else if (line.StartsWith("VERSION_ID=", System.StringComparison.Ordinal))
{
ret.VersionId = line.Substring("VERSION_ID=".Length);
ret.VersionId = RemoveQuotes(line.Substring("VERSION_ID=".Length));
}
}
}

string versionId = ret.VersionId;
if (versionId.Length >= 2 && versionId[0] == '"' && versionId[versionId.Length - 1] == '"')
else
{
// Remove quotes.
ret.VersionId = versionId.Substring(1, versionId.Length - 2);
}
string fileName = null;
if (File.Exists("/etc/redhat-release"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be beneficial to use the RuntimeEnvironment class here? It is already doing all this logic, and it is how our customers get the current RID of the machine, and what version of RedHat we are on, etc.

I know the dependency would be a little inverted here (corefx code referencing core-setup code). But we do that today when using the "shared framework" to run tests. So there is precedent.

/cc @weshaggard

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make core-setup necessary for running dotnet apps. Now it is not needed - you can just build coreclr and corefx and use corerun / coreconsole for running your app. I believe that's what the Samsung folks are using.
If we want to unify the detection code (which would be great), I think we should rather move it to corefx and let core-setup use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was only for test code?

As for moving the real logic into CoreFX- yes please!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I am sorry, I've missed that it is for tests only. You are right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eerhardt does RuntimeEnvironment is a public class which can easily take a dependency on? Also, how easy we can add more properties to it? I am asking because for the tests we need to be flexible of adding more properties to the PaltformDetection code and customize it to fit the cases needed in the tests. in another word, for tests, I prefer the flexibility even if we duplicate some of such features.

Another thing, this PR is for 2.0 servicing branch which I prefer to minimize the changes here, so if we decide to take a dependency on RuntimeEnvironment, I prefer to do it in master and not in 2.0 branch.

Copy link
Member

@tarekgh tarekgh Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weshaggard why this functionality is not exposed from RuntimeInformation class instead? why have we introduced RuntimeEnvironment while this can fit better in RuntimeInformation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may get moved down eventually but today the RID stuff is only on the RuntimeEnviroment class.

Copy link
Member Author

@krwq krwq Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I see advantage of using RuntimeEnvironment here I'll exclude this change request from 2.0.0 PR as it will introduce too much churn because PlatformDetection currently lives in the Common folder meaning every test project will now have to depend on PlatformAbstraction. I'll change it in the PR against master as we can change just test utils project.

If you strongly feel we should do that in 2.0.0 branch I'll do it but currently think we should limit 2.0.0 changes to minimum.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, I'll do this for master branch but will exclude for servicing branch (because too many projects will have to be touched)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing, this PR is for 2.0 servicing branch which I prefer to minimize the changes here, so if we decide to take a dependency on RuntimeEnvironment, I prefer to do it in master and not in 2.0 branch.

I'll do this for master branch but will exclude for servicing branch (because too many projects will have to be touched)

Works for me. Thanks.

fileName = "/etc/redhat-release";

if (fileName == null && File.Exists("/etc/system-release"))
fileName = "/etc/system-release";

if (fileName != null)
{
// Parse the format like the following line:
// Red Hat Enterprise Linux Server release 7.3 (Maipo)
using (StreamReader file = new StreamReader(fileName))
{
string line = file.ReadLine();
if (!String.IsNullOrEmpty(line))
{
if (line.StartsWith("Red Hat Enterprise Linux", StringComparison.OrdinalIgnoreCase))
{
ret.Id = "rhel";
}
else if (line.StartsWith("Centos", StringComparison.OrdinalIgnoreCase))
{
ret.Id = "centos";
}
else
{
// automatically generate the distro label
string [] words = line.Split(' ');
StringBuilder sb = new StringBuilder();

foreach (string word in words)
{
if (word.Length > 0)
{
if (Char.IsNumber(word[0]) ||
word.Equals("release", StringComparison.OrdinalIgnoreCase) ||
word.Equals("server", StringComparison.OrdinalIgnoreCase))
{
break;
}
sb.Append(Char.ToLower(word[0]));
}
}
ret.Id = sb.ToString();
}

if (ret.Id.Length >= 2 && ret.Id[0] == '"' && ret.Id[ret.Id.Length - 1] == '"')
{
// Remove quotes.
ret.Id = ret.Id.Substring(1, ret.Id.Length - 2);
int i = 0;
while (i < line.Length && !Char.IsNumber(line[i])) // stop at first number
i++;

if (i < line.Length)
{
int j = i + 1;
while (j < line.Length && (Char.IsNumber(line[j]) || line[j] == '.'))
j++;

ret.VersionId = line.Substring(i, j - i);
}
}
}
}
}

return ret;
Expand Down Expand Up @@ -348,6 +445,18 @@ private static int GetWindowsBuildNumber()
return -1;
}

private static string RemoveQuotes(string s)
{
s = s.Trim();
if (s.Length >= 2 && s[0] == '"' && s[s.Length - 1] == '"')
{
// Remove quotes.
s = s.Substring(1, s.Length - 2);
}

return s;
}

[DllImport("ntdll.dll")]
private static extern int RtlGetVersion(out RTL_OSVERSIONINFOEX lpVersionInformation);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public static int[] UrINNumberGroupSizes()
|| (PlatformDetection.IsUbuntu && !PlatformDetection.IsUbuntu1404)
|| PlatformDetection.IsFedora
|| (PlatformDetection.IsDebian && !PlatformDetection.IsDebian8)
|| PlatformDetection.IsRedHatFamily69
)
{
return new int[] { 3 };
Expand Down
3 changes: 3 additions & 0 deletions src/System.IO.FileSystem/tests/Directory/Delete.cs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@ public void UnixShouldBeAbleToDeleteHiddenDirectory()
[Trait(XunitConstants.Category, XunitConstants.RequiresElevation)]
public void Unix_NotFoundDirectory_ReadOnlyVolume()
{
if (PlatformDetection.IsRedHatFamily69)
return; // [ActiveIssue(https://github.com/dotnet/corefx/issues/21920)]

ReadOnly_FileSystemHelper(readOnlyDirectory =>
{
Assert.Throws<DirectoryNotFoundException>(() => Delete(Path.Combine(readOnlyDirectory, "DoesNotExist")));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public static void ClonedClient_ActsAsOriginalClient()
}
}

[Fact]
[ConditionalFact(nameof(PlatformDetection) + "." + "IsNotRedHatFamily69")]
[PlatformSpecific(TestPlatforms.Linux)] // On Linux, setting the buffer size of the server will also set the buffer size of the client
public static void Linux_BufferSizeRoundtrips()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public async Task GetAsync_SupportedSSLVersion_Succeeds(SslProtocols sslProtocol
{
using (HttpClientHandler handler = new HttpClientHandler())
{
if (PlatformDetection.IsCentos7)
if (PlatformDetection.IsRedHatFamily7)
{
// Default protocol selection is always TLSv1 on Centos7 libcurl 7.29.0
// Hence, set the specific protocol on HttpClient that is required by test
Expand Down