Skip to content

build: Intl: deps: bump ICU to 56.1 (GA)#3281

Closed
srl295 wants to merge 2 commits intonodejs:masterfrom
srl295:srl295-icu56-2917
Closed

build: Intl: deps: bump ICU to 56.1 (GA)#3281
srl295 wants to merge 2 commits intonodejs:masterfrom
srl295:srl295-icu56-2917

Conversation

@srl295
Copy link
Member

@srl295 srl295 commented Oct 8, 2015

Fixes: #2917

@jasnell if you can land this in master also it would be great otherwise I will do later

@srl295 srl295 self-assigned this Oct 8, 2015
@srl295
Copy link
Member Author

srl295 commented Oct 8, 2015

test running https://ci.nodejs.org/job/node-test-pull-request/454/console
Note: i haven't mirrored the ICU url to our CDN yet. Hopefully server won't melt before I get a round tuit.

@mscdex mscdex added build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. labels Oct 8, 2015
@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

One challenge in the CI build: "ok 469 test-intl.js # skip Intl tests because Intl object not present." ... it doesn't look like our CI is configured with --with-intl enabled (/cc @nodejs/build)

@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

@srl295 ... getting compile erros when attempting to run a build locally on Mac OS X 10.11

clang: error: clangno such file or directory: '../deps/icu/source/tools/genrb/reslist.c': 
error: no such file or directory: '../deps/icu/source/tools/genrb/genrb.c'clang: error
: no input files
clang: error: no input files
  g++ '-D_DARWIN_USE_64_BIT_INODE=1' '-DUCONFIG_NO_TRANSLITERATION=1' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DUCONFIG_NO_LEGACY_CONVERSION=1' '-DUCONFIG_NO_IDNA=1' -I../deps/icu/source/common -I../deps/icu/source/i18n -I../deps/icu/source/io -I../deps/icu/source/tools/toolutil  -Os -gdwarf-2 -mmacosx-version-min=10.5 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++0x -fno-rtti -fno-exceptions -fno-thre:clang: error: no such file or directory: '../deps/icu/source/tools/genrb/wrtjava.c'
clang: error: no input files
make[1]: *** [/Users/james/Node/main/node/out/Release/obj.host/genrb/deps/icu/source/tools/genrb/wrtjava.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: *** [/Users/james/Node/main/node/out/Release/obj.host/genrb/deps/icu/source/tools/genrb/genrb.o] Error 1
make[1]: *** [/Users/james/Node/main/node/out/Release/obj.host/genrb/deps/icu/source/tools/genrb/reslist.o] Error 1
../deps/icu/source/tools/genrb/wrtxml.cpp:397:1: warning: unused function 'print' [-Wunused-function]
print(UChar* src, int32_t srcLen,const char *tagStart,const char *tagEnd,  UErrorCode *status){
^
../deps/icu/source/tools/genrb/wrtxml.cpp:472:13: warning: unused function 'printAttribute' [-Wunused-function]
static void printAttribute(const char *name, const UnicodeString value, int32_t /*len*/)
            ^
../tools/icu/iculslocs.cc:196:15: warning: unused variable 'isTable' [-Wunused-variable]
  const UBool isTable = (UBool)(ures_getType(bund.getAlias()) == URES_TABLE);
              ^
1 warning generated.
2 warnings generated.
make: *** [node] Error 2

@srl295
Copy link
Member Author

srl295 commented Oct 8, 2015

no such file ... genrb.c
@jasnell I think the above is due to one of the generated makefiles not being removed cleanly. In 56 that file is genrb.cpp.

@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

Ok, that's what I was figuring. I've removed the out directory manually and kicked off another build. I'll let you know how it goes. Can you please investigate to see if additional ICU cleanups need to be done by make clean?

I hesitate to land this yet because not everyone is going to build from a clean branch. The cleanup of the old deps/icu, icu zip and make files could trip others up. Ideally, make clean would fully reset and/or configure would catch that deps/icu and the icu zip are out of date and prepare accordingly

@srl295
Copy link
Member Author

srl295 commented Oct 8, 2015

@jasnell okay, the offending file is: node/out/Release/.deps//home/srl/src/node/out/Release/obj.host/genrb/deps/icu/source/tools/genrb/genrb.o.d

Why doesn't make clean delete this? Looks like it only deletes *.o files. I assumed I was just doing it wrong™.

Should make clean deleting the .deps dir? For that matter should configure delete it?

@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

No objection from me. @nodejs/build @rvagg ... any opinions?

@Fishrock123
Copy link
Contributor

Fishrock123 commented Oct 8, 2015

https -- yes please!

@srl295
Copy link
Member Author

srl295 commented Oct 8, 2015

@Fishrock123

https

Thanks for noticing. I had meant to do that before.

@srl295
build: Intl: deps: bump ICU to 56.1 (GA)
b7229bb 
* ICU 56 was just released yesterday. Update to it.
* Notable changes: Unicode 8, CLDR 28, 2-3x number format perf,
  20% improvement in Collator startup
  * more at http://site.icu-project.org/download/56 or in nodejs#2917

Also:

* cleanup out/**/*.d and deps/icu  on "make clean"

Fixes: nodejs#2917
@srl295 srl295 force-pushed the srl295-icu56-2917 branch from 1d2a2b8 to b7229bb Compare October 8, 2015 17:18
@srl295
Copy link
Member Author

srl295 commented Oct 8, 2015

updated pr so make check deletes out/…/*.d and also deps/icu … (a notice is printed if deps/icu was removed)

@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

Ok, updated PR LGTM on OS X. Will spin up a windows test run shortly

@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

On windows, vcbuild.bat clean needs to be updated to remove the deps/icu directory. Other than that, build looks good. Once the PR is updated with the vcbuild changes, I'll do one more test pass then get it landed.

@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

LGTM! @mhdawson ... I'll be getting this landed in master... do you want to do some testing before I pull it into v4.x?

srl295 added a commit that referenced this pull request Oct 8, 2015
@srl295 @jasnell
build: Intl: deps: bump ICU to 56.1 (GA)
01908d0 
* ICU 56 was just released yesterday. Update to it.
* Notable changes: Unicode 8, CLDR 28, 2-3x number format perf,
  20% improvement in Collator startup
  * more at http://site.icu-project.org/download/56 or in #2917

Also:

* cleanup out/**/*.d and deps/icu  on "make clean"
* cleanup deps/icu on "vcbuild clean"

When building from an non-clean directory, it's important to
run `make clean` or `vcbuild clean` to remove the existing
ICU 55 from the deps path before building.

Fixes: #2917
PR-URL: #3281
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

Landed in 01908d0. Still need to land on v4.x

@srl295 srl295 closed this Oct 8, 2015
@srl295 srl295 deleted the srl295-icu56-2917 branch October 8, 2015 22:02
srl295 added a commit that referenced this pull request Oct 9, 2015
@srl295 @jasnell
build: Intl: deps: bump ICU to 56.1 (GA)
af24376 
* ICU 56 was just released yesterday. Update to it.
* Notable changes: Unicode 8, CLDR 28, 2-3x number format perf,
  20% improvement in Collator startup
  * more at http://site.icu-project.org/download/56 or in #2917

Also:

* cleanup out/**/*.d and deps/icu  on "make clean"
* cleanup deps/icu on "vcbuild clean"

When building from an non-clean directory, it's important to
run `make clean` or `vcbuild clean` to remove the existing
ICU 55 from the deps path before building.

Fixes: #2917
PR-URL: #3281
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 9, 2015

Landed in v4.x with af24376

@jasnell jasnell mentioned this pull request Oct 9, 2015
Closed
jasnell added a commit that referenced this pull request Oct 12, 2015
@jasnell
2015-10-08, Version 4.2.0 'Argon' (LTS) Release
08c0aed 
The first Node.js LTS release! See https://github.com/nodejs/LTS/
for details of the LTS process.

* **icu**: Updated to version 56 with significant performance improvements
  (Steven R. Loomis) #3281
* **node**:
  - Added new `-c` (or `--check`) command line argument for checking script
    syntax without executing the code (Dave Eddy) #2411
  - Added `process.versions.icu` to hold the current ICU library version
    (Evan Lucas) #3102
  - Added `process.release.lts` to hold the current LTS codename when the
    binary is from an active LTS release line (Rod Vagg) #3212
* **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes:
  https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299

PR-URL: #3258
jasnell added a commit that referenced this pull request Oct 12, 2015
@jasnell
2015-10-08, Version 4.2.0 'Argon' (LTS) Release
9fd6904 
The first Node.js LTS release! See https://github.com/nodejs/LTS/
for details of the LTS process.

* **icu**: Updated to version 56 with significant performance improvements
  (Steven R. Loomis) #3281
* **node**:
  - Added new `-c` (or `--check`) command line argument for checking script
    syntax without executing the code (Dave Eddy) #2411
  - Added `process.versions.icu` to hold the current ICU library version
    (Evan Lucas) #3102
  - Added `process.release.lts` to hold the current LTS codename when the
    binary is from an active LTS release line (Rod Vagg) #3212
* **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes:
  https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299

PR-URL: #3258
jasnell added a commit that referenced this pull request Oct 12, 2015
@jasnell
2015-10-12, Version 4.2.0 'Argon' (LTS) Release
6dc12b1 
The first Node.js LTS release! See https://github.com/nodejs/LTS/
for details of the LTS process.

* **icu**: Updated to version 56 with significant performance improvements
  (Steven R. Loomis) #3281
* **node**:
  - Added new `-c` (or `--check`) command line argument for checking script
    syntax without executing the code (Dave Eddy) #2411
  - Added `process.versions.icu` to hold the current ICU library version
    (Evan Lucas) #3102
  - Added `process.release.lts` to hold the current LTS codename when the
    binary is from an active LTS release line (Rod Vagg) #3212
* **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes:
  https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299

PR-URL: #3258
jasnell added a commit that referenced this pull request Oct 12, 2015
@jasnell
2015-10-12, Version 4.2.0 'Argon' (LTS) Release
47db247 
The first Node.js LTS release! See https://github.com/nodejs/LTS/
for details of the LTS process.

* **icu**: Updated to version 56 with significant performance improvements
  (Steven R. Loomis) #3281
* **node**:
  - Added new `-c` (or `--check`) command line argument for checking script
    syntax without executing the code (Dave Eddy) #2411
  - Added `process.versions.icu` to hold the current ICU library version
    (Evan Lucas) #3102
  - Added `process.release.lts` to hold the current LTS codename when the
    binary is from an active LTS release line (Rod Vagg) #3212
* **npm**: Upgraded to npm 2.14.7 from 2.14.4, see release notes:
  https://github.com/npm/npm/releases/tag/v2.14.7 for full details (Kat Marchán) #3299

PR-URL: #3258
@srl295 srl295 mentioned this pull request Feb 15, 2016
Closed
@srl295 srl295 mentioned this pull request Nov 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@srl295 srl295

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@srl295 @jasnell @Fishrock123 @mscdex @MylesBorins