From 46345b937448dbeac54f33bd3a8d38be4c1dc1ef Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 11 Feb 2017 14:00:22 +0100 Subject: [PATCH 1/2] src: make --icu-data-dir= switch testable Move some code around so we can properly test whether the switch actually does anything. PR-URL: https://github.com/nodejs/node/pull/11255 Reviewed-By: James M Snell Reviewed-By: Sam Roberts --- lib/internal/process.js | 4 +--- src/node.cc | 13 +------------ src/node_config.cc | 7 +++++-- src/node_i18n.cc | 3 --- src/node_i18n.h | 2 +- test/parallel/test-intl-no-icu-data.js | 7 +++++++ 6 files changed, 15 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-intl-no-icu-data.js diff --git a/lib/internal/process.js b/lib/internal/process.js index 3f050b42cafe42..47c2c69b3e89f5 100644 --- a/lib/internal/process.js +++ b/lib/internal/process.js @@ -124,7 +124,7 @@ function setupConfig(_source) { const oldV8BreakIterator = Intl.v8BreakIterator; const des = Object.getOwnPropertyDescriptor(Intl, 'v8BreakIterator'); des.value = require('internal/util').deprecate(function v8BreakIterator() { - if (processConfig.hasSmallICU && !process.icu_data_dir) { + if (processConfig.hasSmallICU && !processConfig.icuDataDir) { // Intl.v8BreakIterator() would crash w/ fatal error, so throw instead. throw new Error('v8BreakIterator: full ICU data not installed. ' + 'See https://github.com/nodejs/node/wiki/Intl'); @@ -134,8 +134,6 @@ function setupConfig(_source) { 'DEP0017'); Object.defineProperty(Intl, 'v8BreakIterator', des); } - // Don’t let icu_data_dir leak through. - delete process.icu_data_dir; } diff --git a/src/node.cc b/src/node.cc index 040f9f597ad559..fc4ac88ba3d360 100644 --- a/src/node.cc +++ b/src/node.cc @@ -156,7 +156,7 @@ static const char* trace_enabled_categories = nullptr; #if defined(NODE_HAVE_I18N_SUPPORT) // Path to ICU data (for i18n / Intl) -static std::string icu_data_dir; // NOLINT(runtime/string) +std::string icu_data_dir; // NOLINT(runtime/string) #endif // used by C++ modules as well @@ -3095,17 +3095,6 @@ void SetupProcessObject(Environment* env, "ares", FIXED_ONE_BYTE_STRING(env->isolate(), ARES_VERSION_STR)); -#if defined(NODE_HAVE_I18N_SUPPORT) && defined(U_ICU_VERSION) - // ICU-related versions are now handled on the js side, see bootstrap_node.js - - if (!icu_data_dir.empty()) { - // Did the user attempt (via env var or parameter) to set an ICU path? - READONLY_PROPERTY(process, - "icu_data_dir", - OneByteString(env->isolate(), icu_data_dir.c_str())); - } -#endif - const char node_modules_version[] = NODE_STRINGIFY(NODE_MODULE_VERSION); READONLY_PROPERTY( versions, diff --git a/src/node_config.cc b/src/node_config.cc index a096372812d9ea..5c9c51585baf3a 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -39,8 +39,11 @@ void InitConfig(Local target, READONLY_BOOLEAN_PROPERTY("hasSmallICU"); #endif // NODE_HAVE_SMALL_ICU - if (flag_icu_data_dir) - READONLY_BOOLEAN_PROPERTY("usingICUDataDir"); + target->DefineOwnProperty(env->context(), + OneByteString(env->isolate(), "icuDataDir"), + OneByteString(env->isolate(), icu_data_dir.data())) + .FromJust(); + #endif // NODE_HAVE_I18N_SUPPORT if (config_preserve_symlinks) diff --git a/src/node_i18n.cc b/src/node_i18n.cc index 40d048fa36d0ce..648012a50601e3 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -70,8 +70,6 @@ using v8::Object; using v8::String; using v8::Value; -bool flag_icu_data_dir = false; - namespace i18n { const size_t kStorageSize = 1024; @@ -415,7 +413,6 @@ bool InitializeICUDirectory(const std::string& path) { #endif // !NODE_HAVE_SMALL_ICU return (status == U_ZERO_ERROR); } else { - flag_icu_data_dir = true; u_setDataDirectory(path.c_str()); return true; // No error. } diff --git a/src/node_i18n.h b/src/node_i18n.h index 0bfd3c5c859792..21567eeb3ec38f 100644 --- a/src/node_i18n.h +++ b/src/node_i18n.h @@ -10,7 +10,7 @@ namespace node { -extern bool flag_icu_data_dir; +extern std::string icu_data_dir; // NOLINT(runtime/string) namespace i18n { diff --git a/test/parallel/test-intl-no-icu-data.js b/test/parallel/test-intl-no-icu-data.js new file mode 100644 index 00000000000000..dd14c146719fb1 --- /dev/null +++ b/test/parallel/test-intl-no-icu-data.js @@ -0,0 +1,7 @@ +// Flags: --icu-data-dir=test/fixtures/empty/ +'use strict'; +require('../common'); +const assert = require('assert'); + +// No-op when ICU case mappings are unavailable. +assert.strictEqual('ç'.toLocaleUpperCase('el'), 'ç'); From 291b599bba0f484f35a2deebb2350b178c1f1758 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 11 Feb 2017 14:04:58 +0100 Subject: [PATCH 2/2] src: fix --icu-data-dir= regression Commit a8734af ("src: make copies of startup environment variables") from two weeks ago introduced a regression in the capturing of the `--icu-data-dir=` switch: it captured the string up to the `=` instead of what comes after it. PR-URL: https://github.com/nodejs/node/pull/11255 Reviewed-By: James M Snell Reviewed-By: Sam Roberts --- src/node.cc | 2 +- test/parallel/test-intl-no-icu-data.js | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node.cc b/src/node.cc index fc4ac88ba3d360..30c44c18ecb26e 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3742,7 +3742,7 @@ static void ParseArgs(int* argc, #endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) } else if (strncmp(arg, "--icu-data-dir=", 15) == 0) { - icu_data_dir.assign(arg, 15); + icu_data_dir.assign(arg + 15); #endif } else if (strcmp(arg, "--expose-internals") == 0 || strcmp(arg, "--expose_internals") == 0) { diff --git a/test/parallel/test-intl-no-icu-data.js b/test/parallel/test-intl-no-icu-data.js index dd14c146719fb1..ce5e9a08121cdc 100644 --- a/test/parallel/test-intl-no-icu-data.js +++ b/test/parallel/test-intl-no-icu-data.js @@ -2,6 +2,8 @@ 'use strict'; require('../common'); const assert = require('assert'); +const config = process.binding('config'); // No-op when ICU case mappings are unavailable. assert.strictEqual('ç'.toLocaleUpperCase('el'), 'ç'); +assert.strictEqual(config.icuDataDir, 'test/fixtures/empty/');