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

[v12.x backport] tools: update icu to 65.1 #31433

Conversation

jameshilliard
Copy link

Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: #30211
Fixes: #29540

PR-URL: #30232
Reviewed-By: Steven R Loomis srloomis@us.ibm.com
Reviewed-By: Michael Dawson michael_dawson@ca.ibm.com
Reviewed-By: Ujjwal Sharma usharma1998@gmail.com

@richardlau
Copy link
Member

richardlau commented Jan 21, 2020

Can you double check the icu data file (icudt65l.dat)? Based on the size of the file removed (icudt64l.dat, 2.85 MB) and the replacement (icudt65l.dat, 26.7 MB) would suggest that this is no longer small-icu but full-icu. We switched to full-icu in master/13.x in #29522 but that's semver-major and won't be backported to 12.x. It may well be that because of that (small-icu vs full-icu) for 12.x it would make more sense to do the ICU upgrade following the steps in https://github.com/nodejs/node/blob/v12.x/tools/icu/README.md rather than trying to backport the upgrade from master.

https://github.com/nodejs/node/compare/a1648b8e175cc0a874fd9c49c04bcf753e940921..ecc4c3cf8498406c33bddb7ec17b1790cafcb0ba

image

@richardlau richardlau mentioned this pull request Jan 21, 2020
4 tasks
@jameshilliard jameshilliard force-pushed the backport-30232-to-v12.x branch from ecc4c3c to fad6f5e Compare January 21, 2020 04:26
@jameshilliard
Copy link
Author

@richardlau That look better now?

@nodejs-github-bot
Copy link
Collaborator

@jameshilliard
Copy link
Author

does this need to be rebased?

Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: nodejs#30211
Fixes: nodejs#29540

PR-URL: nodejs#30232
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@jameshilliard jameshilliard force-pushed the backport-30232-to-v12.x branch from fad6f5e to 8e9e7d7 Compare February 17, 2020 23:38
@jameshilliard
Copy link
Author

Rebased

@ex1st ex1st mentioned this pull request Mar 19, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs nodejs deleted a comment from nodejs-github-bot Mar 23, 2020
@nodejs nodejs deleted a comment from nodejs-github-bot Mar 23, 2020
@nodejs nodejs deleted a comment from nodejs-github-bot Mar 23, 2020
@codebytere
Copy link
Member

the failing test is this:

10:50:07 not ok 1123 parallel/test-http2-reset-flood
10:50:07   ---
10:50:07   duration_ms: 0.419
10:50:07   severity: fail
10:50:07   exitcode: 1
10:50:07   stack: |-
10:50:07     events.js:287
10:50:07           throw er; // Unhandled 'error' event
10:50:07           ^
10:50:07     
10:50:07     Error: This socket has been ended by the other party
10:50:07         at Socket.writeAfterFIN [as write] (net.js:452:14)
10:50:07         at Immediate.writeRequests (/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora-last-latest-x64/test/parallel/test-http2-reset-flood.js:72:12)
10:50:07         at processImmediate (internal/timers.js:456:21)
10:50:07     Emitted 'error' event on Socket instance at:
10:50:07         at emitErrorNT (net.js:1340:8)
10:50:07         at processTicksAndRejections (internal/process/task_queues.js:84:21) {
10:50:07       code: 'EPIPE'
10:50:07     }
10:50:07   ...

i don't believe it's related but it's happened a few times in a row so would someone mind confirming that?

@nodejs-github-bot
Copy link
Collaborator

@codebytere codebytere force-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190 Compare March 31, 2020 23:57
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: #30211
Fixes: #29540

Backport-PR-URL: #31433
PR-URL: #30232
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@MylesBorins
Copy link
Contributor

landed in dca3d29

@MylesBorins MylesBorins closed this Apr 1, 2020
@mmarchini
Copy link
Contributor

It seems like this PR broke the full-icu module due to icu filename change (nodejs/full-icu-npm#46). Should this be considered a breaking change, or is this filename considered private API?

@codebytere
Copy link
Member

I would say this is acceptable breakage (the latter), considering that this is resolvable on the ecosystem end with a new version publish; 12.x is still shipping small-icu by default so this just needs a new publish of full-icu data. I'm planning on coordinating asap - the full-icu repo doesn't have public issues and i wasn't aware of the alternate location at https://github.com/unicode-org/full-icu-npm until i saw this so I'll start there.

@MylesBorins
Copy link
Contributor

@mmarchini fwiw the issue was just that the binary for the new version of full-icu needed to be published. It has been published and it it is working now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants