Skip to content

Commit

Permalink
src: check if --icu-data-dir= points to valid dir
Browse files Browse the repository at this point in the history
Call uc_init() after u_setDataDirectory() to find out if the data
directory is actually valid.

This commit removes parallel/test-intl-no-icu-data, added in commit
46345b9 ("src: make --icu-data-dir= switch testable").  It no longer
works now that an invalid --icu-data-dir= argument is rejected.
Coverage is now provided by parallel/test-icu-data-dir.

Fixes: #13043
Refs: nodejs/node-gyp#1199
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
  • Loading branch information
bnoordhuis committed May 19, 2017
1 parent 6f21671 commit 46e773c
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 14 deletions.
7 changes: 5 additions & 2 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4342,8 +4342,11 @@ void Init(int* argc,
// Initialize ICU.
// If icu_data_dir is empty here, it will load the 'minimal' data.
if (!i18n::InitializeICUDirectory(icu_data_dir)) {
FatalError(nullptr, "Could not initialize ICU "
"(check NODE_ICU_DATA or --icu-data-dir parameters)");
fprintf(stderr,
"%s: could not initialize ICU "
"(check NODE_ICU_DATA or --icu-data-dir parameters)",
argv[0]);
exit(9);
}
#endif

Expand Down
7 changes: 4 additions & 3 deletions src/node_i18n.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
#include <unicode/utypes.h>
#include <unicode/putil.h>
#include <unicode/uchar.h>
#include <unicode/uclean.h>
#include <unicode/udata.h>
#include <unicode/uidna.h>
#include <unicode/ucnv.h>
Expand Down Expand Up @@ -418,19 +419,19 @@ void GetVersion(const FunctionCallbackInfo<Value>& args) {
} // anonymous namespace

bool InitializeICUDirectory(const std::string& path) {
UErrorCode status = U_ZERO_ERROR;
if (path.empty()) {
UErrorCode status = U_ZERO_ERROR;
#ifdef NODE_HAVE_SMALL_ICU
// install the 'small' data.
udata_setCommonData(&SMALL_ICUDATA_ENTRY_POINT, &status);
#else // !NODE_HAVE_SMALL_ICU
// no small data, so nothing to do.
#endif // !NODE_HAVE_SMALL_ICU
return (status == U_ZERO_ERROR);
} else {
u_setDataDirectory(path.c_str());
return true; // No error.
u_init(&status);
}
return status == U_ZERO_ERROR;
}

int32_t ToUnicode(MaybeStackBuffer<char>* buf,
Expand Down
1 change: 0 additions & 1 deletion test/parallel/test-cli-node-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ if (common.hasCrypto) {
expect('--use-bundled-ca', 'B\n');
expect('--openssl-config=_ossl_cfg', 'B\n');
}
expect('--icu-data-dir=_d', 'B\n');

// V8 options
expect('--max_old_space_size=0', 'B\n');
Expand Down
19 changes: 19 additions & 0 deletions test/parallel/test-icu-data-dir.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';
require('../common');
const assert = require('assert');
const { spawnSync } = require('child_process');

const expected =
'could not initialize ICU ' +
'(check NODE_ICU_DATA or --icu-data-dir parameters)';

{
const child = spawnSync(process.execPath, ['--icu-data-dir=/', '-e', '0']);
assert(child.stderr.toString().includes(expected));
}

{
const env = { NODE_ICU_DATA: '/' };
const child = spawnSync(process.execPath, ['-e', '0'], { env });
assert(child.stderr.toString().includes(expected));
}
8 changes: 0 additions & 8 deletions test/parallel/test-intl-no-icu-data.js

This file was deleted.

2 comments on commit 46e773c

@MylesBorins
Copy link
Contributor

Choose a reason for hiding this comment

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

a PR-URL was not included in this commit

@bnoordhuis
Copy link
Member Author

Choose a reason for hiding this comment

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

It was #13053.

Please sign in to comment.