[Android][libraries] TimeZoneInfo Android imp - #54845

Merged
steveisok merged 82 commits into
dotnet:mainfrom
mdh1418:port_android_timezoneinfo
Jul 21, 2021
Merged

[Android][libraries] TimeZoneInfo Android imp#54845
steveisok merged 82 commits into
dotnet:mainfrom
mdh1418:port_android_timezoneinfo

Conversation

@mdh1418

@mdh1418mdh1418 commented Jun 28, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#41867

Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab
TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs
Where TZI is TimeZoneInfo

zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.

The problem is that the time zone data for Android can be found in the file tzdata at the possible locations

/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at

 /usr/share/zoneinfo/zone.tab

Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path. Moreover, tzdata contains all timezones byte data.

This PR achieves the following:

  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.Unix.cs, TimeZoneInfo.Unix.NonAndroid.cs (non-Android), and TimeZoneInfo.Unix.Android.cs (Android specific)
  2. Adds an interop to obtain the default time zone on Android based on persist.sys.timezone
  3. Implements GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore and GetTimeZoneIds for Android based on mono/mono implementation https://github.com/mono/mono/blob/main/mcs/class/corlib/System/TimeZoneInfo.Android.cs
  4. Adds new string resources to throw exceptions
  5. Refactors the mono/mono implementation of parsing tzdata
Android tzdata files are found in the format of
Header <Beginning of Entry Index> Entry Entry Entry ... Entry <Beginning of Data Index> <TZDATA>
https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/bionic.cpp
The header (24 bytes) contains the following information
signature - 12 bytes of the form "tzdata2012f\0" where 2012f is subject to change
index offset - 4 bytes that denotes the offset at which the index of the tzdata file starts
data offset - 4 bytes that denotes the offset at which the data of the tzdata file starts
final offset - 4 bytes that used to denote the final offset, which we don't use but will note.
Each Data Entry (52 bytes) can be used to generate a TimeZoneInfo and contain the following information
id - 40 bytes that contain the id of the time zone data entry timezone<id>
byte offset - 4 bytes that denote the offset from the data offset timezone<id> data can be found
length - 4 bytes that denote the length of the data for timezone<id>
unused - 4 bytes that used to be raw GMT offset, but now is always 0 since tzdata2014f (L).

When GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore or GetTimeZoneIds are called, an android timezone data instance is instantiated and loaded by attempting to load a tzdata file that can be found at four locations mentioned earlier. The file is parsed by first loading the header which contains information about where the data index and data begin. The data index is then parsed to obtain the timezone and the corresponding bytes location in the file to fill the three arrays _ids_byteOffsets_lengths. These arrays are referenced to obtain the corresponding byte data for a timezone, and functions from TimeZoneInfo.Unix.cs are leveraged to create a TimeZoneInfo from there.

@ghost

ghost commented Jun 28, 2021

Copy link
Copy Markdown

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.

@mdh1418
mdh1418 marked this pull request as ready for review June 28, 2021 18:13
@mdh1418

mdh1418 commented Jun 28, 2021

Copy link
Copy Markdown
MemberAuthor

Unlike other Unix implementations where zone.tab contains meta-data and specific timezone files (Europe/London) contain the actual timezone data, tzdata on Android combines all timezone data together with the metadata into tzdata. As such, more mono implementation is needed to be ported over in order to get TryGetTimeZoneFromLocalMachine to work.

steveisok
steveisok reviewed Jun 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs Outdated
@steveisok

steveisok commented Jun 29, 2021

Copy link
Copy Markdown
Member

@mdh1418 As I suspected, there's a little bit more to this. I have more local and we can talk tomorrow about the details.

marek-safar
marek-safar reviewed Jun 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs Outdated
}
}

return Environment.GetEnvironmentVariable("ANDROID_ROOT") + DefaultTimeZoneDirectory;

@marek-safarmarek-safarJun 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This look odd. Are you sure we should mix 2 rooted locations?

@mdh1418mdh1418Jul 1, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think I'm missing something, what are the two rooted locations, ANDROID_ROOT and?
The code was adapted from https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73 and https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L488-L495 before AndroidTzData's own Path was introduced into the PR.

@ghost

ghost commented Jun 29, 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

Fixes #41867

Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab
TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs
Where TZI is TimeZoneInfo

zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.

The problem is that the time zone data for Android can be found in the file tzdata at the possible locations

/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at

 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.

This PR achieves the following:

  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:

area-System.Globalization

Milestone:-

@ghost

ghost commented Jun 29, 2021

Copy link
Copy Markdown

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #41867

Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab
TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs
Where TZI is TimeZoneInfo

zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.

The problem is that the time zone data for Android can be found in the file tzdata at the possible locations

/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at

 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.

This PR achieves the following:

  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:

os-android

Milestone:-

@ghost

ghost commented Jun 29, 2021

Copy link
Copy Markdown

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

Issue Details

Fixes #41867

Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab
TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs
Where TZI is TimeZoneInfo

zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.

The problem is that the time zone data for Android can be found in the file tzdata at the possible locations

/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at

 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.

This PR achieves the following:

  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:

area-System.Runtime, os-android

Milestone:-

Steve Pfister
Pulled in parsing implementation from mono and broke out GetLocalTime…
ce91611
…Zone into separate Unix and Android functions.
@steveisoksteveisok added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 29, 2021
eerhardt
eerhardt reviewed Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
*/
private sealed class AndroidTzData
{
private unsafe struct AndroidTzDataHeader

@eerhardteerhardtJul 16, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
privateunsafestruct AndroidTzDataHeader
privatereadonlystruct AndroidTzDataHeader

@mdh1418mdh1418Jul 16, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

error CS8340: Instance fields of readonly structs must be readonly.
/Users/mdhwang/runtime_droid/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs(314,17): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)

I went ahead and removed this struct altogether, using out parameters for indexOffset and dataOffset in LoadTzFile and LoadHeader

eerhardt
eerhardt reviewed Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
eerhardt
eerhardt reviewed Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
eerhardt
eerhardt reviewed Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
eerhardt
eerhardt reviewed Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
eerhardt
eerhardt reviewed Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
eerhardt
eerhardt reviewed Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
Comment on lines +412 to +414
Span<byte> buffer = stackalloc byte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath), offset, buffer);
byte[] tzBuffer = buffer.ToArray();

@eerhardteerhardtJul 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No reason to make a stack buffer here, copy the file into the buffer, and then ToArray it.

  1. It is unnecessary copying
  2. We don't know that the length of the file will fit on the stack. In general, stackalloc's should not be unbounded.
Suggested change
Span<byte>buffer=stackallocbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,buffer);
byte[]tzBuffer=buffer.ToArray();
byte[]tzBuffer=newbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,tzBuffer);

@mdh1418mdh1418Jul 16, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks. I see, was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span<byte> would suffice?

Is there a general rule for how large a stackalloc should be?

@eerhardteerhardtJul 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span would suffice?

Correct. In LoadTzFile, you just need a buffer to read the file data into, and then you are parsing the values out of it from the buffer. The buffer didn't need to stick around.

Here, since the method expects you to return a byte[], you might as well just read directly into the byte[] you are going to return.

Is there a general rule for how large a stackalloc should be?

Looking around the codebase, we usually don't let a stackalloc go above 1K of memory.

eerhardt
eerhardt reviewed Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
marek-safar
marek-safar approved these changes Jul 17, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
@mdh1418

mdh1418 commented Jul 19, 2021

Copy link
Copy Markdown
MemberAuthor

Made a few more changes, including parsing the data entry bytes to construct a string immediately, removing the structs all together, removed unsafe keywords, and updated the description of this PR. Can I get another review @eerhardt@jkotas@steveisok

jkotas
jkotas reviewed Jul 19, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated
@mdh1418mdh1418 mentioned this pull request Jul 19, 2021
Closed
This was referenced Jul 19, 2021
Closed
Closed
@mdh1418mdh1418 mentioned this pull request Jul 20, 2021
Closed
@karelzkarelz mentioned this pull request Jul 20, 2021
Closed
@steveisok
steveisok merged commit 46d9b31 into dotnet:mainJul 21, 2021
@mdh1418
mdh1418 deleted the port_android_timezoneinfo branch July 26, 2021 12:58
@karelzkarelz mentioned this pull request Jul 28, 2021
Closed
@ghostghost locked as resolved and limited conversation to collaborators Aug 25, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@akoeplingerakoeplingerakoeplinger left review comments
@jkotasjkotasjkotas left review comments
@eerhardteerhardteerhardt left review comments
@marek-safarmarek-safarmarek-safar approved these changes
@steveisoksteveisoksteveisok approved these changes
+1 more reviewer
@grendellogrendellogrendello left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.DateTimeos-android

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Port Android APEX timezone location lookup from mono/mono

10 participants

@mdh1418@steveisok@SamMonoRT@grendello@marek-safar@akoeplinger@jkotas@eerhardt@jeffhandley@maryamariyan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
[Android][libraries] TimeZoneInfo Android imp - #54845
Merged
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Jul 21, 2021
Merged
[Android][libraries] TimeZoneInfo Android imp#54845
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Conversation
@mdh1418
@mdh1418mdh1418
commented
Jun 28, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path. Moreover, tzdata contains all timezones byte data.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.Unix.cs, TimeZoneInfo.Unix.NonAndroid.cs (non-Android), and TimeZoneInfo.Unix.Android.cs (Android specific)
  2. Adds an interop to obtain the default time zone on Android based on persist.sys.timezone
  3. Implements GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore and GetTimeZoneIds for Android based on mono/mono implementation https://github.com/mono/mono/blob/main/mcs/class/corlib/System/TimeZoneInfo.Android.cs
  4. Adds new string resources to throw exceptions
  5. Refactors the mono/mono implementation of parsing tzdata
Android tzdata files are found in the format of
Header <Beginning of Entry Index> Entry Entry Entry ... Entry <Beginning of Data Index> <TZDATA>
https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/bionic.cpp
The header (24 bytes) contains the following information
signature - 12 bytes of the form "tzdata2012f\0" where 2012f is subject to change
index offset - 4 bytes that denotes the offset at which the index of the tzdata file starts
data offset - 4 bytes that denotes the offset at which the data of the tzdata file starts
final offset - 4 bytes that used to denote the final offset, which we don't use but will note.
Each Data Entry (52 bytes) can be used to generate a TimeZoneInfo and contain the following information
id - 40 bytes that contain the id of the time zone data entry timezone<id>
byte offset - 4 bytes that denote the offset from the data offset timezone<id> data can be found
length - 4 bytes that denote the length of the data for timezone<id>
unused - 4 bytes that used to be raw GMT offset, but now is always 0 since tzdata2014f (L).

When GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore or GetTimeZoneIds are called, an android timezone data instance is instantiated and loaded by attempting to load a tzdata file that can be found at four locations mentioned earlier. The file is parsed by first loading the header which contains information about where the data index and data begin. The data index is then parsed to obtain the timezone and the corresponding bytes location in the file to fill the three arrays _ids_byteOffsets_lengths. These arrays are referenced to obtain the corresponding byte data for a timezone, and functions from TimeZoneInfo.Unix.cs are leveraged to create a TimeZoneInfo from there.
@ghost
ghost
commented
Jun 28, 2021

Copy link


Copy Markdown

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.
@mdh1418
mdh1418
marked this pull request as ready for review
June 28, 2021 18:13
@mdh1418
mdh1418
commented
Jun 28, 2021

Copy link


Copy Markdown

MemberAuthor
Unlike other Unix implementations where zone.tab contains meta-data and specific timezone files (Europe/London) contain the actual timezone data, tzdata on Android combines all timezone data together with the metadata into tzdata. As such, more mono implementation is needed to be ported over in order to get TryGetTimeZoneFromLocalMachine to work.
steveisok
steveisok
reviewed
Jun 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

@steveisok
steveisok
commented
Jun 29, 2021

Copy link


Copy Markdown

Member
@mdh1418 As I suspected, there's a little bit more to this. I have more local and we can talk tomorrow about the details.
marek-safar
marek-safar
reviewed
Jun 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

 }
 }

 return Environment.GetEnvironmentVariable("ANDROID_ROOT") + DefaultTimeZoneDirectory;
@marek-safarmarek-safarJun 29, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This look odd. Are you sure we should mix 2 rooted locations?
@mdh1418mdh1418Jul 1, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think I'm missing something, what are the two rooted locations, ANDROID_ROOT and?

The code was adapted from https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73 and https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L488-L495 before AndroidTzData's own Path was introduced into the PR.
@ghost
ghost
commented
Jun 29, 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
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Globalization
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
os-android
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Runtime, os-android
Milestone:-
Steve Pfister
Pulled in parsing implementation from mono and broke out GetLocalTime…
ce91611
…Zone into separate Unix and Android functions.
@steveisoksteveisok
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Jun 29, 2021
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

 */
 private sealed class AndroidTzData
 {
 private unsafe struct AndroidTzDataHeader
@eerhardteerhardtJul 16, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

privateunsafestruct AndroidTzDataHeader
privatereadonlystruct AndroidTzDataHeader
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

error CS8340: Instance fields of readonly structs must be readonly.
/Users/mdhwang/runtime_droid/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs(314,17): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)
I went ahead and removed this struct altogether, using out parameters for indexOffset and dataOffset in LoadTzFile and LoadHeader
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated


Comment on lines

+412
 to 
+414

 Span<byte> buffer = stackalloc byte[length];
 ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath), offset, buffer);
 byte[] tzBuffer = buffer.ToArray();
@eerhardteerhardtJul 16, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

No reason to make a stack buffer here, copy the file into the buffer, and then ToArray it.
  1. It is unnecessary copying
  2. We don't know that the length of the file will fit on the stack. In general, stackalloc's should not be unbounded.

Suggested change

Span<byte>buffer=stackallocbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,buffer);
byte[]tzBuffer=buffer.ToArray();
byte[]tzBuffer=newbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,tzBuffer);
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks. I see, was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span<byte> would suffice?
Is there a general rule for how large a stackalloc should be?
@eerhardteerhardtJul 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span would suffice?
Correct. In LoadTzFile, you just need a buffer to read the file data into, and then you are parsing the values out of it from the buffer. The buffer didn't need to stick around.
Here, since the method expects you to return a byte[], you might as well just read directly into the byte[] you are going to return.
Is there a general rule for how large a stackalloc should be?
Looking around the codebase, we usually don't let a stackalloc go above 1K of memory.
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

marek-safar
marek-safar
approved these changes
Jul 17, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418
mdh1418
commented
Jul 19, 2021

Copy link


Copy Markdown

MemberAuthor
Made a few more changes, including parsing the data entry bytes to construct a string immediately, removing the structs all together, removed unsafe keywords, and updated the description of this PR. Can I get another review @eerhardt@jkotas@steveisok
jkotas
jkotas
reviewed
Jul 19, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418mdh1418
mentioned this pull request
Jul 19, 2021

Closed


This was referenced Jul 19, 2021

Closed


Closed

@mdh1418mdh1418
mentioned this pull request
Jul 20, 2021

Closed

@karelzkarelz
mentioned this pull request
Jul 20, 2021

Closed

@steveisok
steveisok
merged commit 46d9b31
into
dotnet:mainJul 21, 2021
@mdh1418
mdh1418
deleted the

port_android_timezoneinfo

branch
July 26, 2021 12:58
@karelzkarelz
mentioned this pull request
Jul 28, 2021

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 25, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger left review comments
@jkotasjkotasjkotas left review comments
@eerhardteerhardteerhardt left review comments
@marek-safarmarek-safarmarek-safar approved these changes
@steveisoksteveisoksteveisok approved these changes

+1 more reviewer

@grendellogrendellogrendello left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.DateTimeos-android

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Port Android APEX timezone location lookup from mono/mono


10 participants

@mdh1418@steveisok@SamMonoRT@grendello@marek-safar@akoeplinger@jkotas@eerhardt@jeffhandley@maryamariyan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Android][libraries] TimeZoneInfo Android imp - #54845
Merged
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Jul 21, 2021
Merged
[Android][libraries] TimeZoneInfo Android imp#54845
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Conversation
@mdh1418
@mdh1418mdh1418
commented
Jun 28, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path. Moreover, tzdata contains all timezones byte data.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.Unix.cs, TimeZoneInfo.Unix.NonAndroid.cs (non-Android), and TimeZoneInfo.Unix.Android.cs (Android specific)
  2. Adds an interop to obtain the default time zone on Android based on persist.sys.timezone
  3. Implements GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore and GetTimeZoneIds for Android based on mono/mono implementation https://github.com/mono/mono/blob/main/mcs/class/corlib/System/TimeZoneInfo.Android.cs
  4. Adds new string resources to throw exceptions
  5. Refactors the mono/mono implementation of parsing tzdata
Android tzdata files are found in the format of
Header <Beginning of Entry Index> Entry Entry Entry ... Entry <Beginning of Data Index> <TZDATA>
https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/bionic.cpp
The header (24 bytes) contains the following information
signature - 12 bytes of the form "tzdata2012f\0" where 2012f is subject to change
index offset - 4 bytes that denotes the offset at which the index of the tzdata file starts
data offset - 4 bytes that denotes the offset at which the data of the tzdata file starts
final offset - 4 bytes that used to denote the final offset, which we don't use but will note.
Each Data Entry (52 bytes) can be used to generate a TimeZoneInfo and contain the following information
id - 40 bytes that contain the id of the time zone data entry timezone<id>
byte offset - 4 bytes that denote the offset from the data offset timezone<id> data can be found
length - 4 bytes that denote the length of the data for timezone<id>
unused - 4 bytes that used to be raw GMT offset, but now is always 0 since tzdata2014f (L).

When GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore or GetTimeZoneIds are called, an android timezone data instance is instantiated and loaded by attempting to load a tzdata file that can be found at four locations mentioned earlier. The file is parsed by first loading the header which contains information about where the data index and data begin. The data index is then parsed to obtain the timezone and the corresponding bytes location in the file to fill the three arrays _ids_byteOffsets_lengths. These arrays are referenced to obtain the corresponding byte data for a timezone, and functions from TimeZoneInfo.Unix.cs are leveraged to create a TimeZoneInfo from there.
@ghost
ghost
commented
Jun 28, 2021

Copy link


Copy Markdown

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.
@mdh1418
mdh1418
marked this pull request as ready for review
June 28, 2021 18:13
@mdh1418
mdh1418
commented
Jun 28, 2021

Copy link


Copy Markdown

MemberAuthor
Unlike other Unix implementations where zone.tab contains meta-data and specific timezone files (Europe/London) contain the actual timezone data, tzdata on Android combines all timezone data together with the metadata into tzdata. As such, more mono implementation is needed to be ported over in order to get TryGetTimeZoneFromLocalMachine to work.
steveisok
steveisok
reviewed
Jun 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

@steveisok
steveisok
commented
Jun 29, 2021

Copy link


Copy Markdown

Member
@mdh1418 As I suspected, there's a little bit more to this. I have more local and we can talk tomorrow about the details.
marek-safar
marek-safar
reviewed
Jun 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

 }
 }

 return Environment.GetEnvironmentVariable("ANDROID_ROOT") + DefaultTimeZoneDirectory;
@marek-safarmarek-safarJun 29, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This look odd. Are you sure we should mix 2 rooted locations?
@mdh1418mdh1418Jul 1, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think I'm missing something, what are the two rooted locations, ANDROID_ROOT and?

The code was adapted from https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73 and https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L488-L495 before AndroidTzData's own Path was introduced into the PR.
@ghost
ghost
commented
Jun 29, 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
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Globalization
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
os-android
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Runtime, os-android
Milestone:-
Steve Pfister
Pulled in parsing implementation from mono and broke out GetLocalTime…
ce91611
…Zone into separate Unix and Android functions.
@steveisoksteveisok
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Jun 29, 2021
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

 */
 private sealed class AndroidTzData
 {
 private unsafe struct AndroidTzDataHeader
@eerhardteerhardtJul 16, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

privateunsafestruct AndroidTzDataHeader
privatereadonlystruct AndroidTzDataHeader
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

error CS8340: Instance fields of readonly structs must be readonly.
/Users/mdhwang/runtime_droid/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs(314,17): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)
I went ahead and removed this struct altogether, using out parameters for indexOffset and dataOffset in LoadTzFile and LoadHeader
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated


Comment on lines

+412
 to 
+414

 Span<byte> buffer = stackalloc byte[length];
 ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath), offset, buffer);
 byte[] tzBuffer = buffer.ToArray();
@eerhardteerhardtJul 16, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

No reason to make a stack buffer here, copy the file into the buffer, and then ToArray it.
  1. It is unnecessary copying
  2. We don't know that the length of the file will fit on the stack. In general, stackalloc's should not be unbounded.

Suggested change

Span<byte>buffer=stackallocbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,buffer);
byte[]tzBuffer=buffer.ToArray();
byte[]tzBuffer=newbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,tzBuffer);
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks. I see, was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span<byte> would suffice?
Is there a general rule for how large a stackalloc should be?
@eerhardteerhardtJul 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span would suffice?
Correct. In LoadTzFile, you just need a buffer to read the file data into, and then you are parsing the values out of it from the buffer. The buffer didn't need to stick around.
Here, since the method expects you to return a byte[], you might as well just read directly into the byte[] you are going to return.
Is there a general rule for how large a stackalloc should be?
Looking around the codebase, we usually don't let a stackalloc go above 1K of memory.
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

marek-safar
marek-safar
approved these changes
Jul 17, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418
mdh1418
commented
Jul 19, 2021

Copy link


Copy Markdown

MemberAuthor
Made a few more changes, including parsing the data entry bytes to construct a string immediately, removing the structs all together, removed unsafe keywords, and updated the description of this PR. Can I get another review @eerhardt@jkotas@steveisok
jkotas
jkotas
reviewed
Jul 19, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418mdh1418
mentioned this pull request
Jul 19, 2021

Closed


This was referenced Jul 19, 2021

Closed


Closed

@mdh1418mdh1418
mentioned this pull request
Jul 20, 2021

Closed

@karelzkarelz
mentioned this pull request
Jul 20, 2021

Closed

@steveisok
steveisok
merged commit 46d9b31
into
dotnet:mainJul 21, 2021
@mdh1418
mdh1418
deleted the

port_android_timezoneinfo

branch
July 26, 2021 12:58
@karelzkarelz
mentioned this pull request
Jul 28, 2021

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 25, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger left review comments
@jkotasjkotasjkotas left review comments
@eerhardteerhardteerhardt left review comments
@marek-safarmarek-safarmarek-safar approved these changes
@steveisoksteveisoksteveisok approved these changes

+1 more reviewer

@grendellogrendellogrendello left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.DateTimeos-android

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Port Android APEX timezone location lookup from mono/mono


10 participants

@mdh1418@steveisok@SamMonoRT@grendello@marek-safar@akoeplinger@jkotas@eerhardt@jeffhandley@maryamariyan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Android][libraries] TimeZoneInfo Android imp - #54845
Merged
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Jul 21, 2021
Merged
[Android][libraries] TimeZoneInfo Android imp#54845
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Conversation
@mdh1418
@mdh1418mdh1418
commented
Jun 28, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path. Moreover, tzdata contains all timezones byte data.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.Unix.cs, TimeZoneInfo.Unix.NonAndroid.cs (non-Android), and TimeZoneInfo.Unix.Android.cs (Android specific)
  2. Adds an interop to obtain the default time zone on Android based on persist.sys.timezone
  3. Implements GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore and GetTimeZoneIds for Android based on mono/mono implementation https://github.com/mono/mono/blob/main/mcs/class/corlib/System/TimeZoneInfo.Android.cs
  4. Adds new string resources to throw exceptions
  5. Refactors the mono/mono implementation of parsing tzdata
Android tzdata files are found in the format of
Header <Beginning of Entry Index> Entry Entry Entry ... Entry <Beginning of Data Index> <TZDATA>
https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/bionic.cpp
The header (24 bytes) contains the following information
signature - 12 bytes of the form "tzdata2012f\0" where 2012f is subject to change
index offset - 4 bytes that denotes the offset at which the index of the tzdata file starts
data offset - 4 bytes that denotes the offset at which the data of the tzdata file starts
final offset - 4 bytes that used to denote the final offset, which we don't use but will note.
Each Data Entry (52 bytes) can be used to generate a TimeZoneInfo and contain the following information
id - 40 bytes that contain the id of the time zone data entry timezone<id>
byte offset - 4 bytes that denote the offset from the data offset timezone<id> data can be found
length - 4 bytes that denote the length of the data for timezone<id>
unused - 4 bytes that used to be raw GMT offset, but now is always 0 since tzdata2014f (L).

When GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore or GetTimeZoneIds are called, an android timezone data instance is instantiated and loaded by attempting to load a tzdata file that can be found at four locations mentioned earlier. The file is parsed by first loading the header which contains information about where the data index and data begin. The data index is then parsed to obtain the timezone and the corresponding bytes location in the file to fill the three arrays _ids_byteOffsets_lengths. These arrays are referenced to obtain the corresponding byte data for a timezone, and functions from TimeZoneInfo.Unix.cs are leveraged to create a TimeZoneInfo from there.
@ghost
ghost
commented
Jun 28, 2021

Copy link


Copy Markdown

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.
@mdh1418
mdh1418
marked this pull request as ready for review
June 28, 2021 18:13
@mdh1418
mdh1418
commented
Jun 28, 2021

Copy link


Copy Markdown

MemberAuthor
Unlike other Unix implementations where zone.tab contains meta-data and specific timezone files (Europe/London) contain the actual timezone data, tzdata on Android combines all timezone data together with the metadata into tzdata. As such, more mono implementation is needed to be ported over in order to get TryGetTimeZoneFromLocalMachine to work.
steveisok
steveisok
reviewed
Jun 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

@steveisok
steveisok
commented
Jun 29, 2021

Copy link


Copy Markdown

Member
@mdh1418 As I suspected, there's a little bit more to this. I have more local and we can talk tomorrow about the details.
marek-safar
marek-safar
reviewed
Jun 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

 }
 }

 return Environment.GetEnvironmentVariable("ANDROID_ROOT") + DefaultTimeZoneDirectory;
@marek-safarmarek-safarJun 29, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This look odd. Are you sure we should mix 2 rooted locations?
@mdh1418mdh1418Jul 1, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think I'm missing something, what are the two rooted locations, ANDROID_ROOT and?

The code was adapted from https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73 and https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L488-L495 before AndroidTzData's own Path was introduced into the PR.
@ghost
ghost
commented
Jun 29, 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
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Globalization
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
os-android
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Runtime, os-android
Milestone:-
Steve Pfister
Pulled in parsing implementation from mono and broke out GetLocalTime…
ce91611
…Zone into separate Unix and Android functions.
@steveisoksteveisok
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Jun 29, 2021
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

 */
 private sealed class AndroidTzData
 {
 private unsafe struct AndroidTzDataHeader
@eerhardteerhardtJul 16, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

privateunsafestruct AndroidTzDataHeader
privatereadonlystruct AndroidTzDataHeader
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

error CS8340: Instance fields of readonly structs must be readonly.
/Users/mdhwang/runtime_droid/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs(314,17): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)
I went ahead and removed this struct altogether, using out parameters for indexOffset and dataOffset in LoadTzFile and LoadHeader
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated


Comment on lines

+412
 to 
+414

 Span<byte> buffer = stackalloc byte[length];
 ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath), offset, buffer);
 byte[] tzBuffer = buffer.ToArray();
@eerhardteerhardtJul 16, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

No reason to make a stack buffer here, copy the file into the buffer, and then ToArray it.
  1. It is unnecessary copying
  2. We don't know that the length of the file will fit on the stack. In general, stackalloc's should not be unbounded.

Suggested change

Span<byte>buffer=stackallocbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,buffer);
byte[]tzBuffer=buffer.ToArray();
byte[]tzBuffer=newbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,tzBuffer);
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks. I see, was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span<byte> would suffice?
Is there a general rule for how large a stackalloc should be?
@eerhardteerhardtJul 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span would suffice?
Correct. In LoadTzFile, you just need a buffer to read the file data into, and then you are parsing the values out of it from the buffer. The buffer didn't need to stick around.
Here, since the method expects you to return a byte[], you might as well just read directly into the byte[] you are going to return.
Is there a general rule for how large a stackalloc should be?
Looking around the codebase, we usually don't let a stackalloc go above 1K of memory.
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

marek-safar
marek-safar
approved these changes
Jul 17, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418
mdh1418
commented
Jul 19, 2021

Copy link


Copy Markdown

MemberAuthor
Made a few more changes, including parsing the data entry bytes to construct a string immediately, removing the structs all together, removed unsafe keywords, and updated the description of this PR. Can I get another review @eerhardt@jkotas@steveisok
jkotas
jkotas
reviewed
Jul 19, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418mdh1418
mentioned this pull request
Jul 19, 2021

Closed


This was referenced Jul 19, 2021

Closed


Closed

@mdh1418mdh1418
mentioned this pull request
Jul 20, 2021

Closed

@karelzkarelz
mentioned this pull request
Jul 20, 2021

Closed

@steveisok
steveisok
merged commit 46d9b31
into
dotnet:mainJul 21, 2021
@mdh1418
mdh1418
deleted the

port_android_timezoneinfo

branch
July 26, 2021 12:58
@karelzkarelz
mentioned this pull request
Jul 28, 2021

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 25, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger left review comments
@jkotasjkotasjkotas left review comments
@eerhardteerhardteerhardt left review comments
@marek-safarmarek-safarmarek-safar approved these changes
@steveisoksteveisoksteveisok approved these changes

+1 more reviewer

@grendellogrendellogrendello left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.DateTimeos-android

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Port Android APEX timezone location lookup from mono/mono


10 participants

@mdh1418@steveisok@SamMonoRT@grendello@marek-safar@akoeplinger@jkotas@eerhardt@jeffhandley@maryamariyan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
[Android][libraries] TimeZoneInfo Android imp - #54845
Merged
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Jul 21, 2021
Merged
[Android][libraries] TimeZoneInfo Android imp#54845
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Conversation
@mdh1418
@mdh1418mdh1418
commented
Jun 28, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path. Moreover, tzdata contains all timezones byte data.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.Unix.cs, TimeZoneInfo.Unix.NonAndroid.cs (non-Android), and TimeZoneInfo.Unix.Android.cs (Android specific)
  2. Adds an interop to obtain the default time zone on Android based on persist.sys.timezone
  3. Implements GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore and GetTimeZoneIds for Android based on mono/mono implementation https://github.com/mono/mono/blob/main/mcs/class/corlib/System/TimeZoneInfo.Android.cs
  4. Adds new string resources to throw exceptions
  5. Refactors the mono/mono implementation of parsing tzdata
Android tzdata files are found in the format of
Header <Beginning of Entry Index> Entry Entry Entry ... Entry <Beginning of Data Index> <TZDATA>
https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/bionic.cpp
The header (24 bytes) contains the following information
signature - 12 bytes of the form "tzdata2012f\0" where 2012f is subject to change
index offset - 4 bytes that denotes the offset at which the index of the tzdata file starts
data offset - 4 bytes that denotes the offset at which the data of the tzdata file starts
final offset - 4 bytes that used to denote the final offset, which we don't use but will note.
Each Data Entry (52 bytes) can be used to generate a TimeZoneInfo and contain the following information
id - 40 bytes that contain the id of the time zone data entry timezone<id>
byte offset - 4 bytes that denote the offset from the data offset timezone<id> data can be found
length - 4 bytes that denote the length of the data for timezone<id>
unused - 4 bytes that used to be raw GMT offset, but now is always 0 since tzdata2014f (L).

When GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore or GetTimeZoneIds are called, an android timezone data instance is instantiated and loaded by attempting to load a tzdata file that can be found at four locations mentioned earlier. The file is parsed by first loading the header which contains information about where the data index and data begin. The data index is then parsed to obtain the timezone and the corresponding bytes location in the file to fill the three arrays _ids_byteOffsets_lengths. These arrays are referenced to obtain the corresponding byte data for a timezone, and functions from TimeZoneInfo.Unix.cs are leveraged to create a TimeZoneInfo from there.
@ghost
ghost
commented
Jun 28, 2021

Copy link


Copy Markdown

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.
@mdh1418
mdh1418
marked this pull request as ready for review
June 28, 2021 18:13
@mdh1418
mdh1418
commented
Jun 28, 2021

Copy link


Copy Markdown

MemberAuthor
Unlike other Unix implementations where zone.tab contains meta-data and specific timezone files (Europe/London) contain the actual timezone data, tzdata on Android combines all timezone data together with the metadata into tzdata. As such, more mono implementation is needed to be ported over in order to get TryGetTimeZoneFromLocalMachine to work.
steveisok
steveisok
reviewed
Jun 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

@steveisok
steveisok
commented
Jun 29, 2021

Copy link


Copy Markdown

Member
@mdh1418 As I suspected, there's a little bit more to this. I have more local and we can talk tomorrow about the details.
marek-safar
marek-safar
reviewed
Jun 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

 }
 }

 return Environment.GetEnvironmentVariable("ANDROID_ROOT") + DefaultTimeZoneDirectory;
@marek-safarmarek-safarJun 29, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This look odd. Are you sure we should mix 2 rooted locations?
@mdh1418mdh1418Jul 1, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think I'm missing something, what are the two rooted locations, ANDROID_ROOT and?

The code was adapted from https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73 and https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L488-L495 before AndroidTzData's own Path was introduced into the PR.
@ghost
ghost
commented
Jun 29, 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
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Globalization
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
os-android
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Runtime, os-android
Milestone:-
Steve Pfister
Pulled in parsing implementation from mono and broke out GetLocalTime…
ce91611
…Zone into separate Unix and Android functions.
@steveisoksteveisok
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Jun 29, 2021
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

 */
 private sealed class AndroidTzData
 {
 private unsafe struct AndroidTzDataHeader
@eerhardteerhardtJul 16, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

privateunsafestruct AndroidTzDataHeader
privatereadonlystruct AndroidTzDataHeader
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

error CS8340: Instance fields of readonly structs must be readonly.
/Users/mdhwang/runtime_droid/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs(314,17): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)
I went ahead and removed this struct altogether, using out parameters for indexOffset and dataOffset in LoadTzFile and LoadHeader
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated


Comment on lines

+412
 to 
+414

 Span<byte> buffer = stackalloc byte[length];
 ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath), offset, buffer);
 byte[] tzBuffer = buffer.ToArray();
@eerhardteerhardtJul 16, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

No reason to make a stack buffer here, copy the file into the buffer, and then ToArray it.
  1. It is unnecessary copying
  2. We don't know that the length of the file will fit on the stack. In general, stackalloc's should not be unbounded.

Suggested change

Span<byte>buffer=stackallocbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,buffer);
byte[]tzBuffer=buffer.ToArray();
byte[]tzBuffer=newbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,tzBuffer);
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks. I see, was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span<byte> would suffice?
Is there a general rule for how large a stackalloc should be?
@eerhardteerhardtJul 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span would suffice?
Correct. In LoadTzFile, you just need a buffer to read the file data into, and then you are parsing the values out of it from the buffer. The buffer didn't need to stick around.
Here, since the method expects you to return a byte[], you might as well just read directly into the byte[] you are going to return.
Is there a general rule for how large a stackalloc should be?
Looking around the codebase, we usually don't let a stackalloc go above 1K of memory.
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

marek-safar
marek-safar
approved these changes
Jul 17, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418
mdh1418
commented
Jul 19, 2021

Copy link


Copy Markdown

MemberAuthor
Made a few more changes, including parsing the data entry bytes to construct a string immediately, removing the structs all together, removed unsafe keywords, and updated the description of this PR. Can I get another review @eerhardt@jkotas@steveisok
jkotas
jkotas
reviewed
Jul 19, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418mdh1418
mentioned this pull request
Jul 19, 2021

Closed


This was referenced Jul 19, 2021

Closed


Closed

@mdh1418mdh1418
mentioned this pull request
Jul 20, 2021

Closed

@karelzkarelz
mentioned this pull request
Jul 20, 2021

Closed

@steveisok
steveisok
merged commit 46d9b31
into
dotnet:mainJul 21, 2021
@mdh1418
mdh1418
deleted the

port_android_timezoneinfo

branch
July 26, 2021 12:58
@karelzkarelz
mentioned this pull request
Jul 28, 2021

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 25, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger left review comments
@jkotasjkotasjkotas left review comments
@eerhardteerhardteerhardt left review comments
@marek-safarmarek-safarmarek-safar approved these changes
@steveisoksteveisoksteveisok approved these changes

+1 more reviewer

@grendellogrendellogrendello left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.DateTimeos-android

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Port Android APEX timezone location lookup from mono/mono


10 participants

@mdh1418@steveisok@SamMonoRT@grendello@marek-safar@akoeplinger@jkotas@eerhardt@jeffhandley@maryamariyan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Android][libraries] TimeZoneInfo Android imp - #54845
Merged
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Jul 21, 2021
Merged
[Android][libraries] TimeZoneInfo Android imp#54845
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Conversation
@mdh1418
@mdh1418mdh1418
commented
Jun 28, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path. Moreover, tzdata contains all timezones byte data.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.Unix.cs, TimeZoneInfo.Unix.NonAndroid.cs (non-Android), and TimeZoneInfo.Unix.Android.cs (Android specific)
  2. Adds an interop to obtain the default time zone on Android based on persist.sys.timezone
  3. Implements GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore and GetTimeZoneIds for Android based on mono/mono implementation https://github.com/mono/mono/blob/main/mcs/class/corlib/System/TimeZoneInfo.Android.cs
  4. Adds new string resources to throw exceptions
  5. Refactors the mono/mono implementation of parsing tzdata
Android tzdata files are found in the format of
Header <Beginning of Entry Index> Entry Entry Entry ... Entry <Beginning of Data Index> <TZDATA>
https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/bionic.cpp
The header (24 bytes) contains the following information
signature - 12 bytes of the form "tzdata2012f\0" where 2012f is subject to change
index offset - 4 bytes that denotes the offset at which the index of the tzdata file starts
data offset - 4 bytes that denotes the offset at which the data of the tzdata file starts
final offset - 4 bytes that used to denote the final offset, which we don't use but will note.
Each Data Entry (52 bytes) can be used to generate a TimeZoneInfo and contain the following information
id - 40 bytes that contain the id of the time zone data entry timezone<id>
byte offset - 4 bytes that denote the offset from the data offset timezone<id> data can be found
length - 4 bytes that denote the length of the data for timezone<id>
unused - 4 bytes that used to be raw GMT offset, but now is always 0 since tzdata2014f (L).

When GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore or GetTimeZoneIds are called, an android timezone data instance is instantiated and loaded by attempting to load a tzdata file that can be found at four locations mentioned earlier. The file is parsed by first loading the header which contains information about where the data index and data begin. The data index is then parsed to obtain the timezone and the corresponding bytes location in the file to fill the three arrays _ids_byteOffsets_lengths. These arrays are referenced to obtain the corresponding byte data for a timezone, and functions from TimeZoneInfo.Unix.cs are leveraged to create a TimeZoneInfo from there.
@ghost
ghost
commented
Jun 28, 2021

Copy link


Copy Markdown

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.
@mdh1418
mdh1418
marked this pull request as ready for review
June 28, 2021 18:13
@mdh1418
mdh1418
commented
Jun 28, 2021

Copy link


Copy Markdown

MemberAuthor
Unlike other Unix implementations where zone.tab contains meta-data and specific timezone files (Europe/London) contain the actual timezone data, tzdata on Android combines all timezone data together with the metadata into tzdata. As such, more mono implementation is needed to be ported over in order to get TryGetTimeZoneFromLocalMachine to work.
steveisok
steveisok
reviewed
Jun 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

@steveisok
steveisok
commented
Jun 29, 2021

Copy link


Copy Markdown

Member
@mdh1418 As I suspected, there's a little bit more to this. I have more local and we can talk tomorrow about the details.
marek-safar
marek-safar
reviewed
Jun 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

 }
 }

 return Environment.GetEnvironmentVariable("ANDROID_ROOT") + DefaultTimeZoneDirectory;
@marek-safarmarek-safarJun 29, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This look odd. Are you sure we should mix 2 rooted locations?
@mdh1418mdh1418Jul 1, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think I'm missing something, what are the two rooted locations, ANDROID_ROOT and?

The code was adapted from https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73 and https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L488-L495 before AndroidTzData's own Path was introduced into the PR.
@ghost
ghost
commented
Jun 29, 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
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Globalization
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
os-android
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Runtime, os-android
Milestone:-
Steve Pfister
Pulled in parsing implementation from mono and broke out GetLocalTime…
ce91611
…Zone into separate Unix and Android functions.
@steveisoksteveisok
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Jun 29, 2021
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

 */
 private sealed class AndroidTzData
 {
 private unsafe struct AndroidTzDataHeader
@eerhardteerhardtJul 16, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

privateunsafestruct AndroidTzDataHeader
privatereadonlystruct AndroidTzDataHeader
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

error CS8340: Instance fields of readonly structs must be readonly.
/Users/mdhwang/runtime_droid/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs(314,17): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)
I went ahead and removed this struct altogether, using out parameters for indexOffset and dataOffset in LoadTzFile and LoadHeader
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated


Comment on lines

+412
 to 
+414

 Span<byte> buffer = stackalloc byte[length];
 ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath), offset, buffer);
 byte[] tzBuffer = buffer.ToArray();
@eerhardteerhardtJul 16, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

No reason to make a stack buffer here, copy the file into the buffer, and then ToArray it.
  1. It is unnecessary copying
  2. We don't know that the length of the file will fit on the stack. In general, stackalloc's should not be unbounded.

Suggested change

Span<byte>buffer=stackallocbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,buffer);
byte[]tzBuffer=buffer.ToArray();
byte[]tzBuffer=newbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,tzBuffer);
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks. I see, was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span<byte> would suffice?
Is there a general rule for how large a stackalloc should be?
@eerhardteerhardtJul 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span would suffice?
Correct. In LoadTzFile, you just need a buffer to read the file data into, and then you are parsing the values out of it from the buffer. The buffer didn't need to stick around.
Here, since the method expects you to return a byte[], you might as well just read directly into the byte[] you are going to return.
Is there a general rule for how large a stackalloc should be?
Looking around the codebase, we usually don't let a stackalloc go above 1K of memory.
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

marek-safar
marek-safar
approved these changes
Jul 17, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418
mdh1418
commented
Jul 19, 2021

Copy link


Copy Markdown

MemberAuthor
Made a few more changes, including parsing the data entry bytes to construct a string immediately, removing the structs all together, removed unsafe keywords, and updated the description of this PR. Can I get another review @eerhardt@jkotas@steveisok
jkotas
jkotas
reviewed
Jul 19, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418mdh1418
mentioned this pull request
Jul 19, 2021

Closed


This was referenced Jul 19, 2021

Closed


Closed

@mdh1418mdh1418
mentioned this pull request
Jul 20, 2021

Closed

@karelzkarelz
mentioned this pull request
Jul 20, 2021

Closed

@steveisok
steveisok
merged commit 46d9b31
into
dotnet:mainJul 21, 2021
@mdh1418
mdh1418
deleted the

port_android_timezoneinfo

branch
July 26, 2021 12:58
@karelzkarelz
mentioned this pull request
Jul 28, 2021

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 25, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger left review comments
@jkotasjkotasjkotas left review comments
@eerhardteerhardteerhardt left review comments
@marek-safarmarek-safarmarek-safar approved these changes
@steveisoksteveisoksteveisok approved these changes

+1 more reviewer

@grendellogrendellogrendello left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.DateTimeos-android

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Port Android APEX timezone location lookup from mono/mono


10 participants

@mdh1418@steveisok@SamMonoRT@grendello@marek-safar@akoeplinger@jkotas@eerhardt@jeffhandley@maryamariyan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[Android][libraries] TimeZoneInfo Android imp - #54845
Merged
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Jul 21, 2021
Merged
[Android][libraries] TimeZoneInfo Android imp#54845
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Conversation
@mdh1418
@mdh1418mdh1418
commented
Jun 28, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path. Moreover, tzdata contains all timezones byte data.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.Unix.cs, TimeZoneInfo.Unix.NonAndroid.cs (non-Android), and TimeZoneInfo.Unix.Android.cs (Android specific)
  2. Adds an interop to obtain the default time zone on Android based on persist.sys.timezone
  3. Implements GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore and GetTimeZoneIds for Android based on mono/mono implementation https://github.com/mono/mono/blob/main/mcs/class/corlib/System/TimeZoneInfo.Android.cs
  4. Adds new string resources to throw exceptions
  5. Refactors the mono/mono implementation of parsing tzdata
Android tzdata files are found in the format of
Header <Beginning of Entry Index> Entry Entry Entry ... Entry <Beginning of Data Index> <TZDATA>
https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/bionic.cpp
The header (24 bytes) contains the following information
signature - 12 bytes of the form "tzdata2012f\0" where 2012f is subject to change
index offset - 4 bytes that denotes the offset at which the index of the tzdata file starts
data offset - 4 bytes that denotes the offset at which the data of the tzdata file starts
final offset - 4 bytes that used to denote the final offset, which we don't use but will note.
Each Data Entry (52 bytes) can be used to generate a TimeZoneInfo and contain the following information
id - 40 bytes that contain the id of the time zone data entry timezone<id>
byte offset - 4 bytes that denote the offset from the data offset timezone<id> data can be found
length - 4 bytes that denote the length of the data for timezone<id>
unused - 4 bytes that used to be raw GMT offset, but now is always 0 since tzdata2014f (L).

When GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore or GetTimeZoneIds are called, an android timezone data instance is instantiated and loaded by attempting to load a tzdata file that can be found at four locations mentioned earlier. The file is parsed by first loading the header which contains information about where the data index and data begin. The data index is then parsed to obtain the timezone and the corresponding bytes location in the file to fill the three arrays _ids_byteOffsets_lengths. These arrays are referenced to obtain the corresponding byte data for a timezone, and functions from TimeZoneInfo.Unix.cs are leveraged to create a TimeZoneInfo from there.
@ghost
ghost
commented
Jun 28, 2021

Copy link


Copy Markdown

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.
@mdh1418
mdh1418
marked this pull request as ready for review
June 28, 2021 18:13
@mdh1418
mdh1418
commented
Jun 28, 2021

Copy link


Copy Markdown

MemberAuthor
Unlike other Unix implementations where zone.tab contains meta-data and specific timezone files (Europe/London) contain the actual timezone data, tzdata on Android combines all timezone data together with the metadata into tzdata. As such, more mono implementation is needed to be ported over in order to get TryGetTimeZoneFromLocalMachine to work.
steveisok
steveisok
reviewed
Jun 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

@steveisok
steveisok
commented
Jun 29, 2021

Copy link


Copy Markdown

Member
@mdh1418 As I suspected, there's a little bit more to this. I have more local and we can talk tomorrow about the details.
marek-safar
marek-safar
reviewed
Jun 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

 }
 }

 return Environment.GetEnvironmentVariable("ANDROID_ROOT") + DefaultTimeZoneDirectory;
@marek-safarmarek-safarJun 29, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This look odd. Are you sure we should mix 2 rooted locations?
@mdh1418mdh1418Jul 1, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think I'm missing something, what are the two rooted locations, ANDROID_ROOT and?

The code was adapted from https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73 and https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L488-L495 before AndroidTzData's own Path was introduced into the PR.
@ghost
ghost
commented
Jun 29, 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
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Globalization
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
os-android
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Runtime, os-android
Milestone:-
Steve Pfister
Pulled in parsing implementation from mono and broke out GetLocalTime…
ce91611
…Zone into separate Unix and Android functions.
@steveisoksteveisok
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Jun 29, 2021
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

 */
 private sealed class AndroidTzData
 {
 private unsafe struct AndroidTzDataHeader
@eerhardteerhardtJul 16, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

privateunsafestruct AndroidTzDataHeader
privatereadonlystruct AndroidTzDataHeader
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

error CS8340: Instance fields of readonly structs must be readonly.
/Users/mdhwang/runtime_droid/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs(314,17): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)
I went ahead and removed this struct altogether, using out parameters for indexOffset and dataOffset in LoadTzFile and LoadHeader
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated


Comment on lines

+412
 to 
+414

 Span<byte> buffer = stackalloc byte[length];
 ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath), offset, buffer);
 byte[] tzBuffer = buffer.ToArray();
@eerhardteerhardtJul 16, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

No reason to make a stack buffer here, copy the file into the buffer, and then ToArray it.
  1. It is unnecessary copying
  2. We don't know that the length of the file will fit on the stack. In general, stackalloc's should not be unbounded.

Suggested change

Span<byte>buffer=stackallocbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,buffer);
byte[]tzBuffer=buffer.ToArray();
byte[]tzBuffer=newbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,tzBuffer);
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks. I see, was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span<byte> would suffice?
Is there a general rule for how large a stackalloc should be?
@eerhardteerhardtJul 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span would suffice?
Correct. In LoadTzFile, you just need a buffer to read the file data into, and then you are parsing the values out of it from the buffer. The buffer didn't need to stick around.
Here, since the method expects you to return a byte[], you might as well just read directly into the byte[] you are going to return.
Is there a general rule for how large a stackalloc should be?
Looking around the codebase, we usually don't let a stackalloc go above 1K of memory.
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

marek-safar
marek-safar
approved these changes
Jul 17, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418
mdh1418
commented
Jul 19, 2021

Copy link


Copy Markdown

MemberAuthor
Made a few more changes, including parsing the data entry bytes to construct a string immediately, removing the structs all together, removed unsafe keywords, and updated the description of this PR. Can I get another review @eerhardt@jkotas@steveisok
jkotas
jkotas
reviewed
Jul 19, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418mdh1418
mentioned this pull request
Jul 19, 2021

Closed


This was referenced Jul 19, 2021

Closed


Closed

@mdh1418mdh1418
mentioned this pull request
Jul 20, 2021

Closed

@karelzkarelz
mentioned this pull request
Jul 20, 2021

Closed

@steveisok
steveisok
merged commit 46d9b31
into
dotnet:mainJul 21, 2021
@mdh1418
mdh1418
deleted the

port_android_timezoneinfo

branch
July 26, 2021 12:58
@karelzkarelz
mentioned this pull request
Jul 28, 2021

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 25, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger left review comments
@jkotasjkotasjkotas left review comments
@eerhardteerhardteerhardt left review comments
@marek-safarmarek-safarmarek-safar approved these changes
@steveisoksteveisoksteveisok approved these changes

+1 more reviewer

@grendellogrendellogrendello left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.DateTimeos-android

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Port Android APEX timezone location lookup from mono/mono


10 participants

@mdh1418@steveisok@SamMonoRT@grendello@marek-safar@akoeplinger@jkotas@eerhardt@jeffhandley@maryamariyan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
[Android][libraries] TimeZoneInfo Android imp - #54845
Merged
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Jul 21, 2021
Merged
[Android][libraries] TimeZoneInfo Android imp#54845
steveisok merged 82 commits into
dotnet:mainfrom 
mdh1418:port_android_timezoneinfo
Conversation
@mdh1418
@mdh1418mdh1418
commented
Jun 28, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path. Moreover, tzdata contains all timezones byte data.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.Unix.cs, TimeZoneInfo.Unix.NonAndroid.cs (non-Android), and TimeZoneInfo.Unix.Android.cs (Android specific)
  2. Adds an interop to obtain the default time zone on Android based on persist.sys.timezone
  3. Implements GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore and GetTimeZoneIds for Android based on mono/mono implementation https://github.com/mono/mono/blob/main/mcs/class/corlib/System/TimeZoneInfo.Android.cs
  4. Adds new string resources to throw exceptions
  5. Refactors the mono/mono implementation of parsing tzdata
Android tzdata files are found in the format of
Header <Beginning of Entry Index> Entry Entry Entry ... Entry <Beginning of Data Index> <TZDATA>
https://github.com/aosp-mirror/platform_bionic/blob/master/libc/tzcode/bionic.cpp
The header (24 bytes) contains the following information
signature - 12 bytes of the form "tzdata2012f\0" where 2012f is subject to change
index offset - 4 bytes that denotes the offset at which the index of the tzdata file starts
data offset - 4 bytes that denotes the offset at which the data of the tzdata file starts
final offset - 4 bytes that used to denote the final offset, which we don't use but will note.
Each Data Entry (52 bytes) can be used to generate a TimeZoneInfo and contain the following information
id - 40 bytes that contain the id of the time zone data entry timezone<id>
byte offset - 4 bytes that denote the offset from the data offset timezone<id> data can be found
length - 4 bytes that denote the length of the data for timezone<id>
unused - 4 bytes that used to be raw GMT offset, but now is always 0 since tzdata2014f (L).

When GetLocalTimeZoneCoreTryGetTimeZoneFromLocalMachineCore or GetTimeZoneIds are called, an android timezone data instance is instantiated and loaded by attempting to load a tzdata file that can be found at four locations mentioned earlier. The file is parsed by first loading the header which contains information about where the data index and data begin. The data index is then parsed to obtain the timezone and the corresponding bytes location in the file to fill the three arrays _ids_byteOffsets_lengths. These arrays are referenced to obtain the corresponding byte data for a timezone, and functions from TimeZoneInfo.Unix.cs are leveraged to create a TimeZoneInfo from there.
@ghost
ghost
commented
Jun 28, 2021

Copy link


Copy Markdown

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.
@mdh1418
mdh1418
marked this pull request as ready for review
June 28, 2021 18:13
@mdh1418
mdh1418
commented
Jun 28, 2021

Copy link


Copy Markdown

MemberAuthor
Unlike other Unix implementations where zone.tab contains meta-data and specific timezone files (Europe/London) contain the actual timezone data, tzdata on Android combines all timezone data together with the metadata into tzdata. As such, more mono implementation is needed to be ported over in order to get TryGetTimeZoneFromLocalMachine to work.
steveisok
steveisok
reviewed
Jun 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

@steveisok
steveisok
commented
Jun 29, 2021

Copy link


Copy Markdown

Member
@mdh1418 As I suspected, there's a little bit more to this. I have more local and we can talk tomorrow about the details.
marek-safar
marek-safar
reviewed
Jun 29, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.AnyUnix.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Android.cs
Outdated

 }
 }

 return Environment.GetEnvironmentVariable("ANDROID_ROOT") + DefaultTimeZoneDirectory;
@marek-safarmarek-safarJun 29, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This look odd. Are you sure we should mix 2 rooted locations?
@mdh1418mdh1418Jul 1, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I think I'm missing something, what are the two rooted locations, ANDROID_ROOT and?

The code was adapted from https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73 and https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L488-L495 before AndroidTzData's own Path was introduced into the PR.
@ghost
ghost
commented
Jun 29, 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
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Globalization
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
os-android
Milestone:-
@ghost
ghost
commented
Jun 29, 2021

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
Issue Details
Fixes #41867
Android has removed zone.tab, so TimeZoneInfo.Unix.cs will no longer work properly on Android as GetTimeZoneIds directly depends on the zone.tab file. The chain of dependency is as follows

GetSystemTimeZones -> PopulateAllSystemTimeZones -> GetTimeZoneIds -> zone.tab

TZI.cs TZI.Unix.cs TZI.Unix.cs TZI.Unix.cs

Where TZI is TimeZoneInfo
zone.tab is a file that is found on the unix system under /usr/share/zoneinfo/
GetTimeZoneIds reads zone.tab to obtain the TimeZoneId in that file
PopulateAllSystemTimeZones caches all the TimeZone Ids in cachedData
GetSystemTimeZones returns a ReadOnlyCollection containing all valid TimeZone’s from the local machine, and the entries are sorted by their DisplayName. It relies on cachedData._systemTimeZones being populated.
The problem is that the time zone data for Android can be found in the file tzdata at the possible locations
/apex/com.android.tzdata/etc/tz/tzdata
/apex/com.android.runtime/etc/tz/tzdata
/data/misc/zoneinfo/tzdata
/system/usr/share/zoneinfo/tzdata

The rest of unix the time zone data can be found in the file zone.tab at
 /usr/share/zoneinfo/zone.tab

As zone.tab and tzdata have the same format and contents, Android's TimeZoneInfo implementation should read time zone data from its locations instead of the general /usr/share/zoneinfo/zone.tab path.
This PR achieves the following:
  1. Splits TimeZoneInfo.Unix.cs into TimeZoneInfo.AnyUnix.cs, TimeZoneInfo.Unix.cs (non-Android), and TimeZoneInfo.Android.cs (Android specific)
  2. Modifies which directory is used in PopulateAllSystemTimeZonesGetTimeZoneDirectory() following the order of https://github.com/mono/mono/blob/7791e4d94206dd63acff49268b56a01c0b69983c/mcs/class/corlib/System/TimeZoneInfo.Android.cs#L68-L73
  3. Renames ZoneTabFileName to TimeZoneFileName and split definitions for Android or non-Android (TimeZoneInfo.Unix.cs)
Author:mdh1418
Assignees:-
Labels:
area-System.Runtime, os-android
Milestone:-
Steve Pfister
Pulled in parsing implementation from mono and broke out GetLocalTime…
ce91611
…Zone into separate Unix and Android functions.
@steveisoksteveisok
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Jun 29, 2021
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

 */
 private sealed class AndroidTzData
 {
 private unsafe struct AndroidTzDataHeader
@eerhardteerhardtJul 16, 2021•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

privateunsafestruct AndroidTzDataHeader
privatereadonlystruct AndroidTzDataHeader
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

error CS8340: Instance fields of readonly structs must be readonly.
/Users/mdhwang/runtime_droid/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs(314,17): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)
I went ahead and removed this struct altogether, using out parameters for indexOffset and dataOffset in LoadTzFile and LoadHeader
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated


Comment on lines

+412
 to 
+414

 Span<byte> buffer = stackalloc byte[length];
 ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath), offset, buffer);
 byte[] tzBuffer = buffer.ToArray();
@eerhardteerhardtJul 16, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

No reason to make a stack buffer here, copy the file into the buffer, and then ToArray it.
  1. It is unnecessary copying
  2. We don't know that the length of the file will fit on the stack. In general, stackalloc's should not be unbounded.

Suggested change

Span<byte>buffer=stackallocbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,buffer);
byte[]tzBuffer=buffer.ToArray();
byte[]tzBuffer=newbyte[length];
ReadTzDataIntoBuffer(File.OpenRead(_tzFilePath),offset,tzBuffer);
@mdh1418mdh1418Jul 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks. I see, was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span<byte> would suffice?
Is there a general rule for how large a stackalloc should be?
@eerhardteerhardtJul 18, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

was it unnecessary copying because we need to still return a byte[] here, whereas in LoadTzFile we didn't "need" to use a byte[] and a Span would suffice?
Correct. In LoadTzFile, you just need a buffer to read the file data into, and then you are parsing the values out of it from the buffer. The buffer didn't need to stick around.
Here, since the method expects you to return a byte[], you might as well just read directly into the byte[] you are going to return.
Is there a general rule for how large a stackalloc should be?
Looking around the codebase, we usually don't let a stackalloc go above 1K of memory.
eerhardt
eerhardt
reviewed
Jul 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

marek-safar
marek-safar
approved these changes
Jul 17, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418
mdh1418
commented
Jul 19, 2021

Copy link


Copy Markdown

MemberAuthor
Made a few more changes, including parsing the data entry bytes to construct a string immediately, removing the structs all together, removed unsafe keywords, and updated the description of this PR. Can I get another review @eerhardt@jkotas@steveisok
jkotas
jkotas
reviewed
Jul 19, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
Outdated

@mdh1418mdh1418
mentioned this pull request
Jul 19, 2021

Closed


This was referenced Jul 19, 2021

Closed


Closed

@mdh1418mdh1418
mentioned this pull request
Jul 20, 2021

Closed

@karelzkarelz
mentioned this pull request
Jul 20, 2021

Closed

@steveisok
steveisok
merged commit 46d9b31
into
dotnet:mainJul 21, 2021
@mdh1418
mdh1418
deleted the

port_android_timezoneinfo

branch
July 26, 2021 12:58
@karelzkarelz
mentioned this pull request
Jul 28, 2021

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Aug 25, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@akoeplingerakoeplingerakoeplinger left review comments
@jkotasjkotasjkotas left review comments
@eerhardteerhardteerhardt left review comments
@marek-safarmarek-safarmarek-safar approved these changes
@steveisoksteveisoksteveisok approved these changes

+1 more reviewer

@grendellogrendellogrendello left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.DateTimeos-android

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Port Android APEX timezone location lookup from mono/mono


10 participants

@mdh1418@steveisok@SamMonoRT@grendello@marek-safar@akoeplinger@jkotas@eerhardt@jeffhandley@maryamariyan