Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS] Calendar not behaving correctly wrt culture #50859

Closed
spouliot opened this issue Apr 7, 2021 · 17 comments
Closed

[iOS] Calendar not behaving correctly wrt culture #50859

spouliot opened this issue Apr 7, 2021 · 17 comments
Assignees
Milestone

Comments

@spouliot
Copy link
Contributor

spouliot commented Apr 7, 2021

Description

I am re-enabling some tests to confirm ICU is working correctly to replace the legacy mono code for iOS.

Most globalization tests have been re-enabled successfully, except for Calendar tests

https://github.com/xamarin/xamarin-macios/blob/main/tests/linker/ios/dont%20link/CalendarTest.cs

Those are not returning the expected types.

DontLink

DontLink.Calendars

DontLink.Calendars.CalendarTest
	[FAIL] Hijri :   Calendar
  Expected string length 34 but was 38. Strings differ at index 21.
  Expected: "System.Globalization.HijriCalendar"
  But was:  "System.Globalization.GregorianCalendar"
  --------------------------------^

		   at DontLink.Calendars.CalendarTest.Hijri() in /Users/poupou/git/main/xamarin-macios/tests/linker/ios/dont link/CalendarTest.cs:line 28
	[FAIL] ThaiBuddhist :   Calendar
  Expected string length 41 but was 38. Strings differ at index 21.
  Expected: "System.Globalization.ThaiBuddhistCalendar"
  But was:  "System.Globalization.GregorianCalendar"
  --------------------------------^

		   at DontLink.Calendars.CalendarTest.ThaiBuddhist() in /Users/poupou/git/main/xamarin-macios/tests/linker/ios/dont link/CalendarTest.cs:line 35
	[FAIL] UmAlQura :   Calendar
  Expected string length 37 but was 38. Strings differ at index 21.
  Expected: "System.Globalization.UmAlQuraCalendar"
  But was:  "System.Globalization.GregorianCalendar"
  --------------------------------^

		   at DontLink.Calendars.CalendarTest.UmAlQura() in /Users/poupou/git/main/xamarin-macios/tests/linker/ios/dont link/CalendarTest.cs:line 21
DontLink.Calendars.CalendarTest : 226.39 ms
DontLink.Calendars : 229.61 ms

Configuration

  • dotnet SDK 6.0.100-preview.3.21202.5
  • the ICU enabled code is not yet merged - in part since I can't re-enable this test
  • unit test Dont link (where the linker is not used) shows the issue

Regression?

Yes. This is working on the current Xamarin.iOS SDK.

Other information

xamarin-macios tracking issue for globalization

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Globalization untriaged New issue has not been triaged by the area owner labels Apr 7, 2021
@ghost
Copy link

ghost commented Apr 7, 2021

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

Issue Details

Description

I am re-enabling some tests to confirm ICU is working correctly to replace the legacy mono code for iOS.

Most globalization tests have been re-enabled successfully, except for Calendar tests

https://github.com/xamarin/xamarin-macios/blob/main/tests/linker/ios/dont%20link/CalendarTest.cs

Those are not returning the expected types.

DontLink

DontLink.Calendars

DontLink.Calendars.CalendarTest
	[FAIL] Hijri :   Calendar
  Expected string length 34 but was 38. Strings differ at index 21.
  Expected: "System.Globalization.HijriCalendar"
  But was:  "System.Globalization.GregorianCalendar"
  --------------------------------^

		   at DontLink.Calendars.CalendarTest.Hijri() in /Users/poupou/git/main/xamarin-macios/tests/linker/ios/dont link/CalendarTest.cs:line 28
	[FAIL] ThaiBuddhist :   Calendar
  Expected string length 41 but was 38. Strings differ at index 21.
  Expected: "System.Globalization.ThaiBuddhistCalendar"
  But was:  "System.Globalization.GregorianCalendar"
  --------------------------------^

		   at DontLink.Calendars.CalendarTest.ThaiBuddhist() in /Users/poupou/git/main/xamarin-macios/tests/linker/ios/dont link/CalendarTest.cs:line 35
	[FAIL] UmAlQura :   Calendar
  Expected string length 37 but was 38. Strings differ at index 21.
  Expected: "System.Globalization.UmAlQuraCalendar"
  But was:  "System.Globalization.GregorianCalendar"
  --------------------------------^

		   at DontLink.Calendars.CalendarTest.UmAlQura() in /Users/poupou/git/main/xamarin-macios/tests/linker/ios/dont link/CalendarTest.cs:line 21
DontLink.Calendars.CalendarTest : 226.39 ms
DontLink.Calendars : 229.61 ms

Configuration

  • dotnet SDK 6.0.100-preview.3.21202.5
  • the ICU enabled code is not yet merged - in part since I can't re-enable this test
  • unit test Dont link (where the linker is not used) shows the issue

Regression?

Yes. This is working on the current Xamarin.iOS SDK.

Other information

xamarin-macios tracking issue for globalization

Author: spouliot
Assignees: -
Labels:

area-System.Globalization, untriaged

Milestone: -

@spouliot
Copy link
Contributor Author

spouliot commented Apr 7, 2021

c.c. @steveisok

@steveisok steveisok added this to the 6.0.0 milestone Apr 7, 2021
@steveisok steveisok self-assigned this Apr 7, 2021
@tarekgh tarekgh added os-ios Apple iOS and removed untriaged New issue has not been triaged by the area owner labels Apr 7, 2021
spouliot added a commit to spouliot/xamarin-macios that referenced this issue Apr 8, 2021
and re-enable some tests for dotnet

Part of the fix for xamarin#8906

Known Issues
* [some Calendar are not the expected ones](dotnet/runtime#50859)
* [No support for tvOS](dotnet/runtime#50912)
spouliot added a commit to xamarin/xamarin-macios that referenced this issue Apr 9, 2021
and re-enable some tests for dotnet

Part of the fix for #8906

Known Issues
* [some Calendar are not the expected ones](dotnet/runtime#50859)
* [No support for tvOS (bitcode)](dotnet/runtime#48508)
@filipnavara
Copy link
Member

filipnavara commented Apr 12, 2021

The tests themselves looks somewhat fishy to be honest. The actual expected mapping:

  • ar (Argentina, not Arabia) -> System.Globalization.GregorianCalendar
  • ps (Persia Pashto) -> System.Globalization.PersianCalendar
  • th (Thailand) -> System.Globalization.ThaiBuddhistCalendar

It doesn't explain why it would always return Gregorian calendar though.

@spouliot
Copy link
Contributor Author

@filipnavara They are old tests we used to ensure DontLink and Link* tests gave the same results.

Being old does does not make them good (or bad) but I disagree with your interpretation of ar as "RFC 5646 Language Tags" shows

 'ar': 'Arabic',
  'ar-AE': 'Arabic (U.A.E.)',
  'ar-BH': 'Arabic (Bahrain)',
  'ar-DZ': 'Arabic (Algeria)',
  'ar-EG': 'Arabic (Egypt)',
  'ar-IQ': 'Arabic (Iraq)',
  'ar-JO': 'Arabic (Jordan)',
  'ar-KW': 'Arabic (Kuwait)',
  'ar-LB': 'Arabic (Lebanon)',
  'ar-LY': 'Arabic (Libya)',
  'ar-MA': 'Arabic (Morocco)',
  'ar-OM': 'Arabic (Oman)',
  'ar-QA': 'Arabic (Qatar)',
  'ar-SA': 'Arabic (Saudi Arabia)',
  'ar-SY': 'Arabic (Syria)',
  'ar-TN': 'Arabic (Tunisia)',
  'ar-YE': 'Arabic (Yemen)',

@filipnavara
Copy link
Member

I stand corrected on the Arabic. Somehow I was totally convinced because "ar" is used as country code for Argentina (ISO 3166). Nevertheless the ar culture and most of its subcultures still return Gregorian calendar. ar-SA returns System.Globalization.UmAlQuraCalendar.

@spouliot
Copy link
Contributor Author

yeah, like I said the tests were to ensure the same calendar was returned - whether or not the linker was enabled
now was mono correct to return that specific calendar (or not) is another story, one that might have evolved over time (at least for the .net desktop framework side)

@filipnavara
Copy link
Member

filipnavara commented Apr 12, 2021

So, apparently the ICU data filters are currently applied on all platforms for the app-local ICU builds and these filters (https://github.com/dotnet/icu/blob/313017d0effca6e8ade5fefc8e580224e65d8525/icu-filters/icudt.json#L261-L266) exclude non-Gregorian calendars from the data.

@steveisok
Copy link
Member

Good catch. We built the filters initially w/ wasm in mind, so it looks like we'll have to tweak a little bit.

@lewing
Copy link
Member

lewing commented Jun 30, 2021

How important is ICU data size for iOS?

@steveisok
Copy link
Member

Well, I haven't heard too much negative feedback re: the current ICU size ;-)

@marek-safar
Copy link
Contributor

How important is ICU data size for iOS?

It's big enough already ;-)

@tarekgh
Copy link
Member

tarekgh commented Jun 30, 2021

The framework already has the code for the calendars. This looks to me the issue is just associating the calendar Id to the culture. which I guess can be supported with minimal increase of size.

@lewing
Copy link
Member

lewing commented Jul 1, 2021

We can either add the calendars back in or change the test, it is a size trade off. This appears to be marked p0 on iOS so I guess we need information from @spouliot

@steveisok
Copy link
Member

Found we'll need to include "likelySubtags" in our filter in order for this to be fixed. Good news is that it only adds .1 MB to the dat file.

steveisok added a commit to dotnet/icu that referenced this issue Jul 13, 2021
#124)

This change also creates separate icudt filters for mobile and wasm as keeping likelySubtags results in a .1MB size increase.

Addresses dotnet/runtime#50859
rolfbjarne added a commit to xamarin/xamarin-macios that referenced this issue Jul 20, 2021
Adjust HttpClientHandler tests to cope with
dotnet/runtime#50859 and
#11392.
rolfbjarne added a commit to xamarin/xamarin-macios that referenced this issue Jul 20, 2021
Adjust HttpClientHandler tests to cope with
dotnet/runtime#50859 and
#11392.
@rolfbjarne
Copy link
Member

I'm seeing different results now with the latest .NET bump: xamarin/xamarin-macios@7e37fe6

@filipnavara
Copy link
Member

You are seeing the correct results now, ie. same as net6.0 on desktop with system ICU.

@steveisok
Copy link
Member

Assuming from the comments that this is fixed. Please reopen if you find otherwise.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants