-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
tools: automate icu update #47727
tools: automate icu update #47727
Conversation
Review requested:
|
7261c4a
to
64c627c
Compare
You can remove the |
64c627c
to
1eaaa62
Compare
tools/dep_updaters/update-icu.sh
Outdated
|
||
rm -rf "$DEPS_DIR/icu" | ||
|
||
CHECKSUM=$(curl -s "$NEW_VERSION_TGZ" | md5sum | cut -d ' ' -f1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be validating this checksum matches the appropriate one from
https://github.com/unicode-org/icu/releases/download/release-${NEW_VERSION}/icu4c-${NEW_MAJOR_VERSION}_${NEW_MINOR_VERSION}-src.md5
(We've had mismatches before.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and even check the GPG key against KEYS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check the generated checksum and the one deposited match? if they dont match we skip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be my recommendation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll likely need to update ./test/fixtures/tz-version.txt
to match the timezone version shipped in the new version of ICU.
Refs: #47456 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and there are potential 'golden' value breakage, but not much to do about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont we have an action that does this? #47302
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marco-ippolito The timezone data gets baked into the ICU data file, which updating ICU will overwrite. If ./test/fixtures/tz-version.txt
doesn't match the timezone data version in the ICU data file then the associated test will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have managed to get the latest version from https://data.iana.org/time-zones/releases/ but its a bit hacky @srl295 do you know any api we could fetch the latest version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you should do is extract the tz version from the ICU you're updating to, and put that in tz-version.txt
.
Maybe
$ node -p process.versions.tz | tee test/fixtures/tz-version.txt
And then in reviewing the PR make sure it's not a regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that will work because the update script added here doesn't rebuild Node.js, so that will output the tz version of whatever node
binary is on the runner instead of the one from the data. I presume the tz version must be in the updated ICU files but maybe it'll be too complicated to extract without building Node.js with the new data?
I suppose we could ignore it and fix up manually if the opened PR breaks because of a different tz version in the new ICU data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardlau it ought to build to make sure node works… the PR will do that, though. the tz version is deep in the gzipped binary .dat file.
Yes, it could be ignored and fixed manually . As i said it's likely there will be some manual tweaks as Node has been made more sensitive to these changes.
In that case, just don't modify tz-version.txt at all in this workflow.
cc FYI @srl295 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with issues noted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, just needs some tweaking
- Also should update the GUIDE to mention this workflow.
debe898
to
44862bf
Compare
except that the in-repo one still has an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you should do is extract the tz version from the ICU you're updating to, and put that in tz-version.txt
.
Maybe
$ node -p process.versions.tz | tee test/fixtures/tz-version.txt
And then in reviewing the PR make sure it's not a regression.
Two requests (to recap):
|
Also note that tz-version isn't the only thing that will break, there may be other test failures. |
@srl295 should be good now |
Commit Queue failed- Loading data for nodejs/node/pull/47727 ✔ Done loading data for nodejs/node/pull/47727 ----------------------------------- PR info ------------------------------------ Title tools: automate icu update (#47727) Author Marco Ippolito (@marco-ippolito) Branch marco-ippolito:feat/automate-icu-update -> nodejs:main Labels meta, tools Commits 9 - tools: automate icu-small update - fix: replace - with . - fix: md5 from source - fix: comparing checksum - fix: mention the script in the maintaining guide - fix: lint - fix: update timezone.txt - fix: remove tz update - fix: use version as whole Committers 1 - Marco Ippolito PR-URL: https://github.com/nodejs/node/pull/47727 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Steven R Loomis Reviewed-By: Rafael Gonzaga ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47727 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Steven R Loomis Reviewed-By: Rafael Gonzaga -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 26 Apr 2023 14:31:28 GMT ✔ Approvals: 2 ✔ - Steven R Loomis (@srl295): https://github.com/nodejs/node/pull/47727#pullrequestreview-1418824208 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/47727#pullrequestreview-1417161365 ✔ Last GitHub CI successful ✘ No Jenkins CI runs detected -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/4927566555 |
Commit Queue failed- Loading data for nodejs/node/pull/47727 ✔ Done loading data for nodejs/node/pull/47727 ----------------------------------- PR info ------------------------------------ Title tools: automate icu update (#47727) Author Marco Ippolito (@marco-ippolito) Branch marco-ippolito:feat/automate-icu-update -> nodejs:main Labels meta, tools Commits 9 - tools: automate icu-small update - fix: replace - with . - fix: md5 from source - fix: comparing checksum - fix: mention the script in the maintaining guide - fix: lint - fix: update timezone.txt - fix: remove tz update - fix: use version as whole Committers 1 - Marco Ippolito PR-URL: https://github.com/nodejs/node/pull/47727 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Steven R Loomis Reviewed-By: Rafael Gonzaga ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47727 Refs: https://github.com/nodejs/security-wg/issues/828 Reviewed-By: Steven R Loomis Reviewed-By: Rafael Gonzaga -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 26 Apr 2023 14:31:28 GMT ✔ Approvals: 2 ✔ - Steven R Loomis (@srl295): https://github.com/nodejs/node/pull/47727#pullrequestreview-1418824208 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/47727#pullrequestreview-1417161365 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-05-09T17:03:40Z: https://ci.nodejs.org/job/node-test-pull-request/51718/ - Querying data for job/node-test-pull-request/51718/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 47727 From https://github.com/nodejs/node * branch refs/pull/47727/merge -> FETCH_HEAD ✔ Fetched commits as 33231b0e8908..15ff4c572aa6 -------------------------------------------------------------------------------- Auto-merging .github/workflows/tools.yml [main 4f311994aa] tools: automate icu-small update Author: Marco Ippolito Date: Wed Apr 26 12:06:22 2023 +0200 3 files changed, 80 insertions(+) create mode 100755 tools/dep_updaters/update-icu.sh mode change 100644 => 100755 tools/icu/shrink-icu-src.py [main da76c2a76b] fix: replace - with . Author: Marco Ippolito Date: Wed Apr 26 18:03:27 2023 +0200 1 file changed, 4 insertions(+), 6 deletions(-) [main bdff48f4f9] fix: md5 from source Author: Marco Ippolito Date: Thu Apr 27 10:02:04 2023 +0200 1 file changed, 9 insertions(+), 4 deletions(-) [main 6c92d489d5] fix: comparing checksum Author: Marco Ippolito Date: Thu Apr 27 10:49:07 2023 +0200 1 file changed, 9 insertions(+) [main 3301ee5878] fix: mention the script in the maintaining guide Author: Marco Ippolito Date: Thu Apr 27 11:04:49 2023 +0200 1 file changed, 3 insertions(+) [main 361a6a53cc] fix: lint Author: Marco Ippolito Date: Thu Apr 27 12:05:29 2023 +0200 1 file changed, 1 insertion(+), 1 deletion(-) [main 8dc41afd62] fix: update timezone.txt Author: Marco Ippolito Date: Fri Apr 28 15:39:45 2023 +0200 1 file changed, 10 insertions(+) [main 03215a2cd9] fix: remove tz update Author: Marco Ippolito Date: Tue May 2 08:50:23 2023 +0200 1 file changed, 10 deletions(-) [main ca71746872] fix: use version as whole Author: Marco Ippolito Date: Tue May 2 09:02:24 2023 +0200 1 file changed, 5 insertions(+), 5 deletions(-) ✔ Patches applied There are 9 commits in the PR. Attempting autorebase. Rebasing (2/18)https://github.com/nodejs/node/actions/runs/4929182555 |
Landed in 1b17793 |
PR-URL: #47727 Refs: nodejs/security-wg#828 Reviewed-By: Steven R Loomis <srl295@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: #47727 Refs: nodejs/security-wg#828 Reviewed-By: Steven R Loomis <srl295@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs#47727 Refs: nodejs/security-wg#828 Reviewed-By: Steven R Loomis <srl295@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Refs: nodejs/security-wg#828