Add ICU support in embedded v8 (Updated).

[kishiguro]

This is PR for denoland/deno#1968 Add ICU support in embedded v8. Please note this PR is still work in progress. Following changes are made in this PR.

1. Add https://chromium.googlesource.com/chromium/deps/icu as third_part/icu (not as submodule)
2. Set v8_enable_i18n_support = true in .gn for ICU support.
3. Set icu_use_data_file = false in third_party/icu/config.gni
4. Remove leading '_' from symbol in case of Windows third_party/icu/scripts/make_data_assembly.py
5. Add ICU header path to third_party/icu/config/mac/BUILD.gn

At least this would be able to compile on all supported platform.

[ry]

@kishiguro Thanks for the PR. This is something we've been putting off for awhile, so I'm glad you're looking into it. Ideally ICU would be setup under a rust/cargo feature to allow people to turn it off if desired downstream...

[kishiguro]

@ry thanks for comment. Sure I hope that I can manage ICU support to be configurable. Still I'm having a build issue on Windows. Let me fix issue and polish build system so that ICU can be turned off.

[kishiguro]

At least it compiles on all of CI env. Let me polish build related changes.

[ry]

@kishiguro Sorry for the slow response! I wonder if ICU can be added as a git submodule rather than directly included in the repository?

[kishiguro]

@ry we need to modify ICU files such as

• icu/config.gni - set false to icu_use_data_file
• icu/scripts/make_data_assembly.py - modify Windows symbol

shall we clone upstream ICU repository as github.com/denoland/icu then submodule it?

[ry]

@kishiguro Yes we can float patches to ICU. For now, why not point the submodule to your own fork of ICU. When this patch is finally merged, we can update the submodule to denoland/icu.

[kishiguro]

@ry sure, let me work on that.

[kishiguro]

@ry done. ICU is integrated as a submodule.

[ry]

@kishiguro Nice!

I think there are couple things left to do:

1. Add a test to tests/test_api.rs demonstrating some i18n JS API
2. Be able to conditionally turn off ICU with a cargo feature flag (Eg cargo build --features=i18n)
3. Figure out how to remove these patches you've made to v8, build, and icu.

[ry]

The changes to v8 and build seem like they shouldn't be necessary. Can you describe the issue you're running into?

[kishiguro]

v8 changes are for fixing build error on Windows. Current v8 code uses icu:: for ICU namespace in isolate.h. On Windows, we need to abstract it with U_ICU_NAMESPACE. It is interesting that this error occurs only on Windows not on other platform such as Linux nor macOS. I'm not sure this is related to ICU Data embedded in library config or not. We may escalate this change to upstream. At least it better than current implementation to reflect namespace abstraction.

diff --git a/src/execution/isolate.h b/src/execution/isolate.h
index 3917d8cb4b..b3c13a61db 100644
--- a/src/execution/isolate.h
+++ b/src/execution/isolate.h
@@ -1172,9 +1172,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
       kDefaultCollator, kDefaultNumberFormat, kDefaultSimpleDateFormat,
       kDefaultSimpleDateFormatForTime, kDefaultSimpleDateFormatForDate};

-  icu::UMemory* get_cached_icu_object(ICUObjectCacheType cache_type);
+  U_ICU_NAMESPACE::UMemory* get_cached_icu_object(ICUObjectCacheType cache_type);
   void set_icu_object_in_cache(ICUObjectCacheType cache_type,
-                               std::shared_ptr<icu::UMemory> obj);
+                               std::shared_ptr<U_ICU_NAMESPACE::UMemory> obj);
   void clear_cached_icu_object(ICUObjectCacheType cache_type);
   void ClearCachedIcuObjects();

@@ -1691,7 +1691,7 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
       return static_cast<std::size_t>(a);
     }
   };
-  std::unordered_map<ICUObjectCacheType, std::shared_ptr<icu::UMemory>,
+  std::unordered_map<ICUObjectCacheType, std::shared_ptr<U_ICU_NAMESPACE::UMemory>,
                      ICUObjectCacheTypeHash>
       icu_object_cache_;

[kishiguro]

build changes are for finding #include <unicode/uversion.h> in icu/source/tools/gencolusb/extract_unsafe_backwards.cpp and some other ICU library codes. I agree with this change is not necessary and would be fixed in other part of the code. I'm not expert of GN/ninjya so I couldn't figure out better solution for it. It would be nice somebody who understand GN/ninjya suggest right way for the fix.

diff --git a/config/gcc/BUILD.gn b/config/gcc/BUILD.gn
index 747245f37..7a74cb1fb 100644
--- a/config/gcc/BUILD.gn
+++ b/config/gcc/BUILD.gn
@@ -38,6 +38,10 @@ config("symbol_visibility_hidden") {
     cflags_cc = [ "-fvisibility-inlines-hidden" ]
     cflags_objcc = cflags_cc
   }
+
+  # Temporally fix of ICU build.
+  cflags += [ "-I../../../../rusty_v8/third_party/icu/source/common" ]
+  cflags += [ "-I../../../../third_party/icu/source/common" ]
 }

 # This config is usually set when :symbol_visibility_hidden is removed.
@@ -45,6 +49,10 @@ config("symbol_visibility_hidden") {
 # which would error out otherwise (e.g. -fsanitize=cfi-unrelated-cast)
 config("symbol_visibility_default") {
   cflags = [ "-fvisibility=default" ]
+
+  # Temporally fix of ICU build.
+  cflags += [ "-I../../../../rusty_v8/third_party/icu/source/common" ]
+  cflags += [ "-I../../../../third_party/icu/source/common" ]
 }

 # The rpath is the dynamic library search path. Setting this config on a link
diff --git a/config/mac/BUILD.gn b/config/mac/BUILD.gn
index de8233bba..4deb03a60 100644
--- a/config/mac/BUILD.gn
+++ b/config/mac/BUILD.gn
@@ -47,6 +47,9 @@ config("compiler") {
   if (export_libcxxabi_from_executables) {
     ldflags += [ "-Wl,-undefined,dynamic_lookup" ]
   }
+
+  cflags += [ "-I../../../../rusty_v8/third_party/icu/source/common" ]
+  cflags += [ "-I../../../../third_party/icu/source/common" ]
 }

 # This is included by reference in the //build/config/compiler:runtime_library

[kishiguro]

1. Add a test to tests/test_api.rs demonstrating some i18n JS API
   -> Just added test cases for date and currency format using i18n JS API.
2. Be able to conditionally turn off ICU with a cargo feature flag (Eg cargo build --features=i18n)
   -> Working on this.

[ry]

@kishiguro I think we can help get this working (we are somewhat experts in ninja/gn). I'll get back to you in a few days.

[CLAassistant]

All committers have signed the CLA.

[ry]

@kishiguro I think we can avoid some of these build issues by binding the udata_setCommonData and allowing users to load their own data file.

Closing in favor of #603. I'm using your tests and will cite your contribution when landing. Thanks for getting the ball rolling - sorry it has taken us so long to act.