nodejs / Release

Node.js Release Working Group
4.03k stars 570 forks source link

Imminent issue for Morocco users (tzdata2020a) #576

Closed apaprocki closed 2 years ago

apaprocki commented 4 years ago

ICU 66.1 only includes IANA time zone data tzdata2019c: https://github.com/unicode-org/icu/blob/maint/maint-66/icu4c/source/data/misc/zoneinfo64.txt#L8

Upstream tzdata2020a was released to address the prior incorrect assumed temporary reversion of DST in Morocco due to Ramadan (eggert/tz@f9aacafa4f15be6e7ff4436164e14d8f08839ac7).

ICU merged unicode-org/icu@a951ab59c7ceba3ca0873ac16984426bfbf62725 and released updated .res files for 2020a about 9 days ago. Unless the data files are manually updated and a new release is cut, users in Morocco and Intl API usage with zone Africa/Casablanca will see the time off by an hour between May 24th and May 31st.

apaprocki commented 4 years ago

The tz-announce mailing list announces new releases, so is a good source to monitor: https://mm.icann.org/mailman/listinfo/tz-announce

Those then have to be packaged up and released as ICU binaries: https://unicode-org.atlassian.net/browse/ICU-21094 https://github.com/unicode-org/icu-data/pull/11 https://github.com/unicode-org/icu-data/tree/master/tzdata/icunew/2020a/44

There's usually a day (or few) lag between the two.

srl295 commented 4 years ago

ICU won't re-release 67 including this. For this we need… actually, we need to follow the process in your PR https://github.com/nodejs/node/pull/30364 :)

but that won't help full-icu: for that we probably need some kind of side-load timezone process, which I mentioned in nodejs/node#32466

apaprocki commented 4 years ago

actually, we need to follow the process in your PR

Yeah, I pinged @MylesBorins with this and let him know it would require that same patching approach to get it out in time.

but that won't help full-icu: for that we probably need some kind of side-load timezone process

FWIW I do that with our build of Node, using the out-of-band .res files on disk and the environment variable support to override the directory if necessary.

codebytere commented 4 years ago

I have a patch locally which applies changes for this issue as specified in @apaprocki's doc PR - shall i go ahead and open that?

srl295 commented 4 years ago

Sounds good. That env var support could be documented too.

srl295 commented 4 years ago

~PR LGTM~. Edit The pull request https://github.com/nodejs/node/pull/33362 looks good. Since this is an imminent issue, I wanted to give watchers here a headsup.

BethGriggs commented 4 years ago

We discussed this in the release meeting today (#580).

nodejs/node#33362 has now landed on master. The PR has also been cherry-picked on to v14.x-staging, ready to go out in the next v14.x release. Unfortunately, that is unlikely to happen before the 24th May (based on our current release schedule).

srl295 commented 4 years ago

@BethGriggs Thanks for the update. It's most critical right at that time of course, but can affect historical calculations also.

richardlau commented 2 years ago

The updated timezone went out in 14.15.0. Node.js 12 hasn't been updated and is currently on timezone data 2019c (ICU 67). A manual backport is required as Node.js 12 is still built with small-icu by default while later release lines are built with full-icu.

richardlau commented 2 years ago

Given that Node.js 12 is in maintenance, does anyone have an opinion on updating the timezone information vs leaving it as-is?

albertyw commented 2 years ago

We're running up to a repeat of this issue with Jordanian users (among other locales). tzdata 2021b (aka 2021a1) updates Jordan's DST taking effect on February 24, 2022 (https://mm.icann.org/pipermail/tz/2021-September/030760.html). It looks like the security releases yesterday didn't update node's ICU files on the LTS versions even though https://github.com/nodejs/node/commit/de6399c0c07bda22bc39a74e4fa8b7e391dfacea and Node v17 are up-to-date.

Would it be possible to get Node v12, v14, and v16 released with updated versions of tzdata as soon as possible, else we would need to follow the env var override mentioned above.

richardlau commented 2 years ago

We're running up to a repeat of this issue with Jordanian users (among other locales). tzdata 2021b (aka 2021a1) updates Jordan's DST taking effect on February 24, 2022 (https://mm.icann.org/pipermail/tz/2021-September/030760.html). It looks like the security releases yesterday didn't update node's ICU files on the LTS versions even though nodejs/node@de6399c and Node v17 are up-to-date.

Would it be possible to get Node v12, v14, and v16 released with updated versions of tzdata as soon as possible, else we would need to follow the env var override mentioned above.

We discussed this in yesterday's Release meeting and the plan is to try landing the ICU 70.1 update (containing the updated tz db) on Node.js 16 and 14. Node.js 12 needs a PR to update the timezone db (following https://github.com/nodejs/node/blob/master/doc/guides/maintaining-icu.md#time-zone-data) because Node.js 12 is built with small-icu so we cannot cherry-pick the ICU updates to it.

albertyw commented 2 years ago

Would https://github.com/nodejs/node/pull/41443 be the PR needed for Node.js 12?

richardlau commented 2 years ago

Would nodejs/node#41443 be the PR needed for Node.js 12?

Yes, thanks! I'd missed this -- probably because it was closed (presumably accidentally) until you reopened it just now.