FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

process: correctly parse Unicode in NODE_OPTIONS by bzoz · Pull Request #34476 · nodejs/node · GitHub

/ node Public

process: correctly parse Unicode in NODE_OPTIONS - #34476

Closed
bzoz wants to merge 5 commits into
nodejs:masterfrom
JaneaSystems:unicode-node-options
ConversationCommits (5)ChecksFiles changed
Closed

process: correctly parse Unicode in NODE_OPTIONS#34476
bzoz wants to merge 5 commits into
nodejs:masterfrom
JaneaSystems:unicode-node-options

Conversation

bzoz commented Jul 22, 2020

Copy link
Copy Markdown
Contributor

Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed
correctly.

Fixes: #34399

Reused the code from node_env_var.cc for getting the enviroment variable value.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 22, 2020

nodejs-github-bot commented Jul 22, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32470/

nodejs-github-bot commented Jul 22, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32475/

lundibundi approved these changes Jul 22, 2020
View reviewed changes
Comment thread test/parallel/test-unicode-node-options.js Outdated
Comment on lines 11 to 20

lundibundi Jul 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit:

Suggested change
const NODE_OPTIONS = `--redirect-warnings=${expected_redirect_value}`;
const result = cp.spawnSync(process.argv0,
['--expose-internals', __filename, 'test'],
{
env: {
NODE_OPTIONS
}
});
const env = { NODE_OPTIONS: `--redirect-warnings=${expected_redirect_value}` };
const result = cp.spawnSync(process.argv0,
['--expose-internals', __filename, 'test'],
{ env });

Comment thread test/parallel/test-unicode-node-options.js Outdated

lundibundi Jul 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this necessary?

Comment thread test/parallel/test-unicode-node-options.js Outdated

lundibundi Jul 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: I'd prefer to have explicit check for 'child process' case (i.e. if (process.argv[2] === 'test') {) instead to handle possibly different test start args.

Comment thread test/parallel/test-unicode-node-options.js Outdated

lundibundi Jul 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
const result = cp.spawnSync(process.argv0,
const result = cp.spawnSync(process.execPath,

This was referenced Jul 23, 2020
Open
Open
bzoz force-pushed the unicode-node-options branch from f5f8a79 to 1a31d81 Compare August 12, 2020 13:46

nodejs-github-bot commented Aug 12, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32745/

bzoz commented Aug 12, 2020

Copy link
Copy Markdown
Contributor Author

Fixed some nits and rebased.

This was referenced Aug 13, 2020
Open
Open

nodejs-github-bot commented Aug 13, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32755/

This was referenced Aug 14, 2020
Open
Open
Open
Open
Open
Open

bzoz commented Aug 20, 2020

Copy link
Copy Markdown
Contributor Author

Ok, this is weird. This fails only on ubuntu1804_sharedlibs_openssl111_x64 with this error message:

ot ok 2466 parallel/test-unicode-node-options
  ---
  duration_ms: 0.144
  severity: fail
  exitcode: 1
  stack: |-
    out/Release/node: /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1: version `OPENSSL_1_1_1b' not found (required by out/Release/node)
    assert.js:103
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    
    1 !== 0
    
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-unicode-node-options.js:18:10)
        at Module._compile (internal/modules/cjs/loader.js:1089:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:1110:10)
        at Module.load (internal/modules/cjs/loader.js:954:32)
        at Function.Module._load (internal/modules/cjs/loader.js:795:14)
        at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
        at internal/main/run_main_module.js:17:47 {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 1,
      expected: 0,
      operator: 'strictEqual'
    }
  ...

It looks like other tests can spawn new Node instances just fine. I'm stuck here 🤔

cjihrig reviewed Aug 20, 2020
View reviewed changes
Comment thread test/parallel/test-unicode-node-options.js Outdated

cjihrig Aug 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does this help with the current CI failure?

Suggested change
env: { NODE_OPTIONS },
env: { ...process.env, NODE_OPTIONS },

bzoz Aug 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, it helped. Thanks!

bzoz force-pushed the unicode-node-options branch from 398d7d5 to 15ac48a Compare August 20, 2020 01:46

bzoz commented Aug 20, 2020

Copy link
Copy Markdown
Contributor Author

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/32863/

bzoz added a commit to JaneaSystems/node that referenced this pull request Aug 20, 2020
process: correctly parse Unicode in NODE_OPTIONS
de565ad 
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed
correctly.

Fixes: nodejs#34399

PR-URL: nodejs#34476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>

bzoz commented Aug 20, 2020

Copy link
Copy Markdown
Contributor Author

Green CI, landed in de565ad

bzoz closed this Aug 20, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
process: correctly parse Unicode in NODE_OPTIONS
fd6efbe 
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed
correctly.

Fixes: #34399

PR-URL: #34476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
danielleadams mentioned this pull request Aug 20, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
process: correctly parse Unicode in NODE_OPTIONS
a16f0f4 
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed
correctly.

Fixes: #34399

PR-URL: #34476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
process: correctly parse Unicode in NODE_OPTIONS
14c7502 
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed
correctly.

Fixes: #34399

PR-URL: #34476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
process: correctly parse Unicode in NODE_OPTIONS
53eae0d 
Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed
correctly.

Fixes: #34399

PR-URL: #34476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
codebytere mentioned this pull request Sep 28, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig left review comments
jasnell jasnell approved these changes
addaleax addaleax approved these changes
+2 more reviewers
devsnek devsnek approved these changes
lundibundi lundibundi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unicode characters are not properly parsed from NODE_OPTIONS on Windows

7 participants

Back | FazBrowse Home | New Git URL