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: backport ICU-20558 to fix Intl crasher #27415

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@srl295
Copy link
Member

commented Apr 25, 2019

Floating patch for ICU 63.x and 64.x
fixing crash in Intl when ICU data not found.

Background:

  • ICU-13778 (landed in ICU 63.1) fixed a bug but
    added a regression.
  • a recent v8 land in Node v12 (which one?) exposes
    this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
(Commit not landed yet in ICU)
Fixes: #27379

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@srl295 srl295 added the intl label Apr 25, 2019

@srl295 srl295 requested review from refack, targos and ryzokuken Apr 25, 2019

@srl295 srl295 self-assigned this Apr 25, 2019

@nodejs-github-bot

This comment has been minimized.

@srl295

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

fyi @jefgen

@srl295

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Notes:

@targos

targos approved these changes Apr 25, 2019

Copy link
Member

left a comment

RSLGTM

@refack

refack approved these changes Apr 25, 2019

Copy link
Member

left a comment

RSLGTM

@srl295 srl295 marked this pull request as ready for review Apr 25, 2019

@refack

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Added a regression test for this. @srl295 obviously feel free to push it out or change it.

@srl295
Copy link
Member Author

left a comment

nice test, thanks @refack !

deps: backport ICU-20558 to fix Intl crasher
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
(Commit not landed yet in ICU)
Fixes: #27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

@srl295 srl295 force-pushed the srl295:patch-ICU-20558 branch from 5400049 to c369ef0 Apr 25, 2019

@srl295

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Rebased and squashed.

@nodejs-github-bot

This comment has been minimized.

@srl295

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@refack do you want me to squash again?

@refack refack force-pushed the srl295:patch-ICU-20558 branch from 5912d0c to d93152d Apr 25, 2019

@refack

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@refack do you want me to squash again?

No rush, we can squash while landing (I could also force push myself). I'm waiting for CI job 22720 to finish so we could start a resume job for the failing platforms.

@nodejs-github-bot

This comment has been minimized.

@srl295

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@refack refack assigned refack and unassigned srl295 Apr 25, 2019

@refack

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@refack want to take this over ?

Yeah, I'm on it.

@nodejs-github-bot

This comment has been minimized.

@refack refack added the fast-track label Apr 25, 2019

@refack

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@here now that #27361 landed, we probably should fast-track this since it solves #27379 which is kind of harsh.

Please 👍 if you concur.

@ryzokuken
Copy link
Member

left a comment

RSLGTM

@targos

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Landed in 617c55e

@targos targos closed this Apr 27, 2019

targos added a commit to targos/node that referenced this pull request Apr 27, 2019

deps: backport ICU-20558 to fix Intl crasher
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: nodejs#27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: nodejs#27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

targos added a commit that referenced this pull request Apr 27, 2019

deps: backport ICU-20558 to fix Intl crasher
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: #27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: #27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

targos added a commit that referenced this pull request Apr 27, 2019

2019-04-29, Version 12.1.0 (Current)
Notable changes:

* intl:
  * Update ICU to 64.2. This adds support for Japanese Era (Reiwa).
    #27361
  * Fixes a bug in ICU that affected Node.js 12.0.0 in the case where
    `new Date().toLocaleString()` was called with a non-default locale.
    #27415
* C++ API:
  * Added an `Environment` overload of `EmitAsyncDestroy`.
    #27255

PR-URL: TODO

@targos targos referenced this pull request Apr 27, 2019

Merged

v12.1.0 proposal #27440

targos added a commit that referenced this pull request Apr 29, 2019

2019-04-29, Version 12.1.0 (Current)
Notable changes:

* intl:
  * Update ICU to 64.2. This adds support for Japanese Era (Reiwa).
    #27361
  * Fixes a bug in ICU that affected Node.js 12.0.0 in the case where
    `new Date().toLocaleString()` was called with a non-default locale.
    #27415
* C++ API:
  * Added an overload of `EmitAsyncDestroy` that can be used during
    garbage collection.
    #27255

PR-URL: #27440

targos added a commit that referenced this pull request Apr 29, 2019

2019-04-29, Version 12.1.0 (Current)
Notable changes:

* intl:
  * Update ICU to 64.2. This adds support for Japanese Era (Reiwa).
    #27361
  * Fixes a bug in ICU that affected Node.js 12.0.0 in the case where
    `new Date().toLocaleString()` was called with a non-default locale.
    #27415
* C++ API:
  * Added an overload of `EmitAsyncDestroy` that can be used during
    garbage collection.
    #27255

PR-URL: #27440

@targos targos referenced this pull request May 1, 2019

Open

v10.16.0 proposal #27514

BethGriggs added a commit that referenced this pull request May 10, 2019

deps: backport ICU-20558 to fix Intl crasher
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: #27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: #27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

BethGriggs added a commit that referenced this pull request May 10, 2019

deps: backport ICU-20558 to fix Intl crasher
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: #27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: #27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

MylesBorins added a commit that referenced this pull request May 16, 2019

deps: backport ICU-20558 to fix Intl crasher
- Floating patch for ICU 63.x and 64.x
- fixing crash in Intl when ICU data not found.
- Regression test from refack included.

Background:
- ICU-13778 (landed in ICU 63.1) fixed a bug but
added a regression.
- a recent v8 land in Node v12 (which one?) exposes
this bug to cause a crash when ICU data is not found.

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-20558
Backport of: unicode-org/icu#632
Fixes: #27379
Co-authored-by: Refael Ackermann <refack@gmail.com>

PR-URL: #27415
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.