Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: update timezone to 2023c #47680

Closed
wants to merge 1 commit into from
Closed

Conversation

nodejs-github-bot
Copy link
Collaborator

This PR was generated by tools/timezone-update.yml.

Updates the ICU files as per the instructions present in https://github.com/nodejs/node/blob/main/doc/contributing/maintaining-icu.md#time-zone-data

To test, build node off this branch & log the version of tz using

console.log(process.versions.tz)

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Apr 23, 2023
@richardlau
Copy link
Member

Kind of surpised by this one... ICU 73.1 already contains timezone 2023c.

@Trott
Copy link
Member

Trott commented Apr 23, 2023

Kind of surpised by this one... ICU 73.1 already contains timezone 2023c.

The PR/commit message is misleading, but it does appear to remove an unneeded file, I think?

@targos
Copy link
Member

targos commented Apr 24, 2023

I doesn't remove the file. It reduces its size (probably by removing some data in it).

@targos
Copy link
Member

targos commented Apr 24, 2023

/cc @srl295

@srl295
Copy link
Member

srl295 commented Apr 24, 2023

Kind of surpised by this one... ICU 73.1 already contains timezone 2023c.

It should perhaps check process.versions.tz before running.

@srl295
Copy link
Member

srl295 commented Apr 24, 2023

I doesn't remove the file. It reduces its size (probably by removing some data in it).

Maybe just a re pack. I'll check it sometime today.

@srl295
Copy link
Member

srl295 commented Apr 24, 2023

in tools/update-timezone.mjs

renameSync(`icu-data/tzdata/icunew/${latestVersion}/44/le/${file}`, `deps/icu-small/source/data/in/${file}`);

this should be a copy instead of a move. Otherwise it's destructive to the icu-data repo :)

@srl295
Copy link
Member

srl295 commented Apr 24, 2023

uncompressed the files are identical in size (but different in content) so the 500 byte 'reduction' was a recompression.

$ ls -ld icudt73l*
-rw-r--r-- 1 srl295 wheel 31998544 Apr 24 09:44 icudt73l.dat
-rw-r--r-- 1 srl295 wheel 31998544 Apr 24 09:45 icudt73lPLUS.dat

./tools/icu/shrink-icu-src.py has

# compression stuff. Keep the suffix and the compression function in sync.
compression_suffix = '.bz2'
def compress_data(infp, outfp):
    with open(infp, 'rb') as inf:
        with bz2.BZ2File(outfp, 'wb') as outf:
            shutil.copyfileobj(inf, outf)

whereas tools/update-timezone.mjs has

execSync('bzip2 -z deps/icu-small/source/data/in/icudt*.dat');

@srl295
Copy link
Member

srl295 commented Apr 24, 2023

also see #47302

@srl295
Copy link
Member

srl295 commented Apr 24, 2023

ok my analysis tools are in #47248 (comment) (document somewhere?)

$ diff ICU0/timezoneTypes.txt ICU1/timezoneTypes.txt
10d9
<             camtr { "cator" }
35d33
<             America:Montreal { "America/Toronto" }
308a307
>             America:Montreal { "camtr" }

this shouldn't have made a change, but it did. Investigating.

Update This is a bad change (regression).

@srl295
Copy link
Member

srl295 commented Apr 24, 2023

@yumaoka any ideas here? Node.js has ICU4C 73.1, and this script "updated" it using https://github.com/unicode-org/icu-data/tree/main/tzdata/icunew/2023c/44 (It's a bug, because 73.1 is already at 2023c)

But it looks like the icu-data repository doesn't reflect unicode-org/icu@2c584ab#diff-6b5713193269b4b1d788bdb509f542cf6c6148ea8821174310b73ecf86b0b1cd

Is icu-data out of date?

Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

Reverts data, So this should not be approved but closed.
(But need to sort out the tool and data issues)

@srl295
Copy link
Member

srl295 commented Apr 24, 2023

Is icu-data out of date?

Probably a better way to say it is: icu-data's 2023c update is valid for ICU4C 72.1 as it does contain tzdata 2023c. however, it doesn't include the CLDR 43 changes that went into ICU4C 73.1. Since ICU4C 73.1 already included 2023c, one wouldn't expect users to update from 2023c "to" 2023c. So it is kind of an edge case.

If there was a 2023d (always likely), it will undoubtably contain the CLDR 43 changes.

Anyway, I propose to close this PR and fix issues in #47702

@Trott Trott closed this Apr 25, 2023
@Trott Trott deleted the actions/timezone-update branch April 25, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants