ICU-22541 Fix MacOS 14 default timezone issue by FrankYFTang · Pull Request #2669 · unicode-org/icu

FrankYFTang · 2023-10-11T22:20:59Z

Checklist

Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22541
Required: The PR title must be prefixed with a JIRA Issue number.
Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
Required: Each commit message must be prefixed with a JIRA Issue number.
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

FrankYFTang · 2023-10-11T22:22:13Z

@Robbos @syg

markusicu

lgtm

@richgillam @pedberg-icu et al. ok?

srl295 · 2023-10-11T22:54:45Z

Sigh, if only tzdb would have agreed (I asked) to include an actual ID in the file. Instead, we deal with randomness such as ../posixrules

syg · 2023-10-11T23:05:29Z

This might not be the right place to discuss: does this fix the Ubuntu 18.04 breakage we saw with Chrome, which was due to /etc/localtime -> /usr/share/zoneinfo/America/New_York -> /usr/share/zoneinfo/posixrules? (To compare, my Linux box (Google's corp Linux distro) has /usr/share/zoneinfo/posixrules -> /usr/share/zoneinfo/America/New_York instead.)

Edit: Rephrased as question.

srl295 · 2023-10-11T23:08:27Z

This might not be the right place to discuss: this won't fix the Ubuntu 18.04 breakage we saw with Chrome,

it seems like it is the right place to discuss since the ticket claims to be about that specific breakage

markusicu · 2023-10-11T23:16:01Z

does this fix the Ubuntu 18.04 breakage we saw with Chrome, which was due to /etc/localtime -> /usr/share/zoneinfo/America/New_York -> /usr/share/zoneinfo/posixrules?

I think not, because we should see tzZoneInfoTailPtr != nullptr.
We should be able to fix that by changing the check to

if (tzZoneInfoTailPtr == nullptr ||
        uprv_strcmp(tzZoneInfoTailPtr + tzZoneInfoTailLen, "posixrules") == 0) {

richgillam

I think this looks okay, but I'm interested in what Debbie Goldsmith has to say in the email discussion that accompanied this PR. From talking to Peter, it sounds possible that we're doing something weird in the way we write the time zone files to the disk, and that therefore the issue might not be with ICU so much as with our own time-zone-updating code.

This is all stuff I personally know diddly-squat about, but with Peter leaving, I guess I'll have to learn. But I think this is probably okay, at least for now.

See unicode-org#2669

jira-pull-request-webhook · 2023-10-12T16:30:21Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

markusicu · 2023-10-12T18:21:28Z

Test failures. Please change icu4c/source/test/depstest/dependencies.txt:

line 25: remove realpath_function
lines 113..115: remove the realpath_function group
line 117: add readlink realpath after readdir
line 871: remove realpath_function

jira-pull-request-webhook · 2023-10-12T21:51:51Z

Notice: the branch changed across the force-push!

icu4c/source/common/putil.cpp is different
icu4c/source/test/depstest/dependencies.txt is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

richgillam · 2023-10-12T22:31:05Z

FWIW, I added commentary to the JIRA ticket explaining the underlying cause and recommended solution for this issue.

richgillam

I can't speak to Markus's comments, but Frank's logic here looks correct to me. Based on my investigation of the code (see my comments in the JIRA ticket), I think that algorithm will work perfectly well as a long-term solution.

markusicu

Please un-revert the "posixrules" change that you accepted yesterday and reverted today.

FrankYFTang · 2023-10-12T23:33:58Z

opps.. I commit it on github, and then forget to pull into my local branch, and then I added the dependecy stuff and force push, causing the revert. Sorry.

See unicode-org#2669

jira-pull-request-webhook · 2023-10-12T23:34:49Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

markusicu

thanks!

Import from https://unicode-org.atlassian.net/browse/ICU-22541 (unicode-org/icu#2669). Differential Revision: https://phabricator.services.mozilla.com/D191013

Import from https://unicode-org.atlassian.net/browse/ICU-22541 (unicode-org/icu#2669). Differential Revision: https://phabricator.services.mozilla.com/D191013 UltraBlame original commit: 91657cb1296c904004ace38877b9ca91f502be3f

Import from https://unicode-org.atlassian.net/browse/ICU-22541 (unicode-org/icu#2669). Differential Revision: https://phabricator.services.mozilla.com/D191013

…=dmeehan Import from https://unicode-org.atlassian.net/browse/ICU-22541 (unicode-org/icu#2669). Differential Revision: https://phabricator.services.mozilla.com/D191013

See unicode-org#2669

…=dmeehan Import from https://unicode-org.atlassian.net/browse/ICU-22541 (unicode-org/icu#2669). Differential Revision: https://phabricator.services.mozilla.com/D191013

Import from https://unicode-org.atlassian.net/browse/ICU-22541 (unicode-org/icu#2669). Differential Revision: https://phabricator.services.mozilla.com/D191013

FrankYFTang requested review from markusicu and richgillam October 11, 2023 22:21

markusicu requested review from garywade, pedberg-icu, srl295 and yumaoka October 11, 2023 22:39

markusicu previously approved these changes Oct 11, 2023

View reviewed changes

Comment thread icu4c/source/common/putil.cpp

srl295 previously approved these changes Oct 11, 2023

View reviewed changes

markusicu reviewed Oct 11, 2023

View reviewed changes

Comment thread icu4c/source/common/putil.cpp Outdated

richgillam previously approved these changes Oct 12, 2023

View reviewed changes

FrankYFTang dismissed stale reviews from richgillam, srl295, and markusicu via 119a18a October 12, 2023 16:28

FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Oct 12, 2023

ICU-22541 Fix MacOS 14 default timezone issue

7d58692

See unicode-org#2669

FrankYFTang force-pushed the ICU-22541-fixTZreadlink branch from 119a18a to 7d58692 Compare October 12, 2023 16:30

FrankYFTang requested review from markusicu, richgillam and srl295 October 12, 2023 16:31

markusicu self-assigned this Oct 12, 2023

FrankYFTang force-pushed the ICU-22541-fixTZreadlink branch from 7d58692 to dad7f7c Compare October 12, 2023 21:51

richgillam previously approved these changes Oct 12, 2023

View reviewed changes

markusicu requested changes Oct 12, 2023

View reviewed changes

FrankYFTang dismissed richgillam’s stale review via eb7ec5e October 12, 2023 23:32

ICU-22541 Fix MacOS 14 default timezone issue

87c3133

See unicode-org#2669

FrankYFTang force-pushed the ICU-22541-fixTZreadlink branch from eb7ec5e to 87c3133 Compare October 12, 2023 23:34

markusicu approved these changes Oct 12, 2023

View reviewed changes

FrankYFTang merged commit ca631ea into unicode-org:maint/maint-74 Oct 13, 2023

FrankYFTang deleted the ICU-22541-fixTZreadlink branch October 13, 2023 17:38

DavidDudson mentioned this pull request Oct 22, 2023

Intl.DateTimeFormat().resolvedOptions().timeZone undefined on MacOS 14.0 (Sonoma) nodejs/node#50301

Closed

MrJithil pushed a commit to MrJithil/icu that referenced this pull request Oct 25, 2023

ICU-22541 Fix MacOS 14 default timezone issue

49dde0d

See unicode-org#2669

MrJithil pushed a commit to MrJithil/icu that referenced this pull request Oct 25, 2023

ICU-22541 Fix MacOS 14 default timezone issue

81e26e0

See unicode-org#2669

MrJithil mentioned this pull request Oct 25, 2023

[DRAFT]: ICU-22541 Fix MacOS 14 default timezone issue #2681

Draft

7 tasks

Uh oh!

Conversation

FrankYFTang commented Oct 11, 2023 • edited by markusicu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

FrankYFTang commented Oct 11, 2023

Uh oh!

markusicu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srl295 commented Oct 11, 2023

Uh oh!

syg commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 commented Oct 11, 2023

Uh oh!

markusicu commented Oct 11, 2023

Uh oh!

Uh oh!

richgillam left a comment

Choose a reason for hiding this comment

Uh oh!

jira-pull-request-webhook Bot commented Oct 12, 2023

Uh oh!

markusicu commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jira-pull-request-webhook Bot commented Oct 12, 2023

Uh oh!

richgillam commented Oct 12, 2023

Uh oh!

richgillam left a comment

Choose a reason for hiding this comment

Uh oh!

markusicu left a comment

Choose a reason for hiding this comment

Uh oh!

FrankYFTang commented Oct 12, 2023

Uh oh!

jira-pull-request-webhook Bot commented Oct 12, 2023

Uh oh!

markusicu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FrankYFTang commented Oct 11, 2023 •

edited by markusicu

Loading

syg commented Oct 11, 2023 •

edited

Loading

markusicu commented Oct 12, 2023 •

edited

Loading