Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix InvariantGlobalization=false in a trimmed app #53453

Merged
merged 5 commits into from
Jun 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@
<method signature="System.Boolean IsEnabled(System.Diagnostics.Tracing.EventLevel,System.Diagnostics.Tracing.EventKeywords)" body="stub" value="false" />
<method signature="System.Boolean IsEnabled(System.Diagnostics.Tracing.EventLevel,System.Diagnostics.Tracing.EventKeywords,System.Diagnostics.Tracing.EventChannel)" body="stub" value="false" />
</type>
<!-- Note: Invariant=true and Invariant=false are substituted at different levels. 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. -->
<type fullname="System.Globalization.GlobalizationMode">
<method signature="System.Boolean get_Invariant()" body="stub" value="true" feature="System.Globalization.Invariant" featurevalue="true" />
</type>
<type fullname="System.Globalization.GlobalizationMode/Settings">
<method signature="System.Boolean get_Invariant()" body="stub" value="false" feature="System.Globalization.Invariant" featurevalue="false" />
</type>
<type fullname="System.LocalAppContextSwitches">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,42 +7,49 @@ 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;
/// <summary>
/// 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.
/// </summary>
static Settings()
{
if (!Invariant)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of mixing static ctor with static fields initializers. If you keep the substitution at higher level you could better control when GetInvariantSwitchValue is called

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the concern here? The static field initializers will run at the beginning of the static cctor, just like if you had instance field initializers and a ctor.

{
if (TryGetAppLocalIcuSwitchValue(out string? icuSuffixAndVersion))
{
LoadAppLocalIcu(icuSuffixAndVersion);
}
else
{
int loaded = LoadICU();
if (loaded == 0)
{
Environment.FailFast(GetIcuLoadFailureMessage());
}
}
}
}

private static bool GetGlobalizationInvariantMode()
{
bool invariantEnabled = GetInvariantSwitchValue();
if (!invariantEnabled)
private static string GetIcuLoadFailureMessage()
{
if (TryGetAppLocalIcuSwitchValue(out string? icuSuffixAndVersion))
// These strings can't go into resources, because a resource lookup requires globalization, which requires ICU
if (OperatingSystem.IsBrowser() || OperatingSystem.IsAndroid() ||
OperatingSystem.IsIOS() || OperatingSystem.IsTvOS() || OperatingSystem.IsWatchOS() || OperatingSystem.IsMacCatalyst())
{
LoadAppLocalIcu(icuSuffixAndVersion);
return "Unable to load required ICU Globalization data. Please see https://aka.ms/dotnet-missing-libicu for more information";
}
else
{
int loaded = LoadICU();
if (loaded == 0 && !OperatingSystem.IsBrowser())
{
// 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 "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.";
}
}
return invariantEnabled;
}

internal static bool UseNls => false;

private static void LoadAppLocalIcuCore(ReadOnlySpan<char> version, ReadOnlySpan<char> suffix)
{

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,18 @@ 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;

private static bool GetInvariantSwitchValue() =>
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

/// <summary>
/// Ensures setting InvariantGlobalization = false still works in a trimmed app.
/// </summary>
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;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// 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.Reflection;
using System.Threading;

/// <summary>
/// Ensures setting InvariantGlobalization = true still works in a trimmed app.
/// </summary>
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;
}

// 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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
<TestConsoleAppSourceFiles Include="GenericArraySortHelperTest.cs" />
<TestConsoleAppSourceFiles Include="InheritedAttributeTests.cs" />
<TestConsoleAppSourceFiles Include="InterfacesOnArrays.cs" />
<TestConsoleAppSourceFiles Include="InvariantGlobalizationFalse.cs">
<ExtraTrimmerArgs>--feature System.Globalization.Invariant false</ExtraTrimmerArgs>
</TestConsoleAppSourceFiles>
<TestConsoleAppSourceFiles Include="InvariantGlobalizationTrue.cs">
<ExtraTrimmerArgs>--feature System.Globalization.Invariant true</ExtraTrimmerArgs>
</TestConsoleAppSourceFiles>
<TestConsoleAppSourceFiles Include="StackFrameHelperTest.cs">
<!-- There is a bug with the linker where it is corrupting the pdbs while trimming
causing the framework to not be able to get source line info any longer. This
Expand Down