[wasm] Throw exception if culture data does not exist in icu by tqiu8 · Pull Request #47301 · dotnet/runtime

tqiu8 · 2021-01-21T22:36:56Z

Throw exception if culture data doesn't exist. Currently defaults to en-us.

ghost · 2021-01-21T22:37:02Z

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

Issue Details

Throw exception if culture data doesn't exist. Currently defaults to en-us.

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

Add PredefinedOnly GlobalizationMode Modified tests if Culture Data not found

tarekgh · 2021-01-22T22:12:05Z


 internal static bool UseNls => false;

+ internal static bool PredefinedOnly { get; } = GetSwitchValue("System.Globalization.PredefinedOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_ONLY");


PredefinedOnly [](start = 29, length = 14)
are you adding this for Linux only? looks the calling site will call it even running on Windows. maybe you need to move this to the file GlobalizationMode.cs?

This is for Browser only.

Why we need to restrict this to the browser only? We may enable this flag globally if we need to. Note the calling site of this property is called from shared code which runs with ICU and Nls. Give me a few minutes and I can send you what I propose.
Also, does the browser always runs with Icu?

I guess I just assumed only Browser would need this use case, but I can definitely move to GlobalizationMode.cs. And yes, the browser always runs with icu.

I would suggest to move the checks
runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.Nls.cs
Line 14 in f1ad1b9
if(CultureData.GetLocaleInfoExInt(name,Interop.Kernel32.LOCALE_ICONSTRUCTEDLOCALE)==1)

and
runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.Icu.cs
Line 14 in f1ad1b9
if(!Interop.Globalization.IsPredefinedLocale(name))
to its own methods, something like
[MethodImpl(MethodImplOptions.AggressiveInlining)]internalstaticIcuIsEnsurePredefinedLocaleName(stringname)// may put this in CultureData.Icu.cs{Debug.Assert(!GlobalizationMode.UseNls);if(!Interop.Globalization.IsPredefinedLocale(name)){thrownewCultureNotFoundException(nameof(name),SR.Format(SR.Argument_InvalidPredefinedCultureName,name));}}[MethodImpl(MethodImplOptions.AggressiveInlining)]internalstaticNlsIsEnsurePredefinedLocaleName(stringname)// may put this in CultureData.Nls.cs{Debug.Assert(GlobalizationMode.UseNls);if(CultureData.GetLocaleInfoExInt(name,Interop.Kernel32.LOCALE_ICONSTRUCTEDLOCALE)==1){thrownewCultureNotFoundException(nameof(name),SR.Format(SR.Argument_InvalidPredefinedCultureName,name));}}
Then replace the code here with
if(GlobalizationMode.PredefinedOnly){if(GlobalizationMode.UseNls){NlsIsEnsurePredefinedLocaleName(cultureName);}else{IcuIsEnsurePredefinedLocaleName(cultureName);}}
Last, replace the checks in the links I mentioned above with the new methods calls. and move the code in GlobalizationMode.Unix.cs to GlobalizationMode.cs.

Ok yes that makes sense.

marek-safar · 2021-01-22T22:42:01Z

 this.mono_wasm_setenv ("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT", "1");
+
+ // Set globalization mode to PredefinedOnly
+ this.mono_wasm_setenv ("DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_ONLY", "1");


Why is it env variable if it's always set?

Just leaving the option open if we don't want it always set.

This won't be easy to change and it'll cost us on size. I think we should just compile the setting in

We want it always set for wasm but I'm not sure if other OS want this option by default. @tarekgh

outside WASM, we shouldn't turn this option on by default. Whoever need it would need to opt-in to get it.

We could do this by setting a default value for the flag and making that default value be true for wasm based on an OS check when you are setting the PredefinedCulturesOnly flag. Or, just if def the code for WASM to always be ON.

Here is a place where we do something similar for WASM:
runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Line 26 in e6105a1
if(loaded==0&&!OperatingSystem.IsBrowser())
So we could do something like:
GetSwitchValue("System.Globalization.PredefinedCulturesOnly","DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY")||OperatingSystem.IsBrowser()

tarekgh · 2021-01-22T22:53:22Z

CC @safern

tarekgh · 2021-01-22T23:32:28Z

Could you please replace the checks in

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.Nls.cs

Line 14 in f1ad1b9

if(CultureData.GetLocaleInfoExInt(name,Interop.Kernel32.LOCALE_ICONSTRUCTEDLOCALE)==1)

and

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.Icu.cs

Line 14 in f1ad1b9

if(!Interop.Globalization.IsPredefinedLocale(name))

with the new introduced method calls?

thank you!

tarekgh · 2021-01-22T23:54:30Z

One last ask, could you please add a new test (not specific to the browser) to test the new environment variable? here is some test example which doing similar idea (testing setting environment variable).

runtime/src/libraries/System.Threading.Tasks.Extensions/tests/AsyncValueTaskMethodBuilderTests.cs

Line 568 in e63969d

varpsi=newProcessStartInfo();

Modify Globalization tests to catch unsupported locales Change var names

tqiu8 · 2021-01-25T21:00:41Z

You need to do this test with RemoteExecutor as I pointed before in my comment #47301 (comment). Otherwise this test can easily fail and also will cause other problems for other tests running after it.

I don't think RemoteExcutor or System.Diagnostics.Process are supported on wasm right now. I'm going to store the current env var and reset it back to that after the test?

tarekgh · 2021-01-25T21:02:23Z

I don't think RemoteExcutor or System.Diagnostics.Process are supported on wasm right now. I'm going to store the current env var and reset it back to that after the test?

Yes, just exclude the added test from running on WASM. just mark the test with the attribute [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]

…est data to indicate whether or not that locale is expected to throw exception on browser

tarekgh

@tqiu8 I have added one more comment I hope you address it.
Also, please track documenting the new environment variable too. Thanks!

tqiu8 · 2021-03-03T19:01:52Z

@tqiu8 I have added one more comment I hope you address it.
Also, please track documenting the new environment variable too. Thanks!

Thank you so much for your feedback!

lewing · 2021-03-05T03:12:43Z

This is looking good but the failures look like they might be related.

…Tests/Assembly/AssemblyTests.cs Co-authored-by: Larry Ewing <lewing@microsoft.com>

…e-dt-fix

…otnet#47301)" This reverts commit 128c757.

tqiu8 added 4 commits January 8, 2021 23:06

[wasm] Throw exception if culture data does not exist in icu
164db20

add lines
fd96d0c

add lines
68a4fa3

remove blank spaces
82759a8

tqiu8 requested review from lewing and radical January 21, 2021 22:36

ghost added the area-System.Globalization label Jan 21, 2021

tarekgh reviewed Jan 21, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs Outdated

tarekgh added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 21, 2021

This was referenced Jan 22, 2021
Inability to unzip assets during build on Unix x64 #32805
Closed
BinderTracingTest.ResolutionFlow.cmd timed out #37132
Closed

tqiu8 marked this pull request as draft January 22, 2021 18:38

Add check for culture data in ICU
d92afab
Add PredefinedOnly GlobalizationMode Modified tests if Culture Data not found

tarekgh reviewed Jan 22, 2021
View reviewed changes

include documentation link in exception message
df4d910

tqiu8 marked this pull request as ready for review January 22, 2021 22:33

tqiu8 requested a review from marek-safar as a code owner January 22, 2021 22:33

marek-safar reviewed Jan 22, 2021
View reviewed changes

Add PredefinedOnly to Windows
7167047

move checks outside of CultureData.cs
7c677dc

tarekgh reviewed Jan 22, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs Outdated

tarekgh reviewed Jan 22, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs Outdated

tarekgh reviewed Jan 22, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs

Add test for predefined culture env var
53151f8
Modify Globalization tests to catch unsupported locales Change var names

remove exception filters from tests and add additional parameter to t…
84b8429
…est data to indicate whether or not that locale is expected to throw exception on browser

tqiu8 requested review from lewing and tarekgh March 3, 2021 16:58

tarekgh removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 3, 2021

tarekgh reviewed Mar 3, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.cs Outdated

tarekgh approved these changes Mar 3, 2021
View reviewed changes

move exception out of CreateCulturedata
f8163f7

tarekgh reviewed Mar 4, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs Outdated

remove else block
451a3ad

tarekgh reviewed Mar 4, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs

tqiu8 added 2 commits March 4, 2021 13:53

remove redundant methods
0bc3a5c

revert logic in nls
c40d610

lewing reviewed Mar 5, 2021
View reviewed changes

Comment threadsrc/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Assembly/AssemblyTests.cs Outdated

tqiu8and others added 3 commits March 5, 2021 11:36

Update src/libraries/System.Reflection.MetadataLoadContext/tests/src/…
eeeda5f
…Tests/Assembly/AssemblyTests.cs Co-authored-by: Larry Ewing <lewing@microsoft.com>

Merge branch 'master' into culture-dt-fix
b956ca6

Merge branch 'culture-dt-fix' of github.com:tqiu8/runtime into cultur…
3b065d2
…e-dt-fix

runfoappBot mentioned this pull request Mar 8, 2021
STATUS_UNSUCCESSFUL in RsaCryptRoundtrip_OaepSHA1 #29683
Open

fix invariant tests
0aab18e

tarekgh mentioned this pull request Mar 9, 2021
Crash in GlobalizationNative_GetSortHandle related to System.Globalization.GlobalizationMode #49073
Closed

jkotas reviewed Mar 9, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Icu.cs Outdated

jkotas reviewed Mar 9, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Nls.cs Outdated

remove aggressive inlining
23842ee

tqiu8 merged commit 128c757 into dotnet:mainMar 10, 2021

radical added a commit to radical/runtime that referenced this pull request Mar 12, 2021
Revert "[wasm] Throw exception if culture data does not exist in icu (d…
a491ad7
…otnet#47301)" This reverts commit 128c757.

lewing mentioned this pull request Mar 14, 2021
[wasm] InvariantGlobalization=true no longer removes all icu references #49499
Closed

ghost locked as resolved and limited conversation to collaborators Apr 9, 2021

karelz added this to the 6.0.0 milestone May 20, 2021


		internal static bool UseNls => false;

		internal static bool PredefinedOnly { get; } = GetSwitchValue("System.Globalization.PredefinedOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_ONLY");

Uh oh!

Conversation

tqiu8 commented Jan 21, 2021

Uh oh!

ghost commented Jan 21, 2021

Uh oh!

Uh oh!

tarekghJan 22, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekghJan 22, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekghJan 25, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safernJan 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Jan 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tarekgh commented Jan 22, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarekgh commented Jan 22, 2021

Uh oh!

tqiu8 commented Jan 25, 2021

Uh oh!

tarekgh commented Jan 25, 2021

Uh oh!

Uh oh!

tarekgh left a comment

Choose a reason for hiding this comment

Uh oh!

tqiu8 commented Mar 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lewing commented Mar 5, 2021

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tarekghJan 22, 2021•
edited
Loading

tarekghJan 22, 2021•
edited
Loading

tarekghJan 25, 2021•
edited
Loading

safernJan 29, 2021•
edited
Loading

tarekgh commented Jan 22, 2021•
edited
Loading