From b10a03eb4e1041c33c748bd82819feb8903a342b Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Fri, 28 May 2021 17:20:03 -0500 Subject: [PATCH 1/5] Fix InvariantGlobalization=false in a trimmed app. Move the LoadICU logic into a static ctor, so it runs early in the process, but not as part of querying the GlobalizationMode.Invariant property. This allows for LoadICU to still be invoked, even when the app is trimmed and GlobalizationMode.Invariant is hard-coded by the linker. While I was changing this code, I also removed the workaround in Browser builds for swallowing errors being returned from LoadICU. Fix #49073 Fix #49391 --- .../ILLink/ILLink.Substitutions.Shared.xml | 2 +- .../Globalization/GlobalizationMode.Unix.cs | 53 +++++++++---------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml b/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml index 67f7b46489ab2f..b85c89711cf6d9 100644 --- a/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml +++ b/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml @@ -6,7 +6,7 @@ - + diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs index 3ad23b92f4a1ba..8a3a3279853538 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs @@ -7,42 +7,41 @@ internal static partial class GlobalizationMode { private static partial class Settings { - internal static readonly bool Invariant = GetGlobalizationInvariantMode(); - } - - internal static bool Invariant => Settings.Invariant; - - internal static bool UseNls => false; + internal static bool Invariant { get; } = GetInvariantSwitchValue(); - private static bool GetGlobalizationInvariantMode() - { - bool invariantEnabled = GetInvariantSwitchValue(); - if (!invariantEnabled) + /// + /// Load ICU (when not in Invariant mode) in a static cctor to ensure it is loaded early in the process. + /// Other places, e.g. CompareInfo.GetSortKey, rely on ICU already being loaded before they are called. + /// + static Settings() { - if (TryGetAppLocalIcuSwitchValue(out string? icuSuffixAndVersion)) - { - LoadAppLocalIcu(icuSuffixAndVersion); - } - else + if (!Invariant) { - int loaded = LoadICU(); - if (loaded == 0 && !OperatingSystem.IsBrowser()) + if (TryGetAppLocalIcuSwitchValue(out string? icuSuffixAndVersion)) { - // This can't go into resources, because a resource lookup requires globalization, which requires ICU - string message = "Couldn't find a valid ICU package installed on the system. " + - "Please install libicu using your package manager and try again. " + - "Alternatively you can set the configuration flag System.Globalization.Invariant to true if you want to run with no globalization support. " + - "Please see https://aka.ms/dotnet-missing-libicu for more information."; - Environment.FailFast(message); + LoadAppLocalIcu(icuSuffixAndVersion); + } + else + { + int loaded = LoadICU(); + if (loaded == 0) + { + // This can't go into resources, because a resource lookup requires globalization, which requires ICU + string message = "Couldn't find a valid ICU package installed on the system. " + + "Please install libicu using your package manager and try again. " + + "Alternatively you can set the configuration flag System.Globalization.Invariant to true if you want to run with no globalization support. " + + "Please see https://aka.ms/dotnet-missing-libicu for more information."; + Environment.FailFast(message); + } } - - // fallback to Invariant mode if LoadICU failed (Browser). - return loaded == 0; } } - return invariantEnabled; } + internal static bool Invariant => Settings.Invariant; + + internal static bool UseNls => false; + private static void LoadAppLocalIcuCore(ReadOnlySpan version, ReadOnlySpan suffix) { From 3a33f797e82a4920494ba2fb96f8d3de40396487 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Fri, 28 May 2021 18:04:26 -0500 Subject: [PATCH 2/5] Add trimming tests for InvariantGlobalization --- .../Globalization/GlobalizationMode.Unix.cs | 4 --- .../GlobalizationMode.Windows.cs | 4 --- .../System/Globalization/GlobalizationMode.cs | 3 +++ .../InvariantGlobalizationFalse.cs | 26 +++++++++++++++++++ .../InvariantGlobalizationTrue.cs | 26 +++++++++++++++++++ .../System.Runtime.TrimmingTests.proj | 6 +++++ 6 files changed, 61 insertions(+), 8 deletions(-) create mode 100644 src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationFalse.cs create mode 100644 src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs index 8a3a3279853538..efdd2ade34ee11 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs @@ -7,8 +7,6 @@ internal static partial class GlobalizationMode { private static partial class Settings { - internal static bool Invariant { get; } = GetInvariantSwitchValue(); - /// /// Load ICU (when not in Invariant mode) in a static cctor to ensure it is loaded early in the process. /// Other places, e.g. CompareInfo.GetSortKey, rely on ICU already being loaded before they are called. @@ -38,8 +36,6 @@ static Settings() } } - internal static bool Invariant => Settings.Invariant; - internal static bool UseNls => false; private static void LoadAppLocalIcuCore(ReadOnlySpan version, ReadOnlySpan suffix) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs index 590aae9b3bdc71..e81ef6234e7b9e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs @@ -5,10 +5,6 @@ namespace System.Globalization { internal static partial class GlobalizationMode { - // Order of these properties in Windows matter because GetUseIcuMode is dependent on Invariant. - // So we need Invariant to be initialized first. - internal static bool Invariant { get; } = GetInvariantSwitchValue(); - internal static bool UseNls { get; } = !Invariant && (AppContextConfigHelper.GetBooleanConfig("System.Globalization.UseNls", "DOTNET_SYSTEM_GLOBALIZATION_USENLS") || !LoadIcu()); diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs index e68195ea569850..59526a997d4013 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs @@ -11,8 +11,11 @@ internal static partial class GlobalizationMode private static partial class Settings { internal static readonly bool PredefinedCulturesOnly = AppContextConfigHelper.GetBooleanConfig("System.Globalization.PredefinedCulturesOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY"); + internal static bool Invariant { get; } = GetInvariantSwitchValue(); } + internal static bool Invariant => Settings.Invariant; + internal static bool PredefinedCulturesOnly => !Invariant && Settings.PredefinedCulturesOnly; private static bool GetInvariantSwitchValue() => diff --git a/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationFalse.cs b/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationFalse.cs new file mode 100644 index 00000000000000..fbdafa45922afc --- /dev/null +++ b/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationFalse.cs @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Globalization; +using System.Threading; + +/// +/// Ensures setting InvariantGlobalization = false still works in a trimmed app. +/// +class Program +{ + static int Main(string[] args) + { + // since we are using Invariant GlobalizationMode = false, setting the culture matters. + // The app will always use the current culture, so in the Turkish culture, 'i' ToUpper will NOT be "I" + Thread.CurrentThread.CurrentCulture = new CultureInfo("tr-TR"); + if ("i".ToUpper() == "I") + { + // 'i' ToUpper was "I", but shouldn't be in the Turkish culture, so fail + return -1; + } + + return 100; + } +} diff --git a/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs b/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs new file mode 100644 index 00000000000000..621137cd481402 --- /dev/null +++ b/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Globalization; +using System.Threading; + +/// +/// Ensures setting InvariantGlobalization = true still works in a trimmed app. +/// +class Program +{ + static int Main(string[] args) + { + // since we are using Invariant GlobalizationMode = true, setting the culture doesn't matter. + // The app will always use Invariant mode, so even in the Turkish culture, 'i' ToUpper will be "I" + Thread.CurrentThread.CurrentCulture = new CultureInfo("tr-TR"); + if ("i".ToUpper() != "I") + { + // 'i' ToUpper was not "I", so fail + return -1; + } + + return 100; + } +} diff --git a/src/libraries/System.Runtime/tests/TrimmingTests/System.Runtime.TrimmingTests.proj b/src/libraries/System.Runtime/tests/TrimmingTests/System.Runtime.TrimmingTests.proj index fd556a37d675ac..02b4882782e967 100644 --- a/src/libraries/System.Runtime/tests/TrimmingTests/System.Runtime.TrimmingTests.proj +++ b/src/libraries/System.Runtime/tests/TrimmingTests/System.Runtime.TrimmingTests.proj @@ -11,6 +11,12 @@ + + --feature System.Globalization.Invariant false + + + --feature System.Globalization.Invariant true + + + + diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs index 59526a997d4013..5afa2a4e2b9792 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs @@ -8,12 +8,16 @@ namespace System.Globalization { internal static partial class GlobalizationMode { + // split from GlobalizationMode so the whole class can be trimmed when Invariant=true. private static partial class Settings { internal static readonly bool PredefinedCulturesOnly = AppContextConfigHelper.GetBooleanConfig("System.Globalization.PredefinedCulturesOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY"); internal static bool Invariant { get; } = GetInvariantSwitchValue(); } + // Note: Invariant=true and Invariant=false are substituted at different levels in the ILLink.Substitutions file. + // This allows for the whole Settings nested class to be trimmed when Invariant=true, and allows for the Settings + // static cctor (on Unix) to be preserved when Invariant=false. internal static bool Invariant => Settings.Invariant; internal static bool PredefinedCulturesOnly => !Invariant && Settings.PredefinedCulturesOnly; diff --git a/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs b/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs index 621137cd481402..7daa557239f3d6 100644 --- a/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs +++ b/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs @@ -3,6 +3,7 @@ using System; using System.Globalization; +using System.Reflection; using System.Threading; /// @@ -21,6 +22,34 @@ static int Main(string[] args) return -1; } + // Ensure the internal GlobalizationMode class is trimmed correctly + Type globalizationMode = GetCoreLibType("System.Globalization.GlobalizationMode"); + const BindingFlags allStatics = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static; + foreach (MemberInfo member in globalizationMode.GetMembers(allStatics)) + { + // properties and their backing getter methods are OK + if (member is PropertyInfo || member.Name.StartsWith("get_")) + { + continue; + } + + if (OperatingSystem.IsWindows()) + { + // Windows still contains a static cctor and a backing field for UseNls + if (member is ConstructorInfo || (member is FieldInfo field && field.Name.Contains("UseNls"))) + { + continue; + } + } + + // Some unexpected member was left on GlobalizationMode, fail + Console.WriteLine($"Member '{member.Name}' was not trimmed from GlobalizationMode, but should have been."); + return -2; + } + return 100; } + + private static Type GetCoreLibType(string name) => + typeof(object).Assembly.GetType(name, throwOnError: true); } From 0c3b56c43c6fec3667a957f2a66d1284251acf13 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Tue, 8 Jun 2021 10:06:35 -0500 Subject: [PATCH 5/5] Add checks for all mobile platforms --- .../src/System/Globalization/GlobalizationMode.Unix.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs index f9f9088393333e..2cc6fd74b8201c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs @@ -33,7 +33,8 @@ static Settings() private static string GetIcuLoadFailureMessage() { // These strings can't go into resources, because a resource lookup requires globalization, which requires ICU - if (OperatingSystem.IsBrowser() || OperatingSystem.IsIOS() || OperatingSystem.IsAndroid()) + if (OperatingSystem.IsBrowser() || OperatingSystem.IsAndroid() || + OperatingSystem.IsIOS() || OperatingSystem.IsTvOS() || OperatingSystem.IsWatchOS() || OperatingSystem.IsMacCatalyst()) { return "Unable to load required ICU Globalization data. Please see https://aka.ms/dotnet-missing-libicu for more information"; }