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

Use same TimeZoneInfo implementation on iOS/tvOS as on browser #51457

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

filipnavara
Copy link
Member

Ref: xamarin/xamarin-macios#11175 (comment), https://unicode-org.atlassian.net/browse/ICU-21591

There seems to be a bug in ICU that leads to deadlock when the time zone data are stripped. Since dotnet/icu uses the same stripping of data on all the platforms the time zone data are also not present on iOS/tvOS or any platform that consumes it. So even if the deadlock itself is resolved at some point it makes sense to use the same implementation for all the platforms that rely on the filtered app-local ICU data.

I also enabled to code path on MacCatalyst to keep it consistent with iOS. I am open to change that. Android may need to be treated the same way too.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@filipnavara
Copy link
Member Author

cc @akoeplinger @steveisok

@steveisok
Copy link
Member

Thanks. Makes sense. We're probably going to need to go back to the filters and include some items that are missing, like time zone and some calendars.

@steveisok
Copy link
Member

I also enabled to code path on MacCatalyst to keep it consistent with iOS. I am open to change that. Android may need to be treated the same way too.

As of right now, MacCatalyst is assuming ICU is installed like the desktop build. We probably should change that since it's supposed to reflect an iOS app.

@filipnavara
Copy link
Member Author

filipnavara commented Apr 18, 2021

We're probably going to need to go back to the filters and include some items that are missing, like time zone and some calendars.

Agreed. It could probably be even optional by offering more than one icudt.dat file.

As of right now, MacCatalyst is assuming ICU is installed like the desktop build. We probably should change that since it's supposed to reflect an iOS app.

I can change it to use full data for Mac Catalyst and we can re-evaluate it later [for ICU as a whole]. Android also uses the local data, right? It would run into the same deadlock as iOS so I guess it makes sense to change it too, at least as a stop gap measure.

@steveisok
Copy link
Member

We weren't able to enable our icu on android because it requires libc++. That would have made the Xamarin android team rather.... unhappy. We are using the system icu there.

@filipnavara
Copy link
Member Author

We weren't able to enable our icu on android

Cool. I was checking just the dotnet/icu repository and what builds were enabled there.

Let me know if you prefer Mac Catalyst to behave one way or another in this PR. Aside from that I think it's good enough to unblock the Xamarin preview4 integration.

Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about any other macatalyst change in this PR. I'd rather have that separate.

@steveisok
Copy link
Member

@akoeplinger please give this a review.

@spouliot
Copy link
Contributor

@steveisok I have not looked into the issue yet (so it might not be applicable) but we do have (legacy) code, inside Mono, to handle TimeZone for macOS/iOS... If it can be reused then it would not require further changes (or more code/data) and remain compatible with the current SDK.

@filipnavara
Copy link
Member Author

The missing data are just time zone names and windows/unix time zone conversions tables (new API in .NET 6). Otherwise the implementation peeks directly into /usr/share/zoneinfo/. If that works on iOS (and I am not sure if it does) then the minimal implementation should be good enough. If the peeking into file would not work then porting some code to use NSTimeZone would likely make sense.

@akoeplinger
Copy link
Member

What's the behavior when we fix the deadlock in our icu fork? Is it just going to fail the lookup and fall back to the invariant name like it does with the minimal implementation?

…b.Shared.projitems

Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@filipnavara
Copy link
Member Author

What's the behavior when we fix the deadlock in our icu fork? Is it just going to fail the lookup and fall back to the invariant name like it does with the minimal implementation?

There is some fallback but it's not immediately obvious if it will return the same thing as the minimal implementation. My gut says it would not and that it's untested code path. It would be easy to fix though.

// If there is an unknown error, don't set the displayName field.
// It will be set to the abbreviation that was read out of the tzfile.
if (result && !string.IsNullOrEmpty(timeZoneDisplayName))
{
displayName = timeZoneDisplayName;
}

@akoeplinger akoeplinger changed the title Use same TimeZoneInfo implementation on iOS/tvOS/MacCatalyst as on browser Use same TimeZoneInfo implementation on iOS/tvOS as on browser Apr 19, 2021
@akoeplinger
Copy link
Member

/backport to release/6.0-preview4

@github-actions
Copy link
Contributor

Started backporting to release/6.0-preview4: https://github.com/dotnet/runtime/actions/runs/764196749

@akoeplinger akoeplinger merged commit db3c25d into dotnet:main Apr 19, 2021
@filipnavara filipnavara deleted the icu_tz_ios branch April 21, 2021 10:54
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants