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

[v14.x] deps: update ICU to 69.1 #38497

Closed
wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented May 1, 2021

  • deps: V8: cherry-pick 035c305ce776
  • deps: update ICU to 69.1

This is a clean cherry-pick of the 69.1 upgrade but it requires a V8 change to work.

@github-actions github-actions bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v14.x labels May 1, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 1, 2021

@targos
Copy link
Member Author

targos commented May 1, 2021

The linter issue is also present on v14.x-staging and comes from my last push. I don't understand why make lint-md doesn't catch it locally.

@targos
Copy link
Member Author

targos commented May 1, 2021

I removed #37599 for the linter issue.

Now investigating the warnings that make test-linux fail.
Edit: I don't know... The lines are here since before v14.0.0 and I don't see what change would have fixed the warnings on master...

@richardlau
Copy link
Member

FWIW I'm getting similar failures with --error-on-warn on master (c975dff) with gcc/g++ 10. I don't know why we're not seeing these on master in the GitHub actions workflow.

../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::Fill(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_buffer.cc:646:35: error: ‘start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  646 |   char* ptr = ts_obj_data + start + str_length;
      |               ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::ASCII]’:
../src/node_buffer.cc:675:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  675 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::BASE64]’:
../src/node_buffer.cc:675:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  675 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::BASE64URL]’:
../src/node_buffer.cc:675:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  675 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc:672:10: error: ‘max_length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  672 |   size_t max_length;
      |          ^~~~~~~~~~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::BINARY]’:
../src/node_buffer.cc:675:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  675 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc:672:10: error: ‘max_length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  672 |   size_t max_length;
      |          ^~~~~~~~~~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::HEX]’:
../src/node_buffer.cc:675:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  675 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::UCS2]’:
../src/node_buffer.cc:675:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  675 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc:672:10: error: ‘max_length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  672 |   size_t max_length;
      |          ^~~~~~~~~~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::UTF8]’:
../src/node_buffer.cc:675:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  675 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc:672:10: error: ‘max_length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  672 |   size_t max_length;
      |          ^~~~~~~~~~
cc1plus: all warnings being treated as errors
make[1]: *** [libnode.target.mk:386: /var/home/rlau/sandbox/github/node/out/Release/obj.target/libnode/src/node_buffer.o] Error 1
make[1]: *** Waiting for unfinished jobs....
rm df07dd532335b97f9f86c77faf538e51e4326a8c.intermediate db010917bf4cd007e4a7e949a2f971606bec8c9a.intermediate bd9e0eec093d1a111d61ff3c9ed6a3abc0f019a2.intermediate
make: *** [Makefile:105: node] Error 2

real	7m21.887s
user	15m35.802s
sys	2m53.333s
⬢[rlau@toolbox node]$ git rev-parse HEAD
c975dff3c0f0f1ecb1574f3b10dd1d135a7704db
⬢[rlau@toolbox node]$ gcc --version
gcc (GCC) 10.2.1 20201125 (Red Hat 10.2.1-9)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

⬢[rlau@toolbox node]$ g++ --version
g++ (GCC) 10.2.1 20201125 (Red Hat 10.2.1-9)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

⬢[rlau@toolbox node]$ 

@targos
Copy link
Member Author

targos commented May 1, 2021

What can we do?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented May 2, 2021

Our CI is green. Can I have some reviews on this? I'll open a separate issue for the test-linux error.

@targos targos mentioned this pull request May 5, 2021
Original commit message:

    [Intl] call new ListFormatter::createInstance

    The one we currently using is now marked as internal and to be removed
    for 68. Migrating to the style which already avaiable in ICU 67-1.

    Bug: v8:11031
    Change-Id: I668382a2e1b8602ddca02bf231c5008a6c92bf2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477751
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70638}

Refs: v8/v8@035c305
Refs: https://github.com/unicode-org/icu/releases/tag/release-69-1

PR-URL: nodejs#38178
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 17, 2021

@targos
Copy link
Member Author

targos commented May 18, 2021

Landed in c260d98...55d0cd8

@targos targos closed this May 18, 2021
@targos targos deleted the icu-69-v14.x branch May 18, 2021 06:34
targos added a commit that referenced this pull request May 18, 2021
Original commit message:

    [Intl] call new ListFormatter::createInstance

    The one we currently using is now marked as internal and to be removed
    for 68. Migrating to the style which already avaiable in ICU 67-1.

    Bug: v8:11031
    Change-Id: I668382a2e1b8602ddca02bf231c5008a6c92bf2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477751
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70638}

Refs: v8/v8@035c305

PR-URL: #38497
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos added a commit that referenced this pull request Jun 11, 2021
Original commit message:

    [Intl] call new ListFormatter::createInstance

    The one we currently using is now marked as internal and to be removed
    for 68. Migrating to the style which already avaiable in ICU 67-1.

    Bug: v8:11031
    Change-Id: I668382a2e1b8602ddca02bf231c5008a6c92bf2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477751
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70638}

Refs: v8/v8@035c305

PR-URL: #38497
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
jameshilliard pushed a commit to jameshilliard/node that referenced this pull request Jul 19, 2021
Original commit message:

    [Intl] call new ListFormatter::createInstance

    The one we currently using is now marked as internal and to be removed
    for 68. Migrating to the style which already avaiable in ICU 67-1.

    Bug: v8:11031
    Change-Id: I668382a2e1b8602ddca02bf231c5008a6c92bf2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477751
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70638}

Refs: v8/v8@035c305

PR-URL: nodejs#38497
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
richardlau pushed a commit to jameshilliard/node that referenced this pull request Jul 23, 2021
Original commit message:

    [Intl] call new ListFormatter::createInstance

    The one we currently using is now marked as internal and to be removed
    for 68. Migrating to the style which already avaiable in ICU 67-1.

    Bug: v8:11031
    Change-Id: I668382a2e1b8602ddca02bf231c5008a6c92bf2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477751
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70638}

Refs: v8/v8@035c305

PR-URL: nodejs#38497
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 23, 2021
Original commit message:

    [Intl] call new ListFormatter::createInstance

    The one we currently using is now marked as internal and to be removed
    for 68. Migrating to the style which already avaiable in ICU 67-1.

    Bug: v8:11031
    Change-Id: I668382a2e1b8602ddca02bf231c5008a6c92bf2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477751
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70638}

Refs: v8/v8@035c305

PR-URL: #38497
Backport-PR-URL: #39451
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@richardlau richardlau mentioned this pull request Jul 23, 2021
guybedford pushed a commit that referenced this pull request Jul 26, 2021
Original commit message:

    [Intl] call new ListFormatter::createInstance

    The one we currently using is now marked as internal and to be removed
    for 68. Migrating to the style which already avaiable in ICU 67-1.

    Bug: v8:11031
    Change-Id: I668382a2e1b8602ddca02bf231c5008a6c92bf2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477751
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70638}

Refs: v8/v8@035c305

PR-URL: #38497
Backport-PR-URL: #39451
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 26, 2021
Original commit message:

    [Intl] call new ListFormatter::createInstance

    The one we currently using is now marked as internal and to be removed
    for 68. Migrating to the style which already avaiable in ICU 67-1.

    Bug: v8:11031
    Change-Id: I668382a2e1b8602ddca02bf231c5008a6c92bf2d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477751
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70638}

Refs: v8/v8@035c305

PR-URL: #38497
Backport-PR-URL: #39451
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants