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.

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

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

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@sam-github
Member

sam-github commented Jul 5, 2017

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

@sam-github sam-github requested a review from richardlau Jul 5, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Jul 5, 2017

Member

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

refack commented Jul 5, 2017

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...
Show outdated Hide outdated src/node.cc
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jul 6, 2017

Member

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.

Member

sam-github commented Jul 6, 2017

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.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Jul 6, 2017

Member

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.

  1. 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.
  2. 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.
Member

refack commented Jul 6, 2017

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.

  1. 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.
  2. 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.
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jul 6, 2017

Member
  1. 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!
  2. 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
Member

sam-github commented Jul 6, 2017

  1. 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!
  2. 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
@refack

refack approved these changes Jul 6, 2017 edited

Convinced

Show outdated Hide outdated test/parallel/test-cli-node-options.js
Show outdated Hide outdated src/node.cc
Show outdated Hide outdated test/parallel/test-cli-node-options.js
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jul 7, 2017

Member

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

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

Member

sam-github commented Jul 7, 2017

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

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

sam-github added a commit that referenced this pull request Jul 10, 2017

src: whitelist v8 options with '_' or '-'
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>

@sam-github sam-github closed this Jul 10, 2017

@sam-github sam-github deleted the sam-github:allow-v8-options-with-underscore branch Jul 10, 2017

@vsemozhetbyt

This comment has been minimized.

Show comment
Hide comment
@vsemozhetbyt

vsemozhetbyt Jul 10, 2017

Member

Landed in 97a6aa9

Member

vsemozhetbyt commented Jul 10, 2017

Landed in 97a6aa9

addaleax added a commit that referenced this pull request Jul 11, 2017

src: whitelist v8 options with '_' or '-'
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>

@addaleax addaleax referenced this pull request Jul 11, 2017

Merged

v8.2.0 proposal #13744

addaleax added a commit that referenced this pull request Jul 18, 2017

src: whitelist v8 options with '_' or '-'
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>

Fishrock123 added a commit that referenced this pull request Jul 19, 2017

src: whitelist v8 options with '_' or '-'
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>
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Nov 13, 2017

Member

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

Member

gibfahn commented Nov 13, 2017

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

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 13, 2017

Member

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

Member

MylesBorins commented Nov 13, 2017

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

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Nov 14, 2017

Member

@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?

Member

gibfahn commented Nov 14, 2017

@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?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 14, 2017

Member
Member

MylesBorins commented Nov 14, 2017

MylesBorins added a commit that referenced this pull request Nov 14, 2017

src: whitelist v8 options with '_' or '-'
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>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 14, 2017

Member

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

Member

MylesBorins commented Nov 14, 2017

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

@MylesBorins MylesBorins referenced this pull request Nov 21, 2017

Merged

v6.12.1 proposal #17180

MylesBorins added a commit that referenced this pull request Nov 21, 2017

src: whitelist v8 options with '_' or '-'
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>

MylesBorins added a commit that referenced this pull request Nov 28, 2017

src: whitelist v8 options with '_' or '-'
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>

MylesBorins added a commit that referenced this pull request Nov 28, 2017

2017-12-05, Version 6.12.1 'Boron' (LTS)
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

MylesBorins added a commit that referenced this pull request Dec 5, 2017

2017-12-05, Version 6.12.1 'Boron' (LTS)
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

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

2017-12-05, Version 6.12.1 'Boron' (LTS)
Notable Changes:

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

PR-URL: nodejs#17180

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

2017-12-05, Version 6.12.1 'Boron' (LTS)
Notable Changes:

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

PR-URL: nodejs#17180
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment