Intl: Build "English-only" binaries for the distribution.

[srl295]

A new issue to continue the discussion in #6371 and #4689 - especially to implement #6371 (comment)

• provide a way to build against any ICU, not just Chromium's
• default build has very little data (just English) for a small file size
• default disk location at build time for additional data --with-default-icu-path=...
• runtime option --icu-data-dir=path.. or NODE_ICU_DATA env var can override the default
• ICU won't be in github, but can be enabled by default in builds.

/cc @tjfontaine @trevnorris

[srl295]

opened a couple of tickets in v8 that may be needed here.

[trevnorris]

Does static bool InitializeICU(const char* icu_data_file = NULL); not do enough? The API doesn't say it can't be run multiple times for each icu_data_file.

[srl295]

@trevnorris that one has some odd behavior (maybe fixable) , ignored under some build configs, and also reads the entire file into memory (fread) before applying it. My version just configures ICU to do the right thing.

Could maybe improve the API upstream (v8) and just have one function.

I actually think that InitializeICU's model may be what we will need to go with eventually, though. u_setDataDirectory() doesn't have the effect I expected it to have - in particular, it cannot override inbuilt data.

[srl295]

@tjfontaine as #7719 is nearing completion and is marked for v0.12 .. what else will need to happen so that we can get "icu .. enabled by default in builds, but not necessarily included in github"? Who builds the binaries for http://nodejs.org and is there anything that should be done to make things easier for building those?

[trevnorris]

Three things I'd like fixed before --with-intl is part of the default build:

1. ./configure shouldn't show the path to every source file that's being included for build.
2. The warning: overriding commands for target messages should be fixed (if this is a problem with gyp then either upstream the fix and update gyp, or float a patch).
3. Determine how the icu dep is included. The icu4c-53_1-src.tgz decompresses to 108 MB, but we only need ~40MB of that to do the build. So either we strip it down and include only the necessary or we download it at compile time. I don't know what we want to do, but the decision will have to be made.

[srl295]

@trevnorris

• #1 - will do.
• #2 Who is the best contact or contacts to figure this out? EDIT done in build: i18n: fix issue with icu toolset dependencies #8681
• #3 There are pros and cons here. For example, the build doesn't need ICU's unit tests, but I hesitate to separate them because they give insight into build correctness. I would like to keep investigating reducing the on disk footprint as well.

[trevnorris]

@bnoordhuis You have any ideas about the possible bug in gyp wrt the warning: overriding commands for target message?

[bnoordhuis]

I looked into it and I know what causes it now but I'm not sure how to fix it. You get that warning when using multiple toolsets for a target of type 'executable' (because both binaries end up in the PRODUCT_DIR directory) or when multiple actions have the same filename as their output (for pretty much the same reason.)

Neither is something that @srl295's patch seems to do outright but perhaps it's the result of transitive dependencies. It may be a bug in our .gyp and .gypi files after all.

[srl295]

@trevnorris re #3 in #7676 (comment) - where do we go from here? I would recommend just redownloading it at this point- source is actually sourceforge mirrored now, could cache locally for buildbots - what do you think?

[srl295]

@bnoordhuis @trevnorris and I'm still not sure what to do about #2 in the same.

[srl295]

• note that ICU 54 is now available. I will look to see if anything needs to be done from a size perspective.
• Also FYI build: i18n: fix a config option that doesn't work on Mac. #8602 is a PR to fix a Mac build issue.

[bnoordhuis]

Apropos the warnings from the Makefile, I'm not sure if that is a bug in GYP or in our .gyp files but it's because some targets and actions have both host and target toolsets. Maybe try asking on the mailing list?

[srl295]

@bnoordhuis sure.. I will try that. Last I checked there weren't docs on toolset

[trevnorris]

@srl295 re #2. I recently ran into this. It occurred when a build and one of its dependencies had the same target_name. In the generated out/tools/icu/icui18n.{host,target}.mk you can see the following:

all_deps += $(obj).target/tools/icu/libicui18n.a
# Add target alias
.PHONY: icui18n
icui18n: $(obj).host/tools/icu/libicui18n.a

# Add target alias to "all" target.
.PHONY: all
all: icui18n

# Add target alias
.PHONY: icui18n
icui18n: $(builddir)/libicui18n.a

That target exists in both tools/icu/icu-{generic,system}.gyp.

[srl295]

@trevnorris yes, but why bother having toolsets be a list? Both the host and the target need this library. Maybe the generator should be generating icui18n-host: .. and icui18n-target: ..? Anyways, I'll ask over on gyp-developer

[trevnorris]

@srl295 that's actually exactly what I had to do. Appended something the target_name and it all magically worked.

[trevnorris]

Though in this case it does look like V8 requires that specific name. From deps/v8/tools/gyp/v8.gyp:

        ['v8_enable_i18n_support==1', {
          'dependencies': [
            '<(icu_gyp_path):icui18n',
            '<(icu_gyp_path):icuuc',
          ]

I'll see if I can fix it.

[trevnorris]

Here's a partial "fix"

diff --git a/tools/icu/icu-generic.gyp b/tools/icu/icu-generic.gyp
index bc0a660..8848096 100644
--- a/tools/icu/icu-generic.gyp
+++ b/tools/icu/icu-generic.gyp
@@ -90,7 +90,7 @@
     {
       'target_name': 'icui18n',
       'type': '<(library)',
-      'toolsets': [ 'host', 'target' ],
+      'toolsets': [ 'target' ],
       'sources': [
         '<@(icu_src_i18n)'
       ],
@@ -108,6 +108,27 @@
       },
       'export_dependent_settings': [ 'icuucx' ],
     },
+    {
+      'target_name': 'icui18n-host',
+      'type': '<(library)',
+      'toolsets': [ 'host' ],
+      'sources': [
+        '<@(icu_src_i18n)'
+      ],
+      'include_dirs': [
+        '../../deps/icu/source/i18n',
+      ],
+      'defines': [
+        'U_I18N_IMPLEMENTATION=1',
+      ],
+      'dependencies': [ 'icuucx-host', 'icu_implementation', 'icu_uconfig' ],
+      'direct_dependent_settings': {
+        'include_dirs': [
+          '../../deps/icu/source/i18n',
+        ],
+      },
+      'export_dependent_settings': [ 'icuucx-host' ],
+    },
     # this library is only built for derb..
     {
       'target_name': 'icuio',
@@ -122,13 +143,13 @@
       'defines': [
         'U_IO_IMPLEMENTATION=1',
       ],
-      'dependencies': [ 'icuucx', 'icui18n', 'icu_implementation', 'icu_uconfig' ],
+      'dependencies': [ 'icuucx-host', 'icui18n-host', 'icu_implementation', 'icu_uconfig' ],
       'direct_dependent_settings': {
         'include_dirs': [
           '../../deps/icu/source/io',
         ],
       },
-      'export_dependent_settings': [ 'icuucx', 'icui18n' ],
+      'export_dependent_settings': [ 'icuucx-host', 'icui18n-host' ],
     },
     # This exports actual ICU data
     {
@@ -288,7 +309,19 @@
     {
       'target_name': 'icustubdata',
       'type': '<(library)',
-      'toolsets': [ 'host', 'target' ],
+      'toolsets': [ 'target' ],
+      'dependencies': [ 'icu_implementation' ],
+      'sources': [
+        '<@(icu_src_stubdata)'
+      ],
+      'include_dirs': [
+        '../../deps/icu/source/common',
+      ],
+    },
+    {
+      'target_name': 'icustubdata-host',
+      'type': '<(library)',
+      'toolsets': [ 'host' ],
       'dependencies': [ 'icu_implementation' ],
       'sources': [
         '<@(icu_src_stubdata)'
@@ -313,7 +346,35 @@
       'target_name': 'icuucx',
       'type': '<(library)',
       'dependencies': [ 'icu_implementation', 'icu_uconfig' ],
-      'toolsets': [ 'host', 'target' ],
+      'toolsets': [ 'target' ],
+      'sources': [
+        '<@(icu_src_common)'
+      ],
+      'include_dirs': [
+        '../../deps/icu/source/common',
+      ],
+      'defines': [
+        'U_COMMON_IMPLEMENTATION=1',
+      ],
+      'export_dependent_settings': [ 'icu_uconfig' ],
+      'direct_dependent_settings': {
+        'include_dirs': [
+          '../../deps/icu/source/common',
+        ],
+        'conditions': [
+          [ 'OS=="win"', {
+            'link_settings': {
+              'libraries': [ '-lAdvAPI32.Lib', '-lUser32.lib' ],
+            },
+          }],
+        ],
+      },
+    },
+    {
+      'target_name': 'icuucx-host',
+      'type': '<(library)',
+      'dependencies': [ 'icu_implementation', 'icu_uconfig' ],
+      'toolsets': [ 'host' ],
       'sources': [
         '<@(icu_src_common)'
       ],
@@ -342,7 +403,7 @@
       'target_name': 'icutools',
       'type': '<(library)',
       'toolsets': [ 'host' ],
-      'dependencies': [ 'icuucx', 'icui18n', 'icustubdata' ],
+      'dependencies': [ 'icuucx-host', 'icui18n-host', 'icustubdata-host' ],
       'sources': [
         '<@(icu_src_tools)'
       ],
@@ -358,7 +419,7 @@
           '../../deps/icu/source/tools/toolutil',
         ],
       },
-      'export_dependent_settings': [ 'icuucx', 'icui18n', 'icustubdata' ],
+      'export_dependent_settings': [ 'icuucx-host', 'icui18n-host', 'icustubdata-host' ],
     },
     # This tool is needed to rebuild .res files from .txt,
     # or to build index (res_index.txt) files for small-icu
     # or to build index (res_index.txt) files for small-icu
@@ -366,7 +427,7 @@
       'target_name': 'genrb',
       'type': 'executable',
       'toolsets': [ 'host' ],
-      'dependencies': [ 'icutools', 'icuucx', 'icui18n' ],
+      'dependencies': [ 'icutools', 'icuucx-host', 'icui18n-host' ],
       'sources': [
         '<@(icu_src_genrb)'
       ],
@@ -382,7 +443,7 @@
       'target_name': 'iculslocs',
       'toolsets': [ 'host' ],
       'type': 'executable',
-      'dependencies': [ 'icutools', 'icuucx', 'icui18n', 'icuio' ],
+      'dependencies': [ 'icutools', 'icuucx-host', 'icui18n-host', 'icuio' ],
       'sources': [
         'iculslocs.cc',
         'no-op.cc',
@@ -394,7 +455,7 @@
       'target_name': 'icupkg',
       'toolsets': [ 'host' ],
       'type': 'executable',
-      'dependencies': [ 'icutools', 'icuucx', 'icui18n' ],
+      'dependencies': [ 'icutools', 'icuucx-host', 'icui18n-host' ],
       'sources': [
         '<@(icu_src_icupkg)',
         'no-op.cc',
@@ -405,7 +466,7 @@
       'target_name': 'genccode',
       'toolsets': [ 'host' ],
       'type': 'executable',
-      'dependencies': [ 'icutools', 'icuucx', 'icui18n' ],
+      'dependencies': [ 'icutools', 'icuucx-host', 'icui18n-host' ],
       'sources': [
         '<@(icu_src_genccode)',
         'no-op.cc',

Maybe there's a better way to do this.

[srl295]

@trevnorris Argh! That's exactly what I was trying to avoid- duplicating everything- seems very fragile. I posted this - https://groups.google.com/forum/#!topic/gyp-developer/NrFzsJd2l78

[trevnorris]

@srl295 Thanks for posting that. Though I'll be surprised if anyone responds. Honestly I'm not super familiar with gyp, but maybe there's a way to use a variable. I forget how gyp uses those, but something like:

'dependencies': [ 'icutools-<@(toolset_type)', 'icuucx-<@(toolset_type)', ... ]

No idea how that would actually work, but might be worth a try.

[srl295]

@trevnorris in between Unicode committe and Unicode conference. . but, I can try to do something like the above if there's a short window to v0.12

[trevnorris]

@srl295 The above should be the quickest solution. Why don't we have that one on hand just in case we can't figure out something better by the time v0.12 is released.

[misterdjules]

I find that having this "general discussion" issue makes it difficult to track the current state of this work.

If I understand correctly, here's what we want regarding the distribution of binaries that include ICU support:

• First, release binaries with "small" ICU support, the target for that is the 0.11.15 milestone. This is already supported, only Jenkins jobs must be updated to use --with-intl=small-icu and vcbuild.bat small-icu.
• When i18n: Allow locating ICU data relative to 'exec_path'. #8983 is implemented, include "full" ICU data in binary distributions (Windows and OS X installers, as well as binary tarball). This can be done in a later milestone, like 0.12.1, because it doesn't change node's API.

If we agree on that, I suggest closing this issue and creating two separate issues that correspond to the two points mentioned above.

@tjfontaine @srl295 @trevnorris What do you think?

[srl295]

First, release binaries with "small" ICU support, the target for that is
the 0.11.15 milestone. This is already supported, only Jenkins jobs must be
updated to use --with-intl=small-icu and vcbuild.bat small-icu.

But, that's the only and entire point of #7676 - if that's true,, it should
be considered fixed.

[misterdjules]

@srl295 Ok, I was just confused by some comments that mention several sub-issues/sub-tasks that we may want to address separately. This issue also contains comments that were not updated and I think that just adds to the confusion.

Like I said I would rather have each issue/PR address only "atomic" issues so that we can track them more easily.

I just tested the small-icu build on all supported platforms here: http://jenkins.nodejs.org/job/nodejs-julien/235/ and here: http://jenkins.nodejs.org/job/nodejs-julien-windows/77/, and there is no regression. The failing test on Windows has nothing to do with ICU-related changes. It was introduced by b636ba8 and is already tracked by #8986.

However, in my previous comment I failed to mention that some more work needs to be done to have small-icu support enabled for some binary packages (OSX pkg and binary tarballs). Basically, the base Makefile needs to be updated to pass with-intl=small-icu to the configure script. I will take care of that.

So as soon as the changes needed to make binary packages include small-icu land, I suggest closing this issue and we'll start creating several more specific issues. @srl295 How does that sound?

[srl295]

Sounds good to me... more later.

[misterdjules]

@srl295 I just submitted #8994 that enables small-icu support for all binary packages.

[misterdjules]

#8994 Landed in 67f87a7, so closing this issue. @srl295 Please ping me on #libuv when you have some time to discuss the next steps.

Thanks all!