Skip to content

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 [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: Ujjwal Sharma [email protected]

@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

@richardlaurichardlau mentioned this pull request Jan 21, 2020
4 tasks
@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#30211Fixes: nodejs#29540 PR-URL: nodejs#30232 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
@jameshilliard
Copy link
Author

Rebased

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

@nodejsnodejs deleted a comment from nodejs-github-botMar 23, 2020
@nodejsnodejs deleted a comment from nodejs-github-botMar 23, 2020
@nodejsnodejs deleted a comment from nodejs-github-botMar 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

@codebyterecodebytereforce-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190CompareMarch 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: #30211Fixes: #29540 Backport-PR-URL: #31433 PR-URL: #30232 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
@MylesBorins
Copy link
Contributor

landed in dca3d29

@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 freeto 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

@jameshilliard@richardlau@nodejs-github-bot@codebytere@MylesBorins@mmarchini@BethGriggs@albertyw