Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: fix recent --icu-data-dir= regression #11255

Merged
merged 2 commits into from
Feb 11, 2017
Merged

src: fix recent --icu-data-dir= regression #11255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46345b9
src: make --icu-data-dir= switch testable
bnoordhuis Feb 11, 2017
291b599
src: fix --icu-data-dir= regression
bnoordhuis Feb 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/internal/process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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;
}


Expand Down
15 changes: 2 additions & 13 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -3753,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) {
Expand Down
7 changes: 5 additions & 2 deletions src/node_config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,11 @@ void InitConfig(Local<Object> 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)
Expand Down
3 changes: 0 additions & 3 deletions src/node_i18n.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
}
Expand Down
2 changes: 1 addition & 1 deletion src/node_i18n.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

namespace node {

extern bool flag_icu_data_dir;
extern std::string icu_data_dir; // NOLINT(runtime/string)

namespace i18n {

Expand Down
9 changes: 9 additions & 0 deletions test/parallel/test-intl-no-icu-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Flags: --icu-data-dir=test/fixtures/empty/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag is invalid if Node.js is compiled without intl. and will probably cause the test runner to fail to execute the test (as opposed to gracefully skipping over it).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you're right but see nodejs/build#419. Since we don't test non-intl builds I feel somewhat ambivalent about complicating the test. I'll update it if you feel strongly about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I did a quick test and other tests are already failing without intl (see comment in main PR) so I'm okay with keeping the test as it is. If someone really does care about having clean runs with non-intl builds we can address in another PR.

'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/');