From ef7b093ceae298c3a434df4a3b6be6547885e90d Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 14 Oct 2022 14:41:51 -0700 Subject: [PATCH 1/8] 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 45aa980fbfa2d77bd971d4fa09e4f8e4828ebbb5 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 14 Oct 2022 18:44:56 -0700 Subject: [PATCH 2/8] 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 17996573861106c27a08534fe3047640f545a73e Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sat, 15 Oct 2022 09:51:36 -0700 Subject: [PATCH 3/8] 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 67e3b0a7ea45e74ffaefec49f0430098bfedb4fb Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sat, 15 Oct 2022 12:48:20 -0700 Subject: [PATCH 4/8] 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 1ca32fb6c3823f7ede608c4bbb891acc2ab5f081 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sun, 16 Oct 2022 10:19:22 -0700 Subject: [PATCH 5/8] 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 130c0b9275d98274f2030a5aa6db9eefa1491514 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sun, 16 Oct 2022 12:01:46 -0700 Subject: [PATCH 6/8] 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 8205afdde967541b16035678c213ee5de8e934aa Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sun, 16 Oct 2022 13:26:07 -0700 Subject: [PATCH 7/8] 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(() => From 66d47992ee79300760b1feeb2fdc10184534f2ba Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 17 Oct 2022 13:32:27 -0700 Subject: [PATCH 8/8] Fix the test --- .../System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs index af8d59d7225f42..aa0a0df938c407 100644 --- a/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs +++ b/src/libraries/System.Globalization/tests/IcuAppLocal/IcuAppLocal.cs @@ -31,6 +31,9 @@ public void TestIcuAppLocal() RemoteExecutor.Invoke(() => { + // Ensure initializing globalization code before checking the ICU version. + CultureInfo ci = CultureInfo.GetCultureInfo("en-US"); + Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib"); Assert.NotNull(interopGlobalization); @@ -41,7 +44,7 @@ public void TestIcuAppLocal() 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)); + Assert.Equal(-1, ci.CompareInfo.Compare("sample\u0000", "Sample\u0000", CompareOptions.IgnoreSymbols)); }, new RemoteInvokeOptions { StartInfo = psi }).Dispose(); } }