Skip to content

Failling to build LTS versions with ICU 59 #13022

Closed
Closed
Failling to build LTS versions with ICU 59#13022
Labels
buildIssues and PRs related to Node.js builds or CI infrastructure.i18n-apiIssues and PRs related to Node.js internationalization support.

Description

@ArchangeGabriel
ArchangeGabriel
opened on May 14, 2017

When trying to build either Argon or Boron with ICU 59.x, it fails with the following error:

../deps/v8/src/runtime/runtime-i18n.cc: In function ‘v8::internal::Object* v8::internal::__RT_impl_Runtime_StringNormalize(v8::internal::Arguments, v8::internal::Isolate*)’:
../deps/v8/src/runtime/runtime-i18n.cc:588:8: error: ‘icu::Normalizer’ has not been declared
   icu::Normalizer::normalize(u_value, normalizationForms[form_id], 0, result,
        ^~~~~~~~~~

I’m not sure how to fix this, and if other changes are required, but this is an issue for ArchLinux packages.

Activity

  1. addaleax
    added
    buildIssues and PRs related to Node.js builds or CI infrastructure.
    i18n-apiIssues and PRs related to Node.js internationalization support.
    on May 14, 2017

  2. addaleax commented on May 14, 2017

    @addaleax
    addaleax
    on May 14, 2017
    Member

    /cc @srl295

  3. refack commented on May 14, 2017

    @refack
    refack
    on May 14, 2017
    Contributor

    Ref: bump ICU to 59.1, I'm not sure it was backported to v6 (Boron), and it will probably won't be ported to v4 that has moved from LTS to maintenance.

    Ref 2: Previous PR depends on #11753

  4. ArchangeGabriel commented on May 14, 2017

    @ArchangeGabriel
    ArchangeGabriel
    on May 14, 2017
    Author

    Hum, maybe I wasn’t clear but I’m building with system ICU, not nodejs embedded one, so #12486 seems irrelevant. I’ve tried to use #11753 as patch, but it didn’t worked. The patching has to be different in v6 and v4 than in v7, and that fixing one (v6) would probably fix both.

  5. bnoordhuis commented on May 14, 2017

    @bnoordhuis
    bnoordhuis
    on May 14, 2017
    Member

    That specific build error is probably easy to fix by adding a #include "unicode/normlzr.h" at the top of runtime-i18n.cc.

    In general though, V8 only supports one ICU version at a time, and with the v4.x and v6.x release lines that is ICU 58.

  6. ArchangeGabriel commented on May 14, 2017

    @ArchangeGabriel
    ArchangeGabriel
    on May 14, 2017
    Author

    @bnoordhuis Thanks, this fixed it. Build fine, only 3 tests not passing:

    === release test-intl ===                                                      
Path: parallel/test-intl
/build/nodejs-lts-argon/src/node-v4.8.3/test/parallel/test-intl.js:44
      new Intl.DateTimeFormat(['en'],
      ^

RangeError: Unsupported time zone specified UTC
    at Object.<anonymous> (/build/nodejs-lts-argon/src/node-v4.8.3/test/parallel/test-intl.js:44:7)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at startup (node.js:140:18)
    at node.js:1043:3
Command: out/Release/node /build/nodejs-lts-argon/src/node-v4.8.3/test/parallel/test-intl.js
=== release test-tls-ecdh-disable ===                                          
Path: parallel/test-tls-ecdh-disable
assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: false == true
    at /build/nodejs-lts-argon/src/node-v4.8.3/test/parallel/test-tls-ecdh-disable.js:34:5
    at /build/nodejs-lts-argon/src/node-v4.8.3/test/common.js:407:15
    at ChildProcess.exithandler (child_process.js:207:5)
    at emitTwo (events.js:87:13)
    at ChildProcess.emit (events.js:172:7)
    at maybeClose (internal/child_process.js:862:16)
    at Socket.<anonymous> (internal/child_process.js:338:11)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at Pipe._onclose (net.js:490:12)
Command: out/Release/node /build/nodejs-lts-argon/src/node-v4.8.3/test/parallel/test-tls-ecdh-disable.js
=== release test-tls-set-ciphers ===                                           
Path: parallel/test-tls-set-ciphers
/build/nodejs-lts-argon/src/node-v4.8.3/test/parallel/test-tls-set-ciphers.js:45
    if (err) throw err;
             ^

Error: Command failed: "openssl" s_client -cipher DES-CBC3-SHA -connect 127.0.0.1:44101
Error with command: "-cipher DES-CBC3-SHA"
140587604950912:error:1410D0B9:SSL routines:SSL_CTX_set_cipher_list:no cipher match:ssl/ssl_lib.c:2018:

    at ChildProcess.exithandler (child_process.js:200:12)
    at emitTwo (events.js:87:13)
    at ChildProcess.emit (events.js:172:7)
    at maybeClose (internal/child_process.js:862:16)
    at Socket.<anonymous> (internal/child_process.js:338:11)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at Pipe._onclose (net.js:490:12)
Command: out/Release/node /build/nodejs-lts-argon/src/node-v4.8.3/test/parallel/test-tls-set-ciphers.js

    The DES-CBC3-SHA one is because although nodejs is built against OpenSSL 1.0, the openssl command on my system is OpenSSL 1.1, so this cipher is gone, hence the error. Not sure about the other two tests, but I would expect the test-tls-ecdh-disable to be something similar. No idea about the first one.

  7. ArchangeGabriel commented on May 14, 2017

    @ArchangeGabriel
    ArchangeGabriel
    on May 14, 2017
    Author

    Currently testing on my server, this single line change seems to be enough. Should I expect something to break, and if so how to detect any breakage?

  8. bnoordhuis commented on May 15, 2017

    @bnoordhuis
    bnoordhuis
    on May 15, 2017
    Member

    The parallel/test-intl failure is probably related. As to breakage: it's an untested combination; bugs are possible.

    I think we'd be open to floating a patch to fix the build error, open a pull request if you want to purse that. I'll go ahead and close this out.

  9. bnoordhuis
    closed this as completedon May 15, 2017

  10. ArchangeGabriel commented on May 15, 2017

    @ArchangeGabriel
    ArchangeGabriel
    on May 15, 2017
    Author

    Should I open PR against -staging branches or just v4.x/v6.x ones?

  11. bnoordhuis commented on May 15, 2017

    @bnoordhuis
    bnoordhuis
    on May 15, 2017
    Member

    The -staging ones. The -staging branches contain what is to be released, the non-staging ones what has been released.

  12. ArchangeGabriel commented on May 15, 2017

    @ArchangeGabriel
    ArchangeGabriel
    on May 15, 2017
    Author

    OK, thanks.

  13. ArchangeGabriel commented on May 15, 2017

    @ArchangeGabriel
    ArchangeGabriel
    on May 15, 2017
    Author

    I’ve opened #13040. I’ll open one for v6.x once I’ll have fixed this one enough for it to be accepted.

  14. MylesBorins
    added 2 commits that reference this issue on May 30, 2017

    deps/v8: add missing #include "unicode/normlzr.h"

    d3ae2f0

    deps/v8: add missing #include "unicode/normlzr.h"

    c44619e
  15. MylesBorins
    added a commit that references this issue on Jun 6, 2017

    deps/v8: add missing #include "unicode/normlzr.h"

    d38364b
  16. MylesBorins
    added a commit that references this issue on Jul 11, 2017

    deps/v8: add missing #include "unicode/normlzr.h"

    0b1df7e
  17. andrew749
    added a commit that references this issue on Jul 19, 2017

    deps/v8: add missing #include "unicode/normlzr.h"

    c7b238c

  18. Qantas94Heavy commented on Sep 13, 2017

    @Qantas94Heavy
    Qantas94Heavy
    on Sep 13, 2017
    Contributor

    @ArchangeGabriel I've just tried building 4.x with ICU 59 myself and also saw this failure for parallel/test-intl. It seems to be a bug in V8 (fixed in later versions) -- wonder if it's worth fixing: https://bugs.chromium.org/p/chromium/issues/detail?id=364374

  19. srl295 commented on Sep 13, 2017

    @srl295
    srl295
    on Sep 13, 2017
    Member

    late once again.

    Yes, as @MylesBorins commit says, the #include above is a latent v8 bug, exactly like #11753 . (I think the Normalizer is the deprecated API anyway.)

  20. srl295 commented on Sep 13, 2017

    @srl295
    srl295
    on Sep 13, 2017
    Member

    In general though, V8 only supports one ICU version at a time, and with the v4.x and v6.x release lines that is ICU 58

    Previous ICUs should be supported as well, at this point.

  21. srl295 commented on Sep 13, 2017

    @srl295
    srl295
    on Sep 13, 2017
    Member

    @Qantas94Heavy but, your assessment of the TZ part seems correct.

  22. MylesBorins
    added a commit that references this issue on Oct 25, 2017

    deps/v8: add missing #include "unicode/normlzr.h"

    545235f
  23. gibfahn
    added a commit that references this issue on Nov 24, 2017

    deps/v8: add missing #include "unicode/normlzr.h"

    d34d148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    buildIssues and PRs related to Node.js builds or CI infrastructure.i18n-apiIssues and PRs related to Node.js internationalization support.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions