Fix InvariantGlobalization=false in a trimmed app #53453
Conversation
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 dotnet#49073 Fix dotnet#49391
|
Tagging subscribers to this area: @tarekgh, @safern Issue DetailsMove 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 While I was changing this code, I also removed the workaround in Browser builds for swallowing errors being returned from LoadICU.
|
src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationFalse.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Outdated
Show resolved
Hide resolved
| @@ -6,7 +6,7 @@ | |||
| <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> | |||
| <type fullname="System.Globalization.GlobalizationMode"> | |||
| <type fullname="System.Globalization.GlobalizationMode/Settings"> | |||
There was a problem hiding this comment.
This effectively removes Invariant = true dependencies trimming. I think it'd be better to substitute at the current level.
There was a problem hiding this comment.
This effectively removes Invariant = true dependencies trimming.
Why?
There was a problem hiding this comment.
I tested it and the dead branches still get trimmed. Why do you think this removes that?
Also, you can’t substitute this at the higher level because then the Settings class wouldn’t be touched and it’s cctor won’t be run.
And there were concerns in the issue about putting the cctor on GlobalizationMode itself because calling UseNls would force it to run. So splitting the cctor off in a separate class was proposed.
There was a problem hiding this comment.
Also, you can’t substitute this at the higher level because then the Settings class wouldn’t be touched and it’s cctor won’t be run.
I think that's what we want for Invariant = true. I think Settings for that mode is redundant.
There was a problem hiding this comment.
I'm not sure how to resolve this comment, plus address all the concerns raised in #49073 (comment). Can you elaborate on what you want here?
Do you want me to split the ILLink.Substitutions.xml apart so Invariant = true and Invariant = false look different? For example:
<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>That doesn't seem like an ideal situation IMO.
I don't think your original concern is a problem. Can you explain what are the disadvantages to the current approach I am using?
| { | ||
| if (TryGetAppLocalIcuSwitchValue(out string? icuSuffixAndVersion)) | ||
| if (!Invariant) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Outdated
Show resolved
Hide resolved
- Split the substitutions of GlobalizationMode.Invariant between true and false - Add a trimming test that ensures Invariant=true trims everything it is supposed to
|
@marek-safar - I believe I have addressed your concerns with the latest changes. PTAL. |
| { | ||
| 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.IsIOS() || OperatingSystem.IsAndroid()) |
There was a problem hiding this comment.
I'd go with the simple message everywhere. Otherwise, you would also need to check for tvOS, MacCatalyst, macOS.
There was a problem hiding this comment.
I'll add the check for tvOS and MacCatalyst. My concern with using the "simple" message is that for "desktop" scenarios like Linux and macOS, it would be less informative than it is today.
There was a problem hiding this comment.
I don't think macOS users will know what to do with the message and in the case of macOS UI apps it makes even less sense if the user somehow manages to see it.
There was a problem hiding this comment.
I believe it does make sense for macOS users. We require they have ICU on the machine. We don't bring ICU with .NET on macOS.
There was a problem hiding this comment.
@marek-safar - I'm going to merge this for now. If we decide to do something different here for macOS, we can open a new issue/PR for it. But, as-is, this change is keeping the current/existing behavior for macOS.
ghost
locked as resolved and limited conversation to collaborators
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
falseby 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