Check (small) ICU into repo

[srl295]

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)

[MylesBorins]

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

(citgm fetches the icu!)

[srl295]

cbc52188a171f3fc1b7bd48bb1d6b51af1e14d7a has a possible windows fix.

[MylesBorins]

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

[srl295]

• 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

[jbergstroem]

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

[srl295]

@jbergstroem seems reasonable, I'll do it.

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

[srl295]

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

[MylesBorins]

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?

[srl295]

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

[srl295]

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.

[srl295]

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

[srl295]

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

[srl295]

OK- let's see how this one does.

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

[MylesBorins]

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

[srl295]

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

[srl295]

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

[jbergstroem]

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.

[srl295]

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

[srl295]

rebased… ci build in progress

[srl295]

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

[srl295]

Can I get some LGTMs?

[jasnell]

Rubber stamp LGTM!

[srl295]

landed in

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

[jasnell]

Awesome! Very happy to see this land.

[MylesBorins]

marking as dont-land for LTS

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

[srl295]

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