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
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5d60299
fix System.Globalization RedHat failure
krwq Sep 29, 2017
8f39758
add RedHat version detection
krwq Oct 2, 2017
a85637c
change test behavior also on centos 6.9
krwq Oct 6, 2017
f4c0b52
unify RedHat and CentOS detection
krwq Oct 6, 2017
f7d387a
disable failing System.IO.Pipes on RedHatFamily69
krwq Oct 6, 2017
f6df665
Disable Unix_NotFoundDirectory_ReadOnlyVolume
krwq Oct 6, 2017
6848bbf
fix version comparison when only major is provided on RedHat
krwq Oct 6, 2017
59088fc
remove confusing checks for non-standard RedHat distro (rhl)
krwq Oct 7, 2017
b7702f2
clean up version comparison and reuse properties
krwq Oct 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 118 additions & 14 deletions src/Common/tests/System/PlatformDetection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,13 @@ 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 IsNotFedoraOrRedHatFamily => !IsDistroAndVersion("fedora") && !IsRedHatFamily;
Copy link
Member

@tarekgh tarekgh Oct 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Use IsFedora instead of IsDistroAndVersion("fedora"). ensure IsFedora is moved before this line

Copy link
Member Author

Choose a reason for hiding this comment

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

pending


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

Choose a reason for hiding this comment

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

please move IsFedora and IsRedHatFamily before the line of IsNotFedoraOrRedHatFamily to ensure the right order of the static initialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

pending

public static bool IsRedHatFamily => IsRedHatFamilyAndVersion();
Copy link
Member

Choose a reason for hiding this comment

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

please add comment that this is cover RedHat and Centos. Just in case if anyone want to check Centos and don't know it is covered by this property.

Copy link
Member Author

Choose a reason for hiding this comment

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

pending

public static bool IsRedHatFamily7 => IsRedHatFamilyAndVersion("7");
Copy link
Member

Choose a reason for hiding this comment

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

For CentOS and RHEL 7, there is a difference in the version reported via /etc/os-release. CentOS reports just 7, but RHEL reports 7.x where x is the minor version. So this needs to be taken into account, unless I am missing something, there is no version normalization code anywhere.
That is different for version 6 where there is no /etc/os-release and the /etc/redhat-release contains version 6.x, that means including the minor version for both RHEL and CentOS.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, i've added additional checks in IsRedHatFamilyAndVersion and RedHatVersionEqual

public static bool IsRedHatFamily69 => IsRedHatFamilyAndVersion("6.9");
public static bool IsNotRedHatFamily69 => !IsRedHatFamilyAndVersion("6.9");

private static bool GetIsWindowsSubsystemForLinux()
{
Expand All @@ -186,7 +190,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 +267,44 @@ 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();
bool isRedHat = v.Id == "rhel";
// RedHat includes minor version. We need to account for that when comparing
if ((isRedHat || v.Id == "centos") && (versionId == null || v.VersionId == versionId || (isRedHat && RedHatVersionEqual(versionId, v.VersionId))))
Copy link
Member

Choose a reason for hiding this comment

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

the version check logic can be written in a more global way which can include checking only concerned parts of the version. this can be done in 2 ways:

1- either you do versionId.Split('.') which will produce an array of strings of the version parts and also do v.VersionId.Split('.'). Then try to check versionId parts against v.VersionId parts (regardless how many parts is in versionId).
2- other idea is when we are reading the distro info, we read the version string and convert it to Version struct. then we can just use numbers instead of strings all the time.

if we do one of these ideas, we don't have to distinguish between isRedHat or not as you are doing in your code.

Copy link
Member Author

Choose a reason for hiding this comment

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

pending (string.split) but might get changed after fixing Eric's comment

{
return true;
}
}

return false;
}

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

if (expectedVersionId.Contains("."))
{
return expectedVersionId == actualVersionId;
}

int dotPos = actualVersionId.IndexOf('.');
string minorVersion = actualVersionId;
if (dotPos != -1)
{
minorVersion = minorVersion.Substring(dotPos + 1);
}

return expectedVersionId == minorVersion;
}

private static IdVersionPair ParseOsReleaseFile()
{
Debug.Assert(RuntimeInformation.IsOSPlatform(OSPlatform.Linux));
Expand All @@ -278,26 +319,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 •
edited
Loading

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 •
edited
Loading

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 +440,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
1 change: 1 addition & 0 deletions src/System.Globalization/tests/NumberFormatInfo/NumberFormatInfoData.cs
View file
Open in desktop
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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion src/System.IO.Pipes/tests/AnonymousPipeTests/AnonymousPipeTest.Specific.cs
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.SslProtocols.cs
View file
Open in desktop
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