Skip to content

Support Time Zone IANA Ids and Windows Ids in all platforms#49412

Merged
tarekgh merged 7 commits into
dotnet:mainfrom
tarekgh:TimeZones
Mar 12, 2021
Merged

Support Time Zone IANA Ids and Windows Ids in all platforms#49412
tarekgh merged 7 commits into
dotnet:mainfrom
tarekgh:TimeZones

Conversation

@tarekgh

@tarekgh tarekgh commented Mar 10, 2021

Copy link
Copy Markdown
Member

This change is to allow using either IANA Id or Windows Id in any platform to create a TimeZoneInfo object. That will allow users not to worry which Id type work on which platform. Also, we have the issue #49407 tracking exposing the APIs which can be used to convert IANA Ids to Windows Ids and vice versa.

As we depend on ICU library, this functionality will be limited to using the ICU. This is the default mode anyway. This functionality will not be available in the following cases:

  • Running with Globalization Invariant Mode.
  • Switching to use NLS on Windows.
  • Using ICU library version less than 52. The only case I am aware of using version less than 52 is Centos 7 (and matching RHEL version). Upgrading ICU on such platforms will work around the issue there.

@ghost

ghost commented Mar 10, 2021

Copy link
Copy Markdown

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

Issue Details

This change is to allow using either IANA Id or Windows Id in any platform to create a TimeZoneInfo object. That will allow users not to worry which Id type work on which platform. Also, we have the issue #49407 tracking exposing the APIs which can be used to convert IANA Ids to Windows Ids and vice versa.

As we depend on ICU library, this functionality will be limited to using the ICU. This is the default mode anyway. This functionality will not be available in the following cases:

  • Running with Globalization Invariant Mode.
  • Switching to use NLS on Windows.
  • Using ICU library version less than 52. The only case I am aware of using version less than 52 is Centos 7 (and matching RHEL version).
Author: tarekgh
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh tarekgh self-assigned this Mar 10, 2021
@tarekgh tarekgh requested review from safern and tqiu8 March 10, 2021 03:24
@tarekgh tarekgh added this to the 6.0.0 milestone Mar 10, 2021
@tarekgh

tarekgh commented Mar 10, 2021

Copy link
Copy Markdown
Member Author

@mattjohnsonpint I'll wait merging this till you merge yours

@tarekgh

tarekgh commented Mar 10, 2021

Copy link
Copy Markdown
Member Author

CC @eerhardt @lewing

@mattjohnsonpint

Copy link
Copy Markdown
Contributor

This should help address #18644, and reduce the need for my TimeZoneConverter library.

@eerhardt

Copy link
Copy Markdown
Member

This will fix #14929, correct?

@tarekgh

tarekgh commented Mar 10, 2021

Copy link
Copy Markdown
Member Author

This will fix #14929, correct?

That is right. thanks for linking the issue to this PR.

Comment thread src/libraries/Native/Unix/System.Globalization.Native/pal_icushim_internal.h Outdated
@tarekgh

tarekgh commented Mar 10, 2021

Copy link
Copy Markdown
Member Author

@steveisok @lambdageek could you please have a look if you see anything related to the mobile platforms? Thanks!

@tarekgh

tarekgh commented Mar 11, 2021

Copy link
Copy Markdown
Member Author

@lewing @tqiu8 looks to me that the ICU version used by the browser not having the time zone Id conversion data. I'll let you decide if this feature is important to support in the browser to enable. Let me know if I can help in anything here.

@mattjohnsonpint

Copy link
Copy Markdown
Contributor

FWIW, browsers use only IANA IDs. I think the scenarios for Windows IDs are mostly server-side or desktop.

@tarekgh

tarekgh commented Mar 11, 2021

Copy link
Copy Markdown
Member Author

Looks the runtime-dev-innerloop CI failure is tracked by #49309

@tarekgh

tarekgh commented Mar 11, 2021

Copy link
Copy Markdown
Member Author

Looks the runtime (Build Browser wasm Release AllSubsets_Mono) CI failure is the issue #49499

Comment thread src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs Outdated
@tarekgh

tarekgh commented Mar 12, 2021

Copy link
Copy Markdown
Member Author

#29683

@tarekgh tarekgh merged commit 9fbaf19 into dotnet:main Mar 12, 2021
@tarekgh tarekgh deleted the TimeZones branch March 12, 2021 04:29
@lewing

lewing commented Mar 12, 2021

Copy link
Copy Markdown
Member

@lewing @tqiu8 looks to me that the ICU version used by the browser not having the time zone Id conversion data. I'll let you decide if this feature is important to support in the browser to enable. Let me know if I can help in anything here.

It is unfortunate that the only options we have are increasing size or making browser even more unique. @tqiu8 can you take a look at the icu filters and see how much size the conversion data would add?

@lewing

lewing commented Mar 12, 2021

Copy link
Copy Markdown
Member

This data will impact the mobile icu sizes as well if they want to support it.

@tarekgh

tarekgh commented Mar 12, 2021

Copy link
Copy Markdown
Member Author

@lewing maybe @mattjohnsonpint comment #49412 (comment) make sense? and the browser should work only with IANA Ids only?

@tqiu8

tqiu8 commented Mar 12, 2021

Copy link
Copy Markdown
Contributor

@lewing @tqiu8 looks to me that the ICU version used by the browser not having the time zone Id conversion data. I'll let you decide if this feature is important to support in the browser to enable. Let me know if I can help in anything here.

It is unfortunate that the only options we have are increasing size or making browser even more unique. @tqiu8 can you take a look at the icu filters and see how much size the conversion data would add?

Taking the idudt.json filter and enabling metaZones, takes the size of icudt.dat from 1480K to 1676K

@tarekgh

tarekgh commented Mar 12, 2021

Copy link
Copy Markdown
Member Author

@tqiu8 I think metaZones has a lot of other data that not needed for the name conversion functionality. Do we have the option to tailor the contents of the metaZones?

@tqiu8

tqiu8 commented Mar 12, 2021

Copy link
Copy Markdown
Contributor

@tarekgh Yes that's definitely possible considering it's just a txt file.

@tarekgh

tarekgh commented Mar 12, 2021

Copy link
Copy Markdown
Member Author

@tqiu8 that is great. let's give it a try and look what size we get.

@mattjohnsonpint

mattjohnsonpint commented Mar 12, 2021

Copy link
Copy Markdown
Contributor

Not sure about how ICU bundles it, but strictly speaking about CLDR data, metazones aren't required for Windows ID conversion. Only windowsZones.xml and bcp47/timeZones.xml are needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants