Skip to content

Conversation

@safern
Copy link
Member

@safern safern commented May 27, 2020

This adds AppLocalIcu switch support on Mono. I also refactored GlobalizationMode to share the sources in between Mono and CoreCLR to avoid duplicating all the AppLocal and UseNls switches logic.

cc: @jkotas @thaystg @akoeplinger @tarekgh @ericstj

@ghost
Copy link

ghost commented May 27, 2020

Tagging subscribers to this area: @tarekgh, @safern, @krwq
Notify danmosemsft if you want to be subscribed.

{
private static bool GetInvariantSwitchValue() =>
GetSwitchValue("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT");

Copy link
Contributor

Choose a reason for hiding this comment

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

The linker will need a bool method which it can transform to false to disable ICU support at app build time.

Copy link
Member

@jkotas jkotas May 27, 2020

Choose a reason for hiding this comment

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

Does the existing GlobalizationMode.get_Invariant property work for this? (It would need to be tranformed to true.)

Copy link
Contributor

Choose a reason for hiding this comment

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

It does.

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, we're still doing something similar:

bool invariantEnabled = GetInvariantSwitchValue();
if (!invariantEnabled)
{
if (TryGetAppLocalIcuSwitchValue(out string? icuSuffixAndVersion))
{
LoadAppLocalIcu(icuSuffixAndVersion, suffixWithSeparator: true);
}
else if (Interop.Globalization.LoadICU() == 0)
{
string message = "Couldn't find a valid ICU package installed on the system. " +
"Set the configuration flag System.Globalization.Invariant to true if you want to run with no globalization support.";
Environment.FailFast(message);
}
}
return invariantEnabled;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@vargaz does this look good, or is there any action I should take?

Copy link
Member Author

Choose a reason for hiding this comment

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

I merged this, but if something else need to happen here, I'm happy to put up a follow up PR.

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Runtime changes look good to me, thanks!

@safern
Copy link
Member Author

safern commented May 27, 2020

@tarekgh @jkotas S.P.CoreLib changes look good?

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

@tarekgh @jkotas S.P.CoreLib changes look good?

Yes

@safern safern merged commit a64f2ec into dotnet:master May 27, 2020
@safern safern deleted the AddIcuAppLocalMono branch May 27, 2020 23:37
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants