src: whitelist v8 options with '_' or '-' by sam-github · Pull Request #14093 · nodejs/node · GitHub

V8 options allow either '_' or '-' to be used in options as a seperator,
such as "--abort-on_uncaught-exception". Allow these case variations
when used with NODE_OPTIONS.

@richardlau , re #13932 (comment)

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

Affected core subsystem(s)

src

Is it worth the extra code? Maybe just give a better error then:

C:\Users\refael>set NODE_OPTIONS=--abort-on_uncaught-exception
C:\Users\refael>node
node: --abort-on_uncaught-exception is not allowed in NODE_OPTIONS

...please use the all dash variant...

Tiniest of nits: space after semicolon.

Node supports the variant spellings at the CLI, but not via NODE_OPTIONS, eventually someone (other than @richardlau ) is going to report this a bug, better to preempt.

This PR makes NODE_OPTIONS=--abort-on_uncaught-exception valid, just as node --abort-on_uncaught-exception is valid) so its not necessary to give a better error, because its not an error.

Node supports the variant spellings at the CLI, but not via NODE_OPTIONS, eventually someone (other than @richardlau ) is going to report this a bug, better to preempt.

This PR makes NODE_OPTIONS=--abort-on_uncaught-exception valid, just as node --abort-on_uncaught-exception is valid) so its not necessary to give a better error, because its not an error.

I'm just preaching parsimony and trying to K.I.S.S. If instead of allowing, we print a not "error" but a "direction", IMHO we can do it in less lines of code.
This PR also enables all node CLI options to work with _, e.g. --inspect_brk, which is a disparity with CLI invocation. read code again.

I don't see how fuzzy matching, and telling the person that we knew what they were trying to do, and know that it works from the CLI, but are just going to give them a nice error message instead of doing the right thing is helpful!
It does not, only V8 options are allowed to mix - and _, just like it is on the CLI.

% NODE_OPTIONS=--inspect_brk ./out/Release/node 
./out/Release/node: --inspect_brk is not allowed in NODE_OPTIONS
NODE_OPTIONS=--inspect-brk ./out/Release/node 
Debugger listening on ws://127.0.0.1:9229/6cb26d3b-f53e-4f57-ba16-7d38229a7c5e

Convinced

Could you fix the strange indent?

AFAICT this could be return true?

nit: define const printB_expectedOutput = 'B\n'; (Actualy I think '^B\n$' would be better).

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

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

Jenkins built the wrong commit last time, not sure why, trying again.

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

V8 options allow either '_' or '-' to be used in options as a seperator, such as "--abort-on_uncaught-exception". Allow these case variations when used with NODE_OPTIONS. PR-URL: #14093 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 97a6aa9

V8 options allow either '_' or '-' to be used in options as a seperator, such as "--abort-on_uncaught-exception". Allow these case variations when used with NODE_OPTIONS. PR-URL: #14093 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

@nodejs/lts I know we just did a semver-minor for 6.x, and thus it probably won't happen for a while, but it seems like it'd be helpful for people if we could backport this at some point, the errors you get with different versions of node are pretty opaque (see #16999).

I'd be up for considering this semver patch if we assume it likely should have landed in 6.12.0

@MylesBorins makes sense, the actual feature (supporting the args in NODE_OPTIONS) landed already, actually supporting the alternatives would be a bug fix.

Thoughts on how we avoid forgetting to include this in the next patch? Maybe just land on staging now?

Yup!

…

On Nov 14, 2017 8:38 AM, "Gibson Fahnestock" ***@***.***> wrote: @MylesBorins <https://github.com/mylesborins> makes sense, the actual feature (supporting the args in NODE_OPTIONS) landed already, not supporting the alternatives would be a bug fix. Thoughts on how we avoid forgetting to include this in the next patch? Maybe just land on staging now? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14093 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecVyA5sjepqCaisZC3XJLcXOX4d7g8ks5s2Ze2gaJpZM4OO5QD> .

V8 options allow either '_' or '-' to be used in options as a seperator, such as "--abort-on_uncaught-exception". Allow these case variations when used with NODE_OPTIONS. PR-URL: #14093 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

I've landed this on v6.x-staging and am going to be shipping it as semver patch

@nodejs/lts please lmk if you have concerns

V8 options allow either '_' or '-' to be used in options as a seperator, such as "--abort-on_uncaught-exception". Allow these case variations when used with NODE_OPTIONS. PR-URL: #14093 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * build: - fix npm install with --shared (Ben Noordhuis) #16438 * build: - building with python 3 is now supported (Emily Marigold Klassen) #16058 * src: - v8 options can be specified with either '\_' or '-' in NODE_OPTIONS (Sam Roberts) #14093 PR-URL: #17180

sam-github requested a review from richardlau July 5, 2017 21:30

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

sam-github force-pushed the allow-v8-options-with-underscore branch from c30731d to 46d25fe Compare July 5, 2017 21:30

richardlau approved these changes Jul 6, 2017

View reviewed changes

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 6, 2017

refack approved these changes Jul 6, 2017

View reviewed changes

sam-github force-pushed the allow-v8-options-with-underscore branch from e39106d to 2a2bba2 Compare July 6, 2017 21:15

sam-github force-pushed the allow-v8-options-with-underscore branch from 2a2bba2 to 083a4df Compare July 7, 2017 17:32

jasnell approved these changes Jul 10, 2017

View reviewed changes

sam-github closed this Jul 10, 2017

sam-github force-pushed the allow-v8-options-with-underscore branch from 083a4df to 97a6aa9 Compare July 10, 2017 19:08

sam-github deleted the allow-v8-options-with-underscore branch July 10, 2017 19:08

addaleax mentioned this pull request Jul 11, 2017

v8.2.0 proposal #13744

Merged

gibfahn added the lts-watch-v6.x label Nov 13, 2017

gibfahn mentioned this pull request Nov 13, 2017

v6.12.0: max-old-space-size can no longer be set via NODE_OPTIONS #16999

Closed

gibfahn added the lts-watch-v8.x label Nov 13, 2017

MylesBorins removed lts-watch-v6.x labels Nov 14, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

mischkl mentioned this pull request Mar 11, 2018

Best way to set --max-old-space-size when running npm? npm/npm#12238

Open

kunagpal mentioned this pull request Apr 30, 2018

JavaScript heap out of memory postmanlabs/newman#1516

Closed

src: whitelist v8 options with '_' or '-' by sam-github · Pull Request #14093 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: whitelist v8 options with '_' or '-' - #14093

src: whitelist v8 options with '_' or '-'#14093sam-github wants to merge 0 commit intonodejs:masternodejs/node:masterfrom sam-github:allow-v8-options-with-underscoresam-github/node:allow-v8-options-with-underscoreCopy head branch name to clipboard

Conversation

sam-github commented Jul 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Jul 5, 2017

Uh oh!

richardlau Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jul 6, 2017

Uh oh!

refack commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jul 6, 2017

Uh oh!

sam-github commented Jul 7, 2017

Uh oh!

sam-github commented Jul 7, 2017

Uh oh!

vsemozhetbyt commented Jul 10, 2017

Uh oh!

gibfahn commented Nov 13, 2017

Uh oh!

MylesBorins commented Nov 13, 2017

Uh oh!

gibfahn commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Nov 14, 2017 via email

Uh oh!

MylesBorins commented Nov 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: whitelist v8 options with '_' or '-'#14093
sam-github wants to merge 0 commit into
nodejs:masterfrom
sam-github:allow-v8-options-with-underscore

refack commented Jul 6, 2017 •
edited
Loading

sam-github commented Jul 6, 2017 •
edited
Loading

refack left a comment •
edited
Loading

gibfahn commented Nov 14, 2017 •
edited
Loading