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

Failling to build LTS versions with ICU 59 #13022

Closed
ArchangeGabriel opened this issue May 14, 2017 · 15 comments
Closed

Failling to build LTS versions with ICU 59 #13022

ArchangeGabriel opened this issue May 14, 2017 · 15 comments
Labels
build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation.

Comments

@ArchangeGabriel
Copy link

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.

@addaleax addaleax added build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. labels May 14, 2017
@addaleax
Copy link
Member

/cc @srl295

@refack
Copy link
Contributor

refack commented May 14, 2017

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

@ArchangeGabriel
Copy link
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.

@bnoordhuis
Copy link
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.

@ArchangeGabriel
Copy link
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.

@ArchangeGabriel
Copy link
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?

@bnoordhuis
Copy link
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.

@ArchangeGabriel
Copy link
Author

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

@bnoordhuis
Copy link
Member

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

@ArchangeGabriel
Copy link
Author

OK, thanks.

@ArchangeGabriel
Copy link
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.

MylesBorins pushed a commit that referenced this issue May 30, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this issue May 30, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this issue Jun 6, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 11, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: nodejs/node#13022
PR-URL: nodejs/node#13040
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@Qantas94Heavy
Copy link
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

@srl295
Copy link
Member

srl295 commented Sep 13, 2017

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.)

@srl295
Copy link
Member

srl295 commented Sep 13, 2017

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.

@srl295
Copy link
Member

srl295 commented Sep 13, 2017

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

MylesBorins pushed a commit that referenced this issue Oct 25, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
gibfahn pushed a commit to ibmruntimes/node that referenced this issue Nov 24, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: nodejs/node#13022
PR-URL: nodejs/node#13040
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
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. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

No branches or pull requests

6 participants