From a2911b81fa181f79e914425921a170c6f96847bf Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 14 Oct 2022 14:41:51 -0700 Subject: [PATCH 1/7] Fix loading app-local ICU --- .../IcuAppLocal/IcuAppLocal.Tests.csproj | 14 +++++++++ .../tests/IcuAppLocal/IcuAppLocal.cs | 29 +++++++++++++++++++ .../System.Globalization.Native/pal_icushim.c | 1 + 3 files changed, 44 insertions(+) create mode 100644 src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj create mode 100644 src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj new file mode 100644 index 00000000000000..ad0bbb6b91364f --- /dev/null +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj @@ -0,0 +1,14 @@ + + + $(NetCoreAppCurrent) + true + + + + + + + + + + \ No newline at end of file diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs new file mode 100644 index 00000000000000..069dd559942452 --- /dev/null +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs @@ -0,0 +1,29 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Reflection; +using Xunit; + +namespace System.Globalization.Tests +{ + public class IcuAppLocalTests + { + private static bool SupportIcuLocals => PlatformDetection.IsNotMobile && !PlatformDetection.Is32BitProcess; + + [ConditionalFact(nameof(SupportIcuLocals))] + public void TestIcuAppLocal() + { + Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib"); + Assert.NotNull(interopGlobalization); + + MethodInfo? methodInfo = interopGlobalization.GetMethod("GetICUVersion", BindingFlags.NonPublic | BindingFlags.Static); + Assert.NotNull(methodInfo); + + // Assert the ICU version 0x44020009 is 68.2.0.9 + Assert.Equal(0x44020009, (int)methodInfo.Invoke(null, null)); + + // Now call globalization API to ensure the binding working without any problem. + Assert.Equal(-1, CultureInfo.GetCultureInfo("en-US").CompareInfo.Compare("sample\u0000", "Sample\u0000", CompareOptions.IgnoreSymbols)); + } + } +} diff --git a/src/native/libs/System.Globalization.Native/pal_icushim.c b/src/native/libs/System.Globalization.Native/pal_icushim.c index d23499c80b1ee1..d6ae58dd1b35b7 100644 --- a/src/native/libs/System.Globalization.Native/pal_icushim.c +++ b/src/native/libs/System.Globalization.Native/pal_icushim.c @@ -539,6 +539,7 @@ void GlobalizationNative_InitICUFunctions(void* icuuc, void* icuin, const char* ValidateICUDataCanLoad(); InitializeVariableMaxAndTopPointers(symbolVersion); + InitializeUColClonePointers(symbolVersion); } #undef PER_FUNCTION_BLOCK From 860765155b0afe0a1298fbbd1ba03a52ba442f1d Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 14 Oct 2022 18:44:56 -0700 Subject: [PATCH 2/7] Restrict the platforms to run the tests on --- .../System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs index 069dd559942452..339f641d202516 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs @@ -8,9 +8,9 @@ namespace System.Globalization.Tests { public class IcuAppLocalTests { - private static bool SupportIcuLocals => PlatformDetection.IsNotMobile && !PlatformDetection.Is32BitProcess; - [ConditionalFact(nameof(SupportIcuLocals))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] + [PlatformSpecific(TestPlatforms.Windows | TestPlatforms.Linux)] public void TestIcuAppLocal() { Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib"); From 299d25503a46eed519e579cfd129888f73c1d4ff Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sat, 15 Oct 2022 09:51:36 -0700 Subject: [PATCH 3/7] Adjust the test supported platforms --- .../tests/IcuAppLocal/IcuAppLocal.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs index 339f641d202516..17984fb8594c1f 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs @@ -9,8 +9,13 @@ namespace System.Globalization.Tests public class IcuAppLocalTests { - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] - [PlatformSpecific(TestPlatforms.Windows | TestPlatforms.Linux)] + private static bool SupportIcuPackageDownload => PlatformDetection.Is64BitProcess && + PlatformDetection.IsNotOSX && + PlatformDetection.IsNotMobile && + !PlatformDetection.IsAlpine && + !PlatformDetection.IsLinuxBionic; + + [ConditionalFact(nameof(SupportIcuPackageDownload))] public void TestIcuAppLocal() { Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib"); From 13ce1cbeafb3953a54415da1f7d5faed51277ebc Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sat, 15 Oct 2022 12:48:20 -0700 Subject: [PATCH 4/7] Test Platforms --- .../System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs index 17984fb8594c1f..62b622a99422da 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs @@ -16,6 +16,7 @@ public class IcuAppLocalTests !PlatformDetection.IsLinuxBionic; [ConditionalFact(nameof(SupportIcuPackageDownload))] + [SkipOnPlatform(TestPlatforms.OSX | TestPlatforms.Browser | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "ICU package doesn't support these platforms.")] public void TestIcuAppLocal() { Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib"); From ea10dcffcf32da811504fc6e371ec327eb130f20 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sun, 16 Oct 2022 10:19:22 -0700 Subject: [PATCH 5/7] Fix the test OS filtering --- .../IcuAppLocal/IcuAppLocal.Tests.csproj | 13 ++++++- .../tests/IcuAppLocal/IcuAppLocal.cs | 35 +++++++++++++------ 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj index ad0bbb6b91364f..28d8c4b6e9b649 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj @@ -2,6 +2,7 @@ $(NetCoreAppCurrent) true + true @@ -9,6 +10,16 @@ - + + \ No newline at end of file diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs index 62b622a99422da..586a020b20ae1b 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using Microsoft.DotNet.RemoteExecutor; +using System.Diagnostics; using System.Reflection; using Xunit; @@ -13,23 +15,36 @@ public class IcuAppLocalTests PlatformDetection.IsNotOSX && PlatformDetection.IsNotMobile && !PlatformDetection.IsAlpine && - !PlatformDetection.IsLinuxBionic; + !PlatformDetection.IsLinuxBionic && + RemoteExecutor.IsSupported; [ConditionalFact(nameof(SupportIcuPackageDownload))] - [SkipOnPlatform(TestPlatforms.OSX | TestPlatforms.Browser | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "ICU package doesn't support these platforms.")] public void TestIcuAppLocal() { - Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib"); - Assert.NotNull(interopGlobalization); + // We define this switch dynamically during the runtime using RemoteExecutor. + // The reason is, if we enable ICU app-local here, this test will compile and run + // on all supported OSs even the ICU NuGet package not have native bits support such OSs. + // Note, it doesn't matter if we have test case conditioned to not run on such OSs, because + // the test has to start running first before filtering the test cases and the globalization + // code will run at that time and will fail fast at that time. - MethodInfo? methodInfo = interopGlobalization.GetMethod("GetICUVersion", BindingFlags.NonPublic | BindingFlags.Static); - Assert.NotNull(methodInfo); + ProcessStartInfo psi = new ProcessStartInfo() { UseShellExecute = false }; + psi.Environment.Add("DOTNET_SYSTEM_GLOBALIZATION_APPLOCALICU", "68.2.0.9"); - // Assert the ICU version 0x44020009 is 68.2.0.9 - Assert.Equal(0x44020009, (int)methodInfo.Invoke(null, null)); + RemoteExecutor.Invoke(() => + { + Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib"); + Assert.NotNull(interopGlobalization); - // Now call globalization API to ensure the binding working without any problem. - Assert.Equal(-1, CultureInfo.GetCultureInfo("en-US").CompareInfo.Compare("sample\u0000", "Sample\u0000", CompareOptions.IgnoreSymbols)); + MethodInfo? methodInfo = interopGlobalization.GetMethod("GetICUVersion", BindingFlags.NonPublic | BindingFlags.Static); + Assert.NotNull(methodInfo); + + // Assert the ICU version 0x44020009 is 68.2.0.9 + Assert.Equal(0x44020009, (int)methodInfo.Invoke(null, null)); + + // Now call globalization API to ensure the binding working without any problem. + Assert.Equal(-1, CultureInfo.GetCultureInfo("en-US").CompareInfo.Compare("sample\u0000", "Sample\u0000", CompareOptions.IgnoreSymbols)); + }, new RemoteInvokeOptions { StartInfo = psi }).Dispose(); } } } From 563ae4b3fc7dc80ea96c5caa526dc503f8bd568c Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sun, 16 Oct 2022 12:01:46 -0700 Subject: [PATCH 6/7] address the feedback --- .../TestUtilities/System/PlatformDetection.Unix.cs | 2 +- .../tests/IcuAppLocal/IcuAppLocal.Tests.csproj | 4 ++-- .../tests/IcuAppLocal/IcuAppLocal.cs | 14 ++++++-------- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Unix.cs b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Unix.cs index 33c4f5f9304230..256a3075b41295 100644 --- a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Unix.cs +++ b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Unix.cs @@ -14,7 +14,7 @@ public static partial class PlatformDetection // do it in a way that failures don't cascade. // - private static bool IsLinux => RuntimeInformation.IsOSPlatform(OSPlatform.Linux); + public static bool IsLinux => RuntimeInformation.IsOSPlatform(OSPlatform.Linux); public static bool IsOpenSUSE => IsDistroAndVersion("opensuse"); public static bool IsUbuntu => IsDistroAndVersion("ubuntu"); public static bool IsDebian => IsDistroAndVersion("debian"); diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj index 28d8c4b6e9b649..3b9800574da644 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.Tests.csproj @@ -16,8 +16,8 @@ The reason is, if we enable ICU app-local here, this test will compile and run on all supported OSs even the ICU NuGet package not have native bits support such OSs. Note, it doesn't matter if we have test case conditioned to not run on such OSs, because - the test has to start running first before filtering the test cases and teh globalization - code will run at that time and will fail fast at that time. + the test has to start running first before filtering the test cases and the globalization + code will run and fail fast at that time. --> diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs index 586a020b20ae1b..92048611dd320b 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs @@ -10,15 +10,13 @@ namespace System.Globalization.Tests { public class IcuAppLocalTests { + private static bool SupportsIcuPackageDownload => RemoteExecutor.IsSupported && + ((PlatformDetection.IsWindows && !PlatformDetection.IsArmProcess) || + (PlatformDetection.IsLinux && (PlatformDetection.IsX64Process || PlatformDetection.IsArm64Process) && + !PlatformDetection.IsAlpine && !PlatformDetection.IsLinuxBionic)); - private static bool SupportIcuPackageDownload => PlatformDetection.Is64BitProcess && - PlatformDetection.IsNotOSX && - PlatformDetection.IsNotMobile && - !PlatformDetection.IsAlpine && - !PlatformDetection.IsLinuxBionic && - RemoteExecutor.IsSupported; - [ConditionalFact(nameof(SupportIcuPackageDownload))] + [ConditionalFact(nameof(SupportsIcuPackageDownload))] public void TestIcuAppLocal() { // We define this switch dynamically during the runtime using RemoteExecutor. @@ -26,7 +24,7 @@ public void TestIcuAppLocal() // on all supported OSs even the ICU NuGet package not have native bits support such OSs. // Note, it doesn't matter if we have test case conditioned to not run on such OSs, because // the test has to start running first before filtering the test cases and the globalization - // code will run at that time and will fail fast at that time. + // code will run and fail fast at that time. ProcessStartInfo psi = new ProcessStartInfo() { UseShellExecute = false }; psi.Environment.Add("DOTNET_SYSTEM_GLOBALIZATION_APPLOCALICU", "68.2.0.9"); From 66b46f521a8a23d50c15da3d9ccce9a7fd6a25db Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sun, 16 Oct 2022 13:26:07 -0700 Subject: [PATCH 7/7] More feedback addressing --- .../System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs index 92048611dd320b..af8d59d7225f42 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs @@ -26,7 +26,7 @@ public void TestIcuAppLocal() // the test has to start running first before filtering the test cases and the globalization // code will run and fail fast at that time. - ProcessStartInfo psi = new ProcessStartInfo() { UseShellExecute = false }; + ProcessStartInfo psi = new ProcessStartInfo(); psi.Environment.Add("DOTNET_SYSTEM_GLOBALIZATION_APPLOCALICU", "68.2.0.9"); RemoteExecutor.Invoke(() =>