src: fix recent --icu-data-dir= regression by bnoordhuis · Pull Request #11255 · nodejs/node · GitHub

Commit a8734af ("src: make copies of startup environment variables")
introduced a regression in the capturing of the --icu-data-dir= switch.

This commit fixes that and shuffles code around so it can be properly
unit-tested.

cc @sam-github @srl295

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).

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.

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.

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).

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.

Just ran a quick build/test with configure --without-intl and other tests fail so I'm +0 about updating the new test to work without intl.

For the record, these tests fail with configure --without-intl (only test-intl-no-icu-data is relevant to this PR):

-bash-4.2$ ./tools/test.py parallel
=== release test-cluster-inspector-debug-port ===
Path: parallel/test-cluster-inspector-debug-port
Inspector support is not available with this Node.js build
out/Release/node: bad option: --inspect=12346
Command: out/Release/node --inspect=12346 /home/users/riclau/sandbox/github/node/test/parallel/test-cluster-inspector-debug-port.js
=== release test-intl-no-icu-data ===
Path: parallel/test-intl-no-icu-data
out/Release/node: bad option: --icu-data-dir=test/fixtures/empty/
Command: out/Release/node --icu-data-dir=test/fixtures/empty/ /home/users/riclau/sandbox/github/node/test/parallel/test-intl-no-icu-data.js
=== release test-url-domain-ascii-unicode ===
Path: parallel/test-url-domain-ascii-unicode
assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 'Ä±ÃdÙÙ´' === 'xn--d-iga7ro0q9f'
    at domainWithASCII.forEach (/home/users/riclau/sandbox/github/node/test/parallel/test-url-domain-ascii-unicode.js:24:3)
    at Array.forEach (native)
    at Object.<anonymous> (/home/users/riclau/sandbox/github/node/test/parallel/test-url-domain-ascii-unicode.js:20:17)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:418:7)
Command: out/Release/node /home/users/riclau/sandbox/github/node/test/parallel/test-url-domain-ascii-unicode.js
=== release test-url-format-whatwg ===
Path: parallel/test-url-format-whatwg
assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 'http://xn--lck1c3crb1723bpq4a.com/a?a=b#c' === 'http://çå®¹ãã«ã ã©.com/a?a=b#c'
    at Object.<anonymous> (/home/users/riclau/sandbox/github/node/test/parallel/test-url-format-whatwg.js:79:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:418:7)
    at startup (bootstrap_node.js:139:9)
    at bootstrap_node.js:533:3
Command: out/Release/node /home/users/riclau/sandbox/github/node/test/parallel/test-url-format-whatwg.js
[03:56|% 100|+ 1259|-   4]: Done
-bash-4.2$

LGTM assuming that the change to the config binding property name is entirely internal, and not an API change.

The binding was introduced to avoid leaking implementation details that then become de facto API, so yes, we should be good on that front. :-)

I decided to split the change into two commits so that the actual bug fix isn't buried so much in the churn. New CI: https://ci.nodejs.org/job/node-test-pull-request/6353/

Move some code around so we can properly test whether the switch actually does anything. PR-URL: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

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: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

@bnoordhuis the commits are not landing in v7.x clearly, any plan to backport?

@italoacasas See #11051 (comment), this PR depends on a few others but @sam-github is working on it.

Move some code around so we can properly test whether the switch actually does anything. PR-URL: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

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: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Move some code around so we can properly test whether the switch actually does anything. PR-URL: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

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: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Move some code around so we can properly test whether the switch actually does anything. PR-URL: #11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

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: #11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Move some code around so we can properly test whether the switch actually does anything. PR-URL: #11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

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: #11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Move some code around so we can properly test whether the switch actually does anything. PR-URL: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

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: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. process Issues and PRs related to the process subsystem. labels Feb 9, 2017

bnoordhuis mentioned this pull request Feb 9, 2017

src: make copies of startup environment variables #11051

Closed

richardlau reviewed Feb 9, 2017

View reviewed changes

jasnell approved these changes Feb 9, 2017

View reviewed changes

sam-github approved these changes Feb 9, 2017

View reviewed changes

bnoordhuis force-pushed the fix-icu-data-dir branch from 2d92b81 to ab1143c Compare February 11, 2017 13:14

bnoordhuis added 2 commits February 11, 2017 15:23

src: make --icu-data-dir= switch testable

46345b9

Move some code around so we can properly test whether the switch actually does anything. PR-URL: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

bnoordhuis force-pushed the fix-icu-data-dir branch from ab1143c to 291b599 Compare February 11, 2017 14:26

bnoordhuis closed this Feb 11, 2017

bnoordhuis deleted the fix-icu-data-dir branch February 11, 2017 14:26

bnoordhuis merged commit 291b599 into nodejs:master Feb 11, 2017

sam-github mentioned this pull request Feb 13, 2017

7.x backport: crypto: support OPENSSL_CONF again (and its dependencies) #11345

Closed

4 tasks

targos mentioned this pull request Feb 14, 2017

deps: update V8 to 5.6 #10992

Merged

2 tasks

italoacasas added the dont-land-on-v7.x label Feb 14, 2017

jasnell added dont-land-on-v4.x labels Mar 7, 2017

evantorrie mentioned this pull request Apr 4, 2017

test-intl-no-icu-data.js fails unless node compiled --with-intl=small-icu #12198

Closed

richardlau mentioned this pull request Apr 5, 2017

test: Skip intl-no-icu-data test when not (Intl and smallICU) #12226

Closed

2 tasks

srl295 mentioned this pull request Dec 4, 2018

src: remove icuDataDir from node config #24780

Closed

2 tasks

src: fix recent --icu-data-dir= regression by bnoordhuis · Pull Request #11255 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src: fix recent --icu-data-dir= regression#11255
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:fix-icu-data-dir

bnoordhuis commented Feb 9, 2017

Uh oh!

richardlau Feb 9, 2017

Uh oh!

bnoordhuis Feb 9, 2017

Uh oh!

richardlau Feb 9, 2017

Uh oh!

richardlau commented Feb 9, 2017

Uh oh!

sam-github left a comment

Uh oh!

bnoordhuis commented Feb 9, 2017

Uh oh!

bnoordhuis commented Feb 11, 2017

Uh oh!

italoacasas commented Feb 13, 2017

Uh oh!

bnoordhuis commented Feb 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src: fix recent --icu-data-dir= regression by bnoordhuis · Pull Request #11255 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src: fix recent --icu-data-dir= regression#11255bnoordhuis merged 2 commits intonodejs:masternodejs/node:masterfrom bnoordhuis:fix-icu-data-dirbnoordhuis/io.js:fix-icu-data-dirCopy head branch name to clipboard

Conversation

bnoordhuis commented Feb 9, 2017

Uh oh!

richardlau Feb 9, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Feb 9, 2017

Choose a reason for hiding this comment

Uh oh!

richardlau Feb 9, 2017

Choose a reason for hiding this comment

Uh oh!

richardlau commented Feb 9, 2017

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Feb 9, 2017

Uh oh!

bnoordhuis commented Feb 11, 2017

Uh oh!

italoacasas commented Feb 13, 2017

Uh oh!

bnoordhuis commented Feb 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src: fix recent --icu-data-dir= regression#11255
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:fix-icu-data-dir