Check (small) ICU into repo by srl295 · Pull Request #6088 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

Description of change

configure updated to be in small-icu mode by default.
repo impact:

21M deps/icu-small/ (958 files)

20M deps/icu-small/ (892 files)

Note: 3 commits.

one is ginormous and is the actual ICU source.
the second is tooling and docs and configure to make everything work, including tooling to rebuild this ICU.
the third is just a bump to the URL and hash for ICU 57 (from deps: Intl: ICU 57 bump #6058)

ci: https://ci.nodejs.org/job/node-test-pull-request/2184/
citgm: https://ci.nodejs.org/job/thealphanerd-smoker/185/

(citgm fetches the icu!)

cbc52188a171f3fc1b7bd48bb1d6b51af1e14d7a has a possible windows fix.

Another go at ci: https://ci.nodejs.org/job/node-test-pull-request/2187/

fix breakage on windows

By the way, we should probably let people know that --with-intl=none would be good to add to their builds (especially on small devices) to preserve the current behavior. They can do this now, that option has been there since v0.12

@srl295 Could we introduce --without-intl ? The double negation with none seems unnecessary.

@jbergstroem seems reasonable, I'll do it.

add --without-intl / vcbuild.bat without-intl

@thealphanerd the linux breakage looks like… the wrong bits got pushed. hrm

this looks like the offending stuff to me

../deps/icu-small/source/common/cstr.cpp:18:68: error: void value not ignored as it ought to be
     int32_t length = in.extract(0, in.length(), NULL, (uint32_t)0);
                                                                    ^
../deps/icu-small/source/common/cstr.cpp:22:55: error: no matching function for call to ‘icu_57::UnicodeString::extract(int, int32_t, char*&, int32_t&) const’
         in.extract(0, in.length(), buf, resultCapacity);
                                                       ^
../deps/icu-small/source/common/cstr.cpp:22:55: note: candidates are:
In file included from ../deps/icu-small/source/common/cstr.cpp:9:0:
../deps/icu-small/source/common/unicode/unistr.h:4368:1: note: void icu_57::UnicodeString::extract(int32_t, int32_t, UChar*, int32_t) const
 UnicodeString::extract(int32_t start,
 ^
../deps/icu-small/source/common/unicode/unistr.h:4368:1: note:   no known conversion for argument 3 from ‘char*’ to ‘UChar* {aka short unsigned int*}’
In file included from ../deps/icu-small/source/common/cstr.cpp:9:0:
../deps/icu-small/source/common/unicode/unistr.h:1485:3: note: int32_t icu_57::UnicodeString::extract(UChar*, int32_t, UErrorCode&) const
   extract(UChar *dest, int32_t destCapacity,
   ^
../deps/icu-small/source/common/unicode/unistr.h:1485:3: note:   candidate expects 3 arguments, 4 provided
In file included from ../deps/icu-small/source/common/cstr.cpp:9:0:
../deps/icu-small/source/common/unicode/unistr.h:4375:1: note: void icu_57::UnicodeString::extract(int32_t, int32_t, icu_57::UnicodeString&) const
 UnicodeString::extract(int32_t start,
 ^
../deps/icu-small/source/common/unicode/unistr.h:4375:1: note:   candidate expects 3 arguments, 4 provided
In file included from ../deps/icu-small/source/common/cstr.cpp:9:0:
../deps/icu-small/source/common/unicode/unistr.h:1552:11: note: int32_t icu_57::UnicodeString::extract(int32_t, int32_t, char*, int32_t, icu_57::UnicodeString::EInvariant) const
   int32_t extract(int32_t start,
           ^
../deps/icu-small/source/common/unicode/unistr.h:1552:11: note:   candidate expects 5 arguments, 4 provided
make[2]: *** [/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-64/out/Release/obj.target/icuucx/deps/icu-small/source/common/cstr.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-64/out'
make[1]: *** [node] Error 2
make[1]: Leaving directory `/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-64'
make: *** [run-ci] Error 2
Build step 'Execute shell' marked build as failure
Publish TAP Results is waiting for a checkpoint on node-test-commit-linux » centos5-64 #2881
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: test.tap
Did not find any matching files. Setting build result to FAILURE.
Build step 'Publish TAP Results' marked build as failure
Notifying upstream projects of job completion
Finished: FAILURE

Not entirely sure what has changed vs the download. Did you do anything more than just add the icu folder to deps?

@thealphanerd yes- hold on a bit, need to sort out how i mis-rebased… update looks mostly sorted…

Notes:

this will need to rebase once deps: Intl: ICU 57 bump #6064 lands
I'm keeping this as 2 commits and would like to do when(if) it lands: so that the big commit that's all ICU code is separate from the more interesting node code.

@thealphanerd okay - give it a try. 5f56785e7ef9c0fb940e14312fd9da6dbfb71761 looks good for me on mac|win|lnx

work in progress — trying to make it a little bit smaller

OK- let's see how this one does.

added a generated deps/icu-small/README-SMALL-ICU.txt

ci: https://ci.nodejs.org/job/node-test-pull-request/2230/ 🎉

@thealphanerd I see failures above— but none of them look relevant to this ticket specifically.

armv8 - "slave went offline during build" - but not during compilation of any files I changed. System problem?
freebsd - g++ : no such command - looks like configure or gyp got confused here. Why is it using g++ just for ICU files? --> Split off to FreeBSD with small-icu fails: g++ not found #6152
pi1p cross compile looks bad, but again not broken by this commit…

The last two look bad, but one question is if configure --with-intl=small-icu --download=all && make ever worked on these two platforms? I assumed so because that's how binaries on nodejs.org are built…

Probably these latter two issues should be two separate issues which need to be fixed before merge, otherwise these platforms will be broken.

I split out the freebsd one. I'd split the cross compile one, but I'm not sure how to reproduce/test locally.

Edit: I don't know what I'm talking about. Sorry.

Note to @nodejs/build: Once this lands we want to pass --download-path=$home/node-icu like we do on release targets in our jenkins jobs. This will unfortunately also be one of those moments where we can't pass it for all jobs, but likely need to pass it through env so it works for all versions we test against.

Why does the download path need to be set due to this change? No download necessary with default args.

…-icu * Change configure default to "small-icu" (Intl on, English only) * add "--without-intl" and "vcbuild without-intl" options, equivalent to --with-intl=none * update BUILDING.md with above changes * Checks in tools that generate the deps/icu-small source directory from ICU source * Tools and process for updating ICU documented in tools/icu/README.md nodejs#3476

* bump to ICU 57.1 - update URL / hash Fixes: nodejs#6058

rebased… ci build in progress

CI green https://ci.nodejs.org/job/node-test-pull-request/2488/

Can I get some LGTMs?

Rubber stamp LGTM!

* this commit has "small" ICU 57.1. See other related commit for tools to generate this commit. Fixes: #3476 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

…-icu * Change configure default to "small-icu" (Intl on, English only) * add "--without-intl" and "vcbuild without-intl" options, equivalent to --with-intl=none * update BUILDING.md with above changes * Checks in tools that generate the deps/icu-small source directory from ICU source * Tools and process for updating ICU documented in tools/icu/README.md Fixes: #3476 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

* bump to ICU 57.1 - update URL / hash Fixes: #6058 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

landed in

3ff54bd (ICU bump to 57.1)
03a8637 (tools)
2bbd1cd (check in ICU)

Awesome! Very happy to see this land.

* bump to ICU 57.1 - update URL / hash Fixes: #6058 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

…-icu * Change configure default to "small-icu" (Intl on, English only) * add "--without-intl" and "vcbuild without-intl" options, equivalent to --with-intl=none * update BUILDING.md with above changes * Checks in tools that generate the deps/icu-small source directory from ICU source * Tools and process for updating ICU documented in tools/icu/README.md Fixes: #3476 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

* this commit has "small" ICU 57.1. See other related commit for tools to generate this commit. Fixes: #3476 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna Henningsen) [#6511](#6511) - **child_process**: use /system/bin/sh on android (Ben Noordhuis) [#6745](#6745) - **deps**: - upgrade npm to 3.8.9 (Rebecca Turner) [#6664](#6664) - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh) [#6572](#6572) - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé) [#6796](#6796) - Intl: ICU 57 bump (Steven R. Loomis) [#6088](#6088) - **repl**: - copying tabs shouldn't trigger completion (Eugene Obrezkov) [#5958](#5958) - exports `Recoverable` (Blake Embrey) [#3488](#3488) - **src**: add O_NOATIME constant (Rich Trott) [#6492](#6492) - **src,module**: add --preserve-symlinks command line flag (James M Snell) [#6537](#6537) - **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen) [#6683](#6683) As of this release the 6.X line now includes 64-bit binaries for Linux on Power Systems running in big endian mode in addition to the existing 64-bit binaries for running in little endian mode. PR-URL: #6810

marking as dont-land for LTS

@srl295 let me know if you feel otherwise about this change

As title implies, "check Icu into repo" mostly affects building so not important for LTS imo.

Could separately land the 57 bump.. Or just plain build LTS with later icu (no code change).

srl295 force-pushed the withicu57 branch from 4e1a523 to cbc5218 Compare April 6, 2016 23:22

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Apr 6, 2016

srl295 self-assigned this Apr 7, 2016

MylesBorins mentioned this pull request Apr 7, 2016

deps: Intl: ICU 57 bump #6064

Closed

4 tasks

srl295 force-pushed the withicu57 branch 2 times, most recently from e57b111 to 5f56785 Compare April 8, 2016 00:52

Fishrock123 added the ctc-agenda label Apr 8, 2016

srl295 force-pushed the withicu57 branch from 5f56785 to c930b37 Compare April 9, 2016 00:35

srl295 force-pushed the withicu57 branch from c930b37 to 103537c Compare April 9, 2016 01:56

srl295 force-pushed the withicu57 branch from 103537c to b482e56 Compare April 9, 2016 02:03

srl295 mentioned this pull request Apr 11, 2016

FreeBSD with small-icu fails: g++ not found #6152

Closed

jbergstroem mentioned this pull request Apr 13, 2016

gyp: Inherit CC/CXX for CC/CXX.host #6173

Merged

2 tasks

srl295 added 2 commits May 3, 2016 14:45

deps: Intl: ICU 57 bump

230ba93

* bump to ICU 57.1 - update URL / hash Fixes: nodejs#6058

srl295 force-pushed the withicu57 branch from b482e56 to 230ba93 Compare May 3, 2016 21:57

srl295 added a commit that referenced this pull request May 4, 2016

deps: Intl: Check in "small-icu" 57.1

2bbd1cd

* this commit has "small" ICU 57.1. See other related commit for tools to generate this commit. Fixes: #3476 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

srl295 added a commit that referenced this pull request May 4, 2016

deps: Intl: ICU 57 bump

3ff54bd

* bump to ICU 57.1 - update URL / hash Fixes: #6058 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

srl295 closed this May 4, 2016

evanlucas pushed a commit that referenced this pull request May 17, 2016

deps: Intl: ICU 57 bump

ee1e5a2

* bump to ICU 57.1 - update URL / hash Fixes: #6058 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas pushed a commit that referenced this pull request May 17, 2016

deps: Intl: Check in "small-icu" 57.1

7a6d2ad

* this commit has "small" ICU 57.1. See other related commit for tools to generate this commit. Fixes: #3476 PR-URL: #6088 Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas mentioned this pull request May 17, 2016

2016-05-17, Version 6.2.0 (Stable) #6810

Merged

MylesBorins added the dont-land-on-v4.x label Jun 2, 2016

thefourtheye mentioned this pull request Jul 12, 2016

tools: remove unnecessary imports and assignments #7483

Closed

2 tasks

richardlau mentioned this pull request May 23, 2018

Create an ansible task for downloading icu nodejs/build#268

Closed

Check (small) ICU into repo by srl295 · Pull Request #6088 · nodejs/node · GitHub

Uh oh!

Uh oh!

Check (small) ICU into repo - #6088

Check (small) ICU into repo#6088srl295 wants to merge 3 commits intonodejs:masternodejs/node:masterfrom srl295:withicu57srl295/node:withicu57Copy head branch name to clipboard

Conversation

srl295 commented Apr 6, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

MylesBorins commented Apr 6, 2016

Uh oh!

srl295 commented Apr 6, 2016

Uh oh!

MylesBorins commented Apr 7, 2016

Uh oh!

srl295 commented Apr 7, 2016

Uh oh!

jbergstroem commented Apr 7, 2016

Uh oh!

srl295 commented Apr 7, 2016

Uh oh!

srl295 commented Apr 7, 2016

Uh oh!

MylesBorins commented Apr 7, 2016

Uh oh!

srl295 commented Apr 7, 2016

Uh oh!

srl295 commented Apr 8, 2016

Uh oh!

srl295 commented Apr 8, 2016

Uh oh!

srl295 commented Apr 9, 2016

Uh oh!

srl295 commented Apr 9, 2016

Uh oh!

MylesBorins commented Apr 9, 2016

Uh oh!

srl295 commented Apr 11, 2016

Uh oh!

srl295 commented Apr 11, 2016

Uh oh!

jbergstroem commented Apr 13, 2016

Uh oh!

srl295 commented Apr 13, 2016

Uh oh!

srl295 commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 commented May 3, 2016

Uh oh!

srl295 commented May 4, 2016

Uh oh!

jasnell commented May 4, 2016

Uh oh!

srl295 commented May 4, 2016

Uh oh!

jasnell commented May 4, 2016

Uh oh!

MylesBorins commented Jun 2, 2016

Uh oh!

srl295 commented Jun 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Check (small) ICU into repo#6088
srl295 wants to merge 3 commits into
nodejs:masterfrom
srl295:withicu57

srl295 commented May 3, 2016 •
edited
Loading