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

tools: Update icu to 65.1 #30232

Closed
wants to merge 1 commit into from
Closed

Conversation

albertyw
Copy link
Contributor

@albertyw albertyw commented Nov 3, 2019

Fixes #29540, #30211.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@albertyw
Copy link
Contributor Author

albertyw commented Nov 3, 2019

I followed the upgrade instructions in https://github.com/nodejs/node/blob/master/tools/icu/README.md. I'm having trouble getting tests to run correctly locally. I'm hoping that CI will validate the correctness of this PR.

@hiagodotme
Copy link

hiagodotme commented Nov 3, 2019

@albertyw I'm having production issues because of this. How did you solve this problem?

@targos
Copy link
Member

targos commented Nov 3, 2019

@nodejs/intl

@nodejs-github-bot
Copy link
Collaborator

@gengjiawen gengjiawen requested a review from ryzokuken November 5, 2019 01:29
@gengjiawen gengjiawen assigned srl295 and unassigned srl295 Nov 5, 2019
@gengjiawen gengjiawen requested a review from srl295 November 5, 2019 01:29
@nodejs-github-bot
Copy link
Collaborator

@GitArika
Copy link

GitArika commented Nov 7, 2019

When this update arrives, will nodejs LTS v8 and v10 also change?

APIs on AWS are being affected by this issue.

@richardlau
Copy link
Member

When this update arrives, will nodejs LTS v8 and v10 also change?

APIs on AWS are being affected by this issue.

Node.js 8 is unlikely as it will be EOL at the end of the year and we really want to avoid having to do a release there. As for 10 we have updated ICU in an LTS release in the past but we've been asked to not do so in the future specifically not to break AWS deployments: nodejs/Release#483

cc @nodejs/lts @nodejs/intl

@ex1st
Copy link

ex1st commented Nov 7, 2019

Node.js 8 is unlikely as it will be EOL at the end of the year and we really want to avoid having to do a release there. As for 10 we have updated ICU in an LTS release in the past but we've been asked to not do so in the future specifically not to break AWS deployments: nodejs/Release#483

Hello @richardlau and @srl295.
Will be updated npm module full-icu to 65.1 (for v12 at least)? Thank you.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM since @srl295 believes it looks good.

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

Rebuild due to failures in original CI run: https://ci.nodejs.org/job/node-test-pull-request/26498/

@srl295
Copy link
Member

srl295 commented Nov 12, 2019

Travis says:

The job exceeded the maximum time limit for jobs, and has been terminated.

Jenkins says memory exhausted on debian9-docker-armv7

@nodejs/build what's happening here?

@richardlau
Copy link
Member

Travis says:

The job exceeded the maximum time limit for jobs, and has been terminated.

There was a V8 update that landed on master between this PR being opened and the most recent (force) push. The Node.js build takes too long on Travis (which imposes a 50 min time limit on jobs) so we rely on ccache for the builds to complete in time. In this case the cache was stored for the previous version of V8 (at the time the PR was opened) which meant that large numbers of V8 files needed to be recompiled.

I've wiped the cache in Travis for this PR and restarted the build. Without a PR cache Travis defaults to the cache for the target branch (master).

@richardlau
Copy link
Member

Travis says:

The job exceeded the maximum time limit for jobs, and has been terminated.

There was a V8 update that landed on master between this PR being opened and the most recent (force) push. The Node.js build takes too long on Travis (which imposes a 50 min time limit on jobs) so we rely on ccache for the builds to complete in time. In this case the cache was stored for the previous version of V8 (at the time the PR was opened) which meant that large numbers of V8 files needed to be recompiled.

I've wiped the cache in Travis for this PR and restarted the build. Without a PR cache Travis defaults to the cache for the target branch (master).

hmm that didn't work 😞 -- I guess V8 depends on ICU so the ICU update means V8 also needs to be recompiled. I'm not sure what we can do here (AFAICT there's no way to increase Travis' job time limit).

@albertyw
Copy link
Contributor Author

let me rebase to get the most recent node master and see if that fixes things.

@pamydev
Copy link

pamydev commented Nov 13, 2019

any solution guys?

@srl295
Copy link
Member

srl295 commented Nov 13, 2019

When this update arrives, will nodejs LTS v8 and v10 also change?

APIs on AWS are being affected by this issue.

which issue? the time zone? you can always build another node with a different ICU, or an updated time zone.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

richardlau pushed a commit that referenced this pull request Dec 6, 2019
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

Landed in 418dd68.

@richardlau richardlau closed this Dec 6, 2019
@srl295
Copy link
Member

srl295 commented Dec 6, 2019

ICU 66preview announced today - http://site.icu-project.org/download/66 ( it won't ship until March 2020). so, better late than never!

@paulodiovani
Copy link

Landed in 418dd68.

But not in v13.3.0,
any idea in which version it ships?

@srl295
Copy link
Member

srl295 commented Dec 6, 2019 via email

targos pushed a commit that referenced this pull request Dec 9, 2019
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>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
@targos
Copy link
Member

targos commented Jan 13, 2020

Should this be backported to v12.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

albertyw added a commit to albertyw/node-1 that referenced this pull request Jan 21, 2020
Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: nodejs#30211
Fixes: nodejs#29540
Backport-PR-URL: nodejs#31434
PR-URL: nodejs#30356
PR-URL: nodejs#30232
@richardlau
Copy link
Member

Should this be backported to v12.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

A backport PR has been raised (#31433) but given that 13.x/master use full-icu and 12.x small-icu it may be easier to update ICU on 12.x directly following the instructions in https://github.com/nodejs/node/blob/v12.x/tools/icu/README.md rather than attempting to backport this PR.

jameshilliard pushed a commit to jameshilliard/node that referenced this pull request Feb 17, 2020
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>
@srl295 srl295 mentioned this pull request Mar 18, 2020
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>
@codebytere codebytere mentioned this pull request Apr 3, 2020
@albertyw albertyw deleted the update-icu branch January 8, 2022 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICU 65.1