[Android] Filter legacy time zones without unversioned tzlookup.xml - #132401
Open
davidnguyen-tech wants to merge 5 commits into
Open
[Android] Filter legacy time zones without unversioned tzlookup.xml#132401davidnguyen-tech wants to merge 5 commits into
davidnguyen-tech wants to merge 5 commits into
Conversation
Prefer Android's versioned tzlookup metadata and fall back to ICU canonical location IDs when that metadata is unavailable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4da39873-ad89-41cf-a5dd-9753c3ab7f91
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Android time zone enumeration so TimeZoneInfo.GetSystemTimeZones() can filter legacy/backward aliases more reliably by preferring Android’s versioned tzdata directories when present and falling back when tzlookup.xml isn’t available, while re-enabling related regression tests.
Changes:
- Prefer the APEX versioned tzdata directory (
etc/tz/versioned/<format-major>) when available, with fallback directory probing. - Add an ICU-based native/managed fallback path to obtain canonical-location time zone IDs when
tzlookup.xmlis missing/unreadable. - Re-enable Android time zone regression tests by removing
ActiveIssuegating.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Globalization.Native/pal_timeZoneInfo.h | Declares a new native export for enumerating canonical-location zone IDs. |
| src/native/libs/System.Globalization.Native/pal_timeZoneInfo.c | Implements ICU-backed enumeration of canonical-location time zone IDs. |
| src/native/libs/System.Globalization.Native/pal_placeholders.c | Adds a placeholder stub for the new native export on unsupported platforms. |
| src/native/libs/System.Globalization.Native/entrypoints.c | Registers the new export in the globalization native entrypoint table (non-wasm). |
| src/libraries/Common/src/Interop/Interop.TimeZoneInfo.cs | Adds LibraryImport for the new native canonical-location IDs export. |
| src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs | Uses versioned tzdata directory when available and falls back to ICU canonical-location IDs when tzlookup.xml is missing/unreadable. |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/TimeZoneInfoTests.cs | Re-enables Android time zone regression tests by removing ActiveIssue exclusions. |
Member
Author
|
/azp run runtime-extra-platforms Note This pipeline trigger was posted by GitHub Copilot at the user's request. |
|
No pipelines are associated with this pull request. |
tarekgh
added
os-android
area-System.DateTime
and removed
area-System.Globalization
labels
Resolve the versioned tzlookup path from the module's tz_version file instead of maintaining an Android API mapping. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4da39873-ad89-41cf-a5dd-9753c3ab7f91
Parse only the length returned by the second enumeration call and keep the Apple placeholder's failure sentinel consistent with the native contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4da39873-ad89-41cf-a5dd-9753c3ab7f91
Keep the new link-time placeholder's neutral return value consistent with the existing globalization placeholders. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4da39873-ad89-41cf-a5dd-9753c3ab7f91
Return -1 from every failure path because managed callers only distinguish success lengths from failure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4da39873-ad89-41cf-a5dd-9753c3ab7f91
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes Android time zone filtering behavior across OS/API variants and adds a new native/managed interop path, which warrants final human review despite no concrete issues found in the diff.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tzlookup.xmlmetadatatzlookup.xmlis missing or unreadableThis removes legacy aliases from
TimeZoneInfo.GetSystemTimeZones()without changing direct lookup throughFindSystemTimeZoneById.Fixes #90269
Fixes #117731
Validation
System.Runtime.Testson a physical Android 16 / API 36 arm64 device: 77,575 run, 0 failedNote
This pull request was prepared with GitHub Copilot.