Allow restricting cultures creation with any arbitrary names in Globalization Invariant Mode #54247

tarekgh · 2021-06-15T22:15:24Z

This change addresses the second part of #43774

This change is forcing to throw exception when creating any culture other than Invariant culture when enabling the Globalization Invariant Mode. This will allow easier diagnosis of the failures when turning on the Globalization Invariant Mode.
When turning on the Invariant globalization mode the config switch System.Globalization.PredefinedCulturesOnly will be automatically turned on. If anyone for any reason want to get back to the old behavior which is allowing creating any cultures by providing Invariant culture data, can do that by explicitly turning off the System.Globalization.PredefinedCulturesOnly switch.

The breaking change doc is tracked by Restrict Cultures Creation in Globalization Invariant Mode issue.

ghost · 2021-06-15T22:15:33Z

Tagging subscribers to this area: @tarekgh, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

This change addresses the second part of #43774

This change is allowing restricting creating cultures with any names different than Invariant culture name (which is empty string). The is opt-in behavior which can get by using the config switch System.Globalization.AllowInvariantCultureOnly or environment variable DOTNET_SYSTEM_GLOBALIZATION_INVARIANT_CULTURE_ONLY.

Anyone enables this mode will force any code creating a culture with names different than Invariant culture name "" to throw exception.

Author:	tarekgh
Assignees:	-
Labels:	`area-System.Globalization`
Milestone:	-

tarekgh · 2021-06-16T16:16:53Z

CC @safern

src/libraries/System.Globalization/tests/CultureInfo/GetCultureInfo.cs

safern

LGTM. It seems like there are some Globalization trimming tests failing.

src/libraries/System.Globalization/tests/CultureInfo/GetCultureInfo.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs

src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml

ghost · 2021-06-24T17:36:29Z

Tagging @dotnet/compat for awareness of the breaking change.

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

src/libraries/System.Globalization/tests/CultureInfo/GetCultureInfo.cs

danmoseley · 2021-06-24T20:04:02Z

(Signed off but someone else should check the linker/trimming stuff.)

tarekgh · 2021-06-30T23:40:35Z

I tried setting the environment variable there, but it looks this didn't affect the test.

  [wasm test] info: Running v8 --expose_wasm runtime.js -- --setenv=DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY=false --setenv=XHARNESS_DISABLE_COLORED_OUTPUT=true --run invariant_True.dll
  [wasm test]
  [wasm test]
  [wasm test] fail: console.error: Error: System.Globalization.CultureNotFoundException: Only the invariant culture is supported in globalization-invariant mode. See https://aka.ms/GlobalizationInvariantMode for more information. (Parameter 'name')
  [wasm test]
  [wasm test] fail: es-ES is an invalid culture identifier.
  [wasm test]

The last idea I have is to force the value of PredefinedCulturesOnly in the test to false through the reflection. @marek-safar @lewing @radical if you have better idea please let me know.

radical · 2021-07-01T00:03:20Z

Can you share the change you made, and binlog for the test?

tarekgh · 2021-07-01T00:54:43Z

@radical the test change is just creating the envVars and passing it to RunAndTestWasmApp

            Dictionary<string, string> envVars = new();
            envVars.Add("DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY", "false");

            RunAndTestWasmApp(buildArgs, expectedExitCode: 42,
                                test: output => Assert.Contains(expectedOutputString, output), host: host, id: id, envVars: envVars);

Looking at the logs, I am seeing the test src/tests/BuildWasmApps/Wasm.Build.Tests/InvariantGlobalizationTests.cs will get trimmed during building. Looks the behavior would be defined during building and not during the runtime. if this true, it would be difficult to control that behavior for the newly introduced property PredefinedCulturesOnly as we don't have msbuild property to control that yet.

I guess the best to do now, is to modify the test to catch the exception and use Invariant culture at that time. I'll try that and see how it goes.

/cc @eerhardt

tarekgh · 2021-07-01T01:22:19Z

I fixed the browser test, hopefully this will be last failure :-)

stephentoub · 2021-07-01T02:55:01Z

src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs

            internal static bool Invariant { get; } = GetInvariantSwitchValue();
+            internal static readonly bool PredefinedCulturesOnly = AppContextConfigHelper.GetBooleanConfig("System.Globalization.PredefinedCulturesOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY", Invariant);


Why are Invariant and PredefinedCulturesOnly not symmetric in their design? e.g. I was expecting to see:

internal static bool Invariant { get; } = GetInvariantSwitchValue(); internal static bool PredefinedCulturesOnly { get; } = GetPredefinedCulturesOnlySwitchValue();

Because we decided to not introduce a new config switch with the new Invariant behavior. @jkotas suggested to use the PredefinedCulturesOnly switch which make a lot of sense. Now the default Invariant behavior is to throw for any non-Invariant culture. For that we have to have PredefinedCulturesOnly be true be default for Invariant. and be false by default for non-Invariant mode. The only difference for PredefinedCulturesOnly is the default value when it is not specified.

I was asking a style question. GetPredefinedCulturesOnlySwitchValue() would return AppContextConfigHelper.GetBooleanConfig("System.Globalization.PredefinedCulturesOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY", Invariant);, exactly what you're already initializing it to.

@stephentoub I am going to merge this change soon. please let me know if you have any concern.

I just don't see why the shape of these is different. But it's not a blocker, it's just inconsistent.

Sorry my reply crossed your reply. This is how it is done to help the linker for trimming purpose I guess. look at @marek-safar change bc27d49 which introduced that.

I think this done this way to allow the linker to trim GetInvariantSwitchValue when Invariant mode is one. Having GetPredefinedCulturesOnlySwitchValue will require more work from the linker to trim it too. @eerhardt or @marek-safar should have definitive answer here.

I think this done this way to allow the linker to trim GetInvariantSwitchValue when Invariant mode is one.

I don't believe this is the case. I think it is inconsistent because this code has been refactored multiple times, and no one has made it consistent.

Having GetPredefinedCulturesOnlySwitchValue will require more work from the linker to trim it too.

I don't believe this is true either. Alternatively, the opposite approach could be taken to make the code look consistent:

internal static bool Invariant { get; } = AppContextConfigHelper.GetBooleanConfig("System.Globalization.Invariant", "DOTNET_SYSTEM_GLOBALIZATION_INVARIANT"); internal static bool PredefinedCulturesOnly { get; } = AppContextConfigHelper.GetBooleanConfig("System.Globalization.PredefinedCulturesOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY", Invariant);

I'll try to submit a PR to make this consistent.

tarekgh · 2021-07-01T17:23:12Z

I opened the issue #55026 to track the unrelated JIT test failure in Android x64.
The failures in System.Net tests are unrelated and already tracked by #54778

This is an attempt to address #47112 based on Egor's suggestion. The changes: - set IsInterpreting MSBuild property to 'true' in case of iOS; - made System.Linq.Expressions.ExpressionCreator class internal static in order not to introduce new public type; otherwise it throws Type 'System.Linq.Expressions.ExpressionCreator<TDelegate>' does not exist in the reference but it does exist in the implementation. -updated the iOS simulator functional test for AOT to verify the fix - added PlatformDetection.IsLinqExpressionsBuiltWithIsInterpretingOnly which checks whether the System.Linq.Expressions is built with IsInterpreting property set to true. To do so, it uses reflection to verify the Expression.Accept method doesn't exist. - disabled some failing library tests using ActiveIssue + PlatformDetection. - updated the invariant functional test for the iOS simulator (b/c of Allow restricting cultures creation with any arbitrary names in Globalization Invariant Mode #54247) Co-authored-by: Stephen Toub <stoub@microsoft.com>

The following commit changed the behavior to throw `CultureNotFoundException` when creating cultures in invariant mode. ``` commit 04dac7b Author: Tarek Mahmoud Sayed <tarekms@microsoft.com> Date: Thu Jul 1 11:55:05 2021 -0700 Allow restricting cultures creation with any arbitrary names in Globalization Invariant Mode (dotnet#54247) ``` This commit updates the corresponding test in `Wasm.Build.Tests` to handle that explicitly. Fixes dotnet#55838

The following commit changed the behavior to throw `CultureNotFoundException` when creating cultures in invariant mode. ``` commit 04dac7b Author: Tarek Mahmoud Sayed <tarekms@microsoft.com> Date: Thu Jul 1 11:55:05 2021 -0700 Allow restricting cultures creation with any arbitrary names in Globalization Invariant Mode (#54247) ``` This commit updates the corresponding test in `Wasm.Build.Tests` to handle that explicitly. Fixes #55838

tarekgh · 2021-09-30T16:23:58Z

@ericstj the description of this issue already referenced the breaking change issue dotnet/docs#24849 and it is already merged to the docs https://docs.microsoft.com/en-us/dotnet/core/compatibility/globalization/6.0/culture-creation-invariant-mode

ericstj · 2021-09-30T16:33:55Z

Thanks @tarekgh, removing that label will get it off my list 👍

tarekgh requested review from marek-safar and eerhardt June 15, 2021 22:15

dotnet-issue-labeler bot added the area-System.Globalization label Jun 15, 2021

tarekgh closed this Jun 16, 2021

tarekgh reopened this Jun 16, 2021

safern reviewed Jun 16, 2021

View reviewed changes

src/libraries/System.Globalization/tests/CultureInfo/GetCultureInfo.cs Outdated Show resolved Hide resolved

safern approved these changes Jun 16, 2021

View reviewed changes

eerhardt reviewed Jun 16, 2021

View reviewed changes

src/libraries/System.Globalization/tests/CultureInfo/GetCultureInfo.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 16, 2021

View reviewed changes

src/libraries/System.Globalization/tests/CultureInfo/GetCultureInfo.cs Show resolved Hide resolved

eerhardt reviewed Jun 16, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs Outdated Show resolved Hide resolved

dotnet deleted a comment from azure-pipelines bot Jun 17, 2021

eerhardt reviewed Jun 22, 2021

View reviewed changes

src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs Outdated Show resolved Hide resolved

tarekgh commented Jun 24, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml Show resolved Hide resolved

tarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jun 24, 2021

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jun 24, 2021

danmoseley reviewed Jun 24, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

danmoseley reviewed Jun 24, 2021

View reviewed changes

src/libraries/System.Globalization/tests/CultureInfo/GetCultureInfo.cs Outdated Show resolved Hide resolved

danmoseley approved these changes Jun 24, 2021

View reviewed changes

Fix the browser AOT tests

8c503e1

tarekgh force-pushed the GlobalizationInvariantRestrictedMode branch from 62f2c4f to 8c503e1 Compare July 1, 2021 01:20

stephentoub reviewed Jul 1, 2021

View reviewed changes

tarekgh mentioned this pull request Jul 1, 2021

Initial Support for PredefinedCultureOnly property dotnet/sdk#18672

Merged

tarekgh merged commit 04dac7b into dotnet:main Jul 1, 2021

tarekgh deleted the GlobalizationInvariantRestrictedMode branch July 1, 2021 18:55

tarekgh mentioned this pull request Jul 1, 2021

Make GlobalizationMode code consistent #55039

Merged

MaximLipnin mentioned this pull request Jul 6, 2021

Make LambdaCompiler prefer interpretation to compilation on iOS #54970

Merged

AntonLapounov mentioned this pull request Jul 24, 2021

Change reading time zone display names #56171

Merged

karelz mentioned this pull request Jul 28, 2021

[WinHttpHandler] Long running test: ResponseHeadersRead_SynchronizationContextNotUsedByHandler on Win7/Win8 #54034

Closed

ghost locked as resolved and limited conversation to collaborators Jul 31, 2021

ericstj added this to the 6.0.0 milestone Sep 30, 2021

ericstj assigned tarekgh Sep 30, 2021

tarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow restricting cultures creation with any arbitrary names in Globalization Invariant Mode #54247

Allow restricting cultures creation with any arbitrary names in Globalization Invariant Mode #54247

tarekgh commented Jun 15, 2021 •

edited

Loading

ghost commented Jun 15, 2021

tarekgh commented Jun 16, 2021

safern left a comment

ghost commented Jun 24, 2021

danmoseley commented Jun 24, 2021

tarekgh commented Jun 30, 2021

radical commented Jul 1, 2021

tarekgh commented Jul 1, 2021 •

edited

Loading

tarekgh commented Jul 1, 2021

stephentoub Jul 1, 2021

tarekgh Jul 1, 2021 •

edited

Loading

stephentoub Jul 1, 2021

tarekgh Jul 1, 2021

stephentoub Jul 1, 2021

tarekgh Jul 1, 2021

tarekgh Jul 1, 2021

eerhardt Jul 1, 2021

tarekgh Jul 1, 2021

tarekgh commented Jul 1, 2021 •

edited

Loading

tarekgh commented Sep 30, 2021

ericstj commented Sep 30, 2021

		internal static bool Invariant { get; } = GetInvariantSwitchValue();
		internal static readonly bool PredefinedCulturesOnly = AppContextConfigHelper.GetBooleanConfig("System.Globalization.PredefinedCulturesOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY", Invariant);

Allow restricting cultures creation with any arbitrary names in Globalization Invariant Mode #54247

Allow restricting cultures creation with any arbitrary names in Globalization Invariant Mode #54247

Conversation

tarekgh commented Jun 15, 2021 • edited Loading

ghost commented Jun 15, 2021

tarekgh commented Jun 16, 2021

safern left a comment

Choose a reason for hiding this comment

ghost commented Jun 24, 2021

danmoseley commented Jun 24, 2021

tarekgh commented Jun 30, 2021

radical commented Jul 1, 2021

tarekgh commented Jul 1, 2021 • edited Loading

tarekgh commented Jul 1, 2021

Choose a reason for hiding this comment

tarekgh Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Jul 1, 2021 • edited Loading

tarekgh commented Sep 30, 2021

ericstj commented Sep 30, 2021

tarekgh commented Jun 15, 2021 •

edited

Loading

tarekgh commented Jul 1, 2021 •

edited

Loading

tarekgh Jul 1, 2021 •

edited

Loading

tarekgh commented Jul 1, 2021 •

edited

Loading