DateTimeFormat.FirstDayOfWeek inconsistent behaviour

andreabalducci commented 2 years ago

Description

Ran in the #51146 issue porting a library from full fx to net6.

CLDR was updated to fix the issue:

I understand FullFx and Net5+ have different implementations (as https://docs.microsoft.com/en-us/dotnet/core/extensions/globalization-icu) but honestly can't figure out why net 6 gives different results running on MacOs vs running on Windows with standard project settings.

Reproduction Steps

Created a simple repro on https://github.com/andreabalducci/culture-info-tests/

Expected behavior

On all platforms / runtimes FirstDayOfWeek should be Monday

Actual behavior

net 6 on windows returns Sunday

Regression?

Works as expected on Net48

Known Workarounds

Fallback to NLS on windows, haven't tried but should work https://docs.microsoft.com/en-us/dotnet/core/extensions/globalization-icu
Changing the FirstDayOfWeek property at runtime (setter is public)

Configuration

6.0.400-preview.22330.6 on macos 6.0.300-preview.22204.3 on windows 10

Other information

No response

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-system-globalization See info in area-owners.md if you want to be subscribed.

Issue Details

### Description Ran in the #51146 issue porting a library from full fx to net6. CLDR was updated to fix the issue: - https://unicode-org.atlassian.net/browse/CLDR-14795 - https://github.com/unicode-org/cldr/pull/1450 I understand FullFx and Net5+ have different implementations (as https://docs.microsoft.com/en-us/dotnet/core/extensions/globalization-icu) but honestly can't figure out why net 6 gives different results running on MacOs vs running on Windows with standard project settings. ### Reproduction Steps Created a simple repro on https://github.com/andreabalducci/culture-info-tests/ ### Expected behavior On all platforms / runtimes FirstDayOfWeek should be Monday ### Actual behavior net 6 on windows returns Sunday ### Regression? Works as expected on Net48 ### Known Workarounds - Fallback to NLS on windows, haven't tried but should work https://docs.microsoft.com/en-us/dotnet/core/extensions/globalization-icu - Changing the FirstDayOfWeek property at runtime (setter is public) ### Configuration 6.0.400-preview.22330.6 on macos 6.0.300-preview.22204.3 on windows 10 ### Other information _No response_

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

MichalPetryka commented 2 years ago

What culture is this on? Some culture do indeed start weeks on Sunday.

andreabalducci commented 2 years ago

@MichalPetryka 'en-AU' , the same for the linked issue and the merged pr on CLDR. The issue is about consistency, different results on different platforms / runtimes and correctness (correct value is Monday). Probably the runtime include an old version of CLDR reference, the fix was merged on Aug 21

danmoseley commented 2 years ago

Windows carries ICU and it perhaps is older than the change you mention. What Windows version/build are you using? @tarekgh

MichalPetryka commented 2 years ago

Try running with app local ICU by adding this to the csproj:

<ItemGroup>
  <PackageReference Include="Microsoft.ICU.ICU4C.Runtime" Version="68.2.0.9" />
  <RuntimeHostConfigurationOption Include="System.Globalization.AppLocalIcu" Value="68.2" />
</ItemGroup>

andreabalducci commented 2 years ago

@danmoseley Windows 10 21H2 (https://github.com/andreabalducci/culture-info-tests/#readme) Just checked system32\icu.dll, version is 64.2 ( CLDR 35, June 21).

andreabalducci commented 2 years ago

@MichalPetryka tried but still failing, there's no newer ICU package on nuget to test.

andreabalducci commented 2 years ago

for my understanding ICU should be at least version 70 (released after en-AU patch)

andreabalducci commented 2 years ago

@MichalPetryka manually downloaded the icu70*.dll, now all tests are green on windows

danmoseley commented 2 years ago

@andreabalducci Can we close this, are you unblocked?

MichalPetryka commented 2 years ago

@andreabalducci Can we close this, are you unblocked?

Should the applocal ICU package, Microsoft.ICU.ICU4C.Runtime, maybe be updated to a newer version?

andreabalducci commented 2 years ago

@danmoseley I wasn't blocked, patched the culture info at runtime to fix the issue (public setters) then filed the issue to document the problem and check if there was something wrong on the runtime.

Still thinking if using the OS provided ICU to "reduce differences on different platforms" instead of shipping an updated ICU with the .net runtime "works as expected".

andreabalducci commented 2 years ago

@MichalPetryka haven't found an updated Microsoft.ICU.ICU4C.Runtime package, the one you linked is the last on nuget. Had to download the .dlls from the ICU github project and manually copy on the net6.0\runtimes\win-x64\native folder

tarekgh commented 2 years ago

@andreabalducci you may log issue to MS ICU asking for releasing a latest version of ICU on NuGet. @chetanladdha is the owner of releasing ICU on NuGet who may help more with this issue and will be good to follow up with him. I'll close the issue here as there is no action that can be taken from the .NET side. I will try to contact @chetanladdha too to see if we can get an updated ICU package soon.

Still thinking if using the OS provided ICU to "reduce differences on different platforms" instead of shipping an updated ICU with the .net runtime "works as expected".

.NET doesn't carry ICU and we are not going to change that. Because this can create update problems after shipping every version of the .NET. The .NET depends on the underlying OS to get whatever ICU used there (for the sake of consistency too with the OS) and .NET provides the app-local feature for the fixability having the apps carry their own version of ICU if needed. This is good enough. For issue here, it is just a matter of time to get updated package to use.

dotnet / runtime