WIP: deps: checkin a full ICU #21676

srl295 · 2018-07-05T22:55:06Z

🚧

update tools to simply delete unneeded files instead of shrinking ICU data to English only
Fixes: Building with full-icu by default #19214

To be based on #21452

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-07-05T22:55:08Z

@srl295 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/212/pipeline

srl295 · 2018-07-05T22:59:58Z

Main discussion: #19214

srl295 · 2018-07-05T23:02:46Z

linter works locally for me

Edit: missed the "markdown linter not installed" thing..

srl295 · 2018-07-05T23:14:30Z

I renamed the icu directory from deps/icu-small to deps/icu-full to keep from there being any confusion about what is checked in. I guess it would reduce churn a bit to keep the same name and just use deps/icu-small for the full ICU data.

devsnek

rubber stamp lgtm

richardlau

I renamed the icu directory from deps/icu-small to deps/icu-full to keep from there being any confusion about what is checked in. I guess it would reduce churn a bit to keep the same name and just use deps/icu-small for the full ICU data.

+1 for renaming, although you will also need to update tools/license-builder.sh and doc/api/intl.md#options-for-building-nodejs

TimothyGu

How about we just use the name deps/icu? The full part won't age well, and the checkout is not technically full since unnecessary files are removed.

TimothyGu · 2018-07-06T00:27:05Z

tools/icu/README.md


-You are ready to check in the updated `deps/small-icu`. This is a big commit,
+You are ready to check in the updated `deps/full-icu`. This is a big commit,


icu-full or just icu, as I recommended.

if you do configure --with-icu-source=somewhere/ssomewhere/icu4c-62_1-src.tgz then deps/icu gets created with the contents of the specified tarball. so deps/icu is already taken, and expected to be transient.

We could also just call it small-icu - it is smaller (trimmed out unneeded stuff), just not small in data.

Ah, in that case I'd rather rename the transient directory if that's not too much work.

^ok, i misread this , and i implemented renaming the modified source to dep/icu-small (even though it has all locales, just smaller source)

will fix this up

TimothyGu · 2018-07-06T01:35:18Z

doc/api/intl.md

@@ -38,26 +38,25 @@ in [BUILDING.md][].



Earlier in this file there's a section:

Node.js (and its underlying V8 engine) uses ICU to implement these features in native C/C++ code. However, some of them require a very large ICU data file in order to support all locales of the world. Because it is expected that most Node.js users will make use of only a small portion of ICU functionality, only a subset of the full ICU data set is provided by Node.js by default. Several options are provided for customizing and expanding the ICU data set either when building or running Node.js.

The underlined portion should probably be edited.

Basically, you could still build ICU with English only, but you'd have to do your own ICU build to get it. Such tools should live in ICU-land, not in Node.

silverwind · 2018-07-06T15:46:31Z

How about we just use the name deps/icu

That'd be my preference too, if it's not too much of a hassle.

Which semver tag should we give this PR? I'm leaning towards semver-minor as full-icu is supposed to be a superset of small-icu, but it could also be seen as breaking as we remove a build option.

silverwind · 2018-07-06T15:49:36Z

deps/icu-small/source/common/brkeng.cpp

@@ -78,7 +78,7 @@ UnhandledEngine::findBreaks( UText *text,
                             int32_t /* startPos */,
                             int32_t endPos,
                             UVector32 &/*foundBreaks*/ ) const {
-    UChar32 c = utext_current32(text);
+    UChar32 c = utext_current32(text); 


Kind of odd that this ICU bump contains quite a lot of trailing whitespace changes.

happens every.single.time. I usually locally re-land with --fix-whitespace=all and then push the landed version back to the branch, but i messed something up in the process this time.

upstream ICU still has trailing whitespace.

silverwind · 2018-07-06T15:53:55Z

doc/api/intl.md

-If the `small-icu` option is used, one can still provide additional locale data
-at runtime so that the JS methods would work for all ICU locales. Assuming the
+If the `full-icu` option is used, one can still provide additional data
+at runtime. (This data does not override the built in data.) Assuming the


silverwind · 2018-07-06T15:56:30Z

BUILDING.md

-```console
-> .\vcbuild full-icu download-all
-```
+enabled by default. The `small-icu` option has been removed.


"The previously available small-icu option has been removed"

srl295 · 2018-07-06T16:28:15Z

@silverwind

it could also be seen as breaking as we remove a build option.

You know, perhaps I should accept --with-intl=small-icu as an alias for full-icu.

Also, in intl.md I will add a note that you can customize ICU itself. I don't have great docs
to point to here, but the node docs already link to ICU's docs under references.

vsemozhetbyt · 2018-07-06T17:05:32Z

It seems this section also needs updating (particularly, the "Encodings Supported by Default (With ICU)" part):

https://nodejs.org/download/nightly/v11.0.0-nightly201807060ef04b8133/docs/api/util.html#util_whatwg_supported_encodings

silverwind · 2018-07-06T17:42:52Z

perhaps I should accept --with-intl=small-icu as an alias for full-icu.

Sounds good. I'd make sure to print a warning that the option was removed and full-icu is assumed. That way, we can avoid the major bump.

srl295 · 2018-07-09T20:32:57Z

doc/api/intl.md

+### `small-icu`
+
+Support for `small-icu` (English only) has been removed. See ICU’s
+documentation for information on how to customize your ICU build.


should link to: http://userguide.icu-project.org/packaging#TOC-Reduce-ICU-Data-used

srl295 · 2018-07-09T21:11:12Z

ok. a lot of churn due to renaming icu-small to icu, but should be a one time…

- remove mechanism for 'small' icu - fix license-builder - remove a copy step in icu-generic - update docs Fixes: nodejs#19214

silverwind

LGTM, I wouldn't go with LFS, it would only introduce additional complexity, and the files aren't that big after all.

srl295 · 2018-07-11T19:35:02Z

ok- i talked with @mhdawson a bit, and we are going to explore #3460 a little bit more. Putting this on hold- but at least, i think it shows a viable option.

vsemozhetbyt · 2018-10-03T20:38:33Z

Is it worth to consider this as Node.js 11 feature?

srl295 · 2018-10-03T22:18:51Z

@vsemozhetbyt could be, but needs some consensus on direction. (NB this PR could be updated to 63.1 after #23244 lands )

refack · 2018-10-03T23:29:32Z

What are the benefits of this over the current setup? AFAICT we could set --with-intl=full-icu for the release builds, and keep both the repo and local dev builds as is?

vsemozhetbyt · 2018-11-02T16:03:34Z

Refs: #24039

Trott · 2018-11-25T02:38:04Z

@srl295 Is this still a thing that's being worked on?

srl295 · 2018-11-26T18:40:23Z

@refack The benefit is simplifying (though increasing the size of) what's in the repo.

@Trott Not actively. It sounds like there's some preference for
#3460

BridgeAR · 2019-03-10T01:00:47Z

Closing due to long inactivity and it seems like there is preference for a different approach. @srl295 please reopen / open a new PR in case you would like to work on this again.

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Jul 5, 2018

srl295 self-assigned this Jul 5, 2018

srl295 requested review from mscdex, silverwind, bnoordhuis, jasnell, TimothyGu and devsnek July 5, 2018 22:55

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 5, 2018

srl295 force-pushed the full-icu-62 branch from 7d582f5 to 114c6a8 Compare July 5, 2018 22:59

srl295 force-pushed the full-icu-62 branch from 114c6a8 to 2281d49 Compare July 5, 2018 23:07

devsnek approved these changes Jul 5, 2018

View reviewed changes

richardlau reviewed Jul 6, 2018

View reviewed changes

TimothyGu reviewed Jul 6, 2018

View reviewed changes

srl295 force-pushed the full-icu-62 branch 2 times, most recently from 56f5608 to 28ca326 Compare July 6, 2018 01:27

TimothyGu reviewed Jul 6, 2018

View reviewed changes

silverwind reviewed Jul 6, 2018

View reviewed changes

silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 6, 2018

srl295 force-pushed the full-icu-62 branch from 02985d6 to b959f30 Compare July 6, 2018 20:45

srl295 force-pushed the full-icu-62 branch from 63fc451 to b39ed23 Compare July 9, 2018 19:05

srl295 commented Jul 9, 2018

View reviewed changes

srl295 mentioned this pull request Jul 9, 2018

deps: ICU 62.1 bump #21728

Closed

2 tasks

srl295 changed the title ~~deps: bump full icu 62.1~~ 🚧 deps: bump full icu 62.1 Jul 9, 2018

srl295 changed the title ~~🚧 deps: bump full icu 62.1~~ WIP: deps: bump full icu 62.1 Jul 9, 2018

srl295 added the wip Issues and PRs that are still a work in progress. label Jul 9, 2018

srl295 force-pushed the full-icu-62 branch 2 times, most recently from 1310723 to 8732b12 Compare July 9, 2018 21:08

srl295 changed the title ~~WIP: deps: bump full icu 62.1~~ WIP: deps: checkin a full ICU Jul 9, 2018

richardlau approved these changes Jul 10, 2018

View reviewed changes

srl295 force-pushed the full-icu-62 branch from 8732b12 to 0707886 Compare July 10, 2018 22:09

addaleax force-pushed the master branch from e0cc867 to 538acea Compare July 10, 2018 22:17

srl295 added 2 commits July 10, 2018 15:21

tools: update tools and configure to expect full-icu

b844da4

- remove mechanism for 'small' icu - fix license-builder - remove a copy step in icu-generic - update docs Fixes: nodejs#19214

deps: icu 62.1 small to full

fce1016

srl295 force-pushed the full-icu-62 branch from 0707886 to fce1016 Compare July 10, 2018 22:22

cjihrig approved these changes Jul 11, 2018

View reviewed changes

silverwind approved these changes Jul 11, 2018

View reviewed changes

richardlau mentioned this pull request Sep 18, 2018

Dist with embedded icu same as modern browsers #22926

Closed

refack mentioned this pull request Nov 2, 2018

intl: Will Intl.Segmenter need full ICU in Node.js even for the 'en' locale? #24039

Closed

BridgeAR closed this Mar 10, 2019

refack added the stalled Issues and PRs that are stalled. label Mar 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: deps: checkin a full ICU #21676

WIP: deps: checkin a full ICU #21676

srl295 commented Jul 5, 2018 •

edited

Loading

nodejs-github-bot commented Jul 5, 2018

srl295 commented Jul 5, 2018

srl295 commented Jul 5, 2018 •

edited

Loading

srl295 commented Jul 5, 2018

devsnek left a comment

richardlau left a comment

TimothyGu left a comment

TimothyGu Jul 6, 2018 •

edited

Loading

srl295 Jul 6, 2018

TimothyGu Jul 6, 2018

srl295 Jul 6, 2018

srl295 Jul 6, 2018 •

edited

Loading

TimothyGu Jul 6, 2018

srl295 Jul 6, 2018 •

edited

Loading

srl295 Jul 6, 2018

silverwind commented Jul 6, 2018

silverwind Jul 6, 2018

srl295 Jul 6, 2018

silverwind Jul 6, 2018 •

edited

Loading

silverwind Jul 6, 2018 •

edited

Loading

srl295 commented Jul 6, 2018

vsemozhetbyt commented Jul 6, 2018

silverwind commented Jul 6, 2018

srl295 Jul 9, 2018 •

edited

Loading

srl295 commented Jul 9, 2018

silverwind left a comment

srl295 commented Jul 11, 2018

vsemozhetbyt commented Oct 3, 2018

srl295 commented Oct 3, 2018 •

edited

Loading

refack commented Oct 3, 2018

vsemozhetbyt commented Nov 2, 2018

Trott commented Nov 25, 2018

srl295 commented Nov 26, 2018

BridgeAR commented Mar 10, 2019


		You are ready to check in the updated `deps/small-icu`. This is a big commit,
		You are ready to check in the updated `deps/full-icu`. This is a big commit,

WIP: deps: checkin a full ICU #21676

WIP: deps: checkin a full ICU #21676

Conversation

srl295 commented Jul 5, 2018 • edited Loading

Checklist

nodejs-github-bot commented Jul 5, 2018

srl295 commented Jul 5, 2018

srl295 commented Jul 5, 2018 • edited Loading

srl295 commented Jul 5, 2018

devsnek left a comment

Choose a reason for hiding this comment

richardlau left a comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

srl295 Jul 6, 2018

Choose a reason for hiding this comment

TimothyGu Jul 6, 2018

Choose a reason for hiding this comment

srl295 Jul 6, 2018

Choose a reason for hiding this comment

srl295 Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

TimothyGu Jul 6, 2018

Choose a reason for hiding this comment

srl295 Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

srl295 Jul 6, 2018

Choose a reason for hiding this comment

silverwind commented Jul 6, 2018

silverwind Jul 6, 2018

Choose a reason for hiding this comment

srl295 Jul 6, 2018

Choose a reason for hiding this comment

silverwind Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

silverwind Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

srl295 commented Jul 6, 2018

vsemozhetbyt commented Jul 6, 2018

silverwind commented Jul 6, 2018

srl295 Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

srl295 commented Jul 9, 2018

silverwind left a comment

Choose a reason for hiding this comment

srl295 commented Jul 11, 2018

vsemozhetbyt commented Oct 3, 2018

srl295 commented Oct 3, 2018 • edited Loading

refack commented Oct 3, 2018

vsemozhetbyt commented Nov 2, 2018

Trott commented Nov 25, 2018

srl295 commented Nov 26, 2018

BridgeAR commented Mar 10, 2019

srl295 commented Jul 5, 2018 •

edited

Loading

srl295 commented Jul 5, 2018 •

edited

Loading

TimothyGu Jul 6, 2018 •

edited

Loading

srl295 Jul 6, 2018 •

edited

Loading

srl295 Jul 6, 2018 •

edited

Loading

silverwind Jul 6, 2018 •

edited

Loading

silverwind Jul 6, 2018 •

edited

Loading

srl295 Jul 9, 2018 •

edited

Loading

srl295 commented Oct 3, 2018 •

edited

Loading