"--" after "-e <script>" means end-of-options #10651

jBarz · 2017-01-06T00:20:27Z

There is currently no way to pass the first argument to an eval script that starts with a hyphen.

node -e 'console.log(process.argv)' -arg1
[ '/usr/bin/node' ]

node -e 'console.log(process.argv)' -- -arg1
[ '/usr/bin/node' ]

After this fix
node -e 'console.log(process.argv)' -- -arg1
[ '/usr/bin/node', '-arg1' ]

Checklist

make -j4 test (UNIX)
tests added
commit message follows commit guidelines

Affected core subsystem(s)

addaleax · 2017-01-06T00:50:48Z

test/parallel/test-cli-eval.js

One suggestion: Could you add tests for more arguments after --, too?

cjihrig · 2017-01-06T13:45:50Z

test/parallel/test-cli-eval.js

Can you remove this blank line.

cjihrig · 2017-01-06T13:45:56Z

test/parallel/test-cli-eval.js

And this one.

cjihrig · 2017-01-06T13:47:08Z

test/parallel/test-cli-eval.js

If you do this string concatenation in a separate variable, this can be formatted in a much more readable fashion.

cjihrig · 2017-01-06T13:47:38Z

test/parallel/test-cli-eval.js

Space before and after +. Does make lint complain about this?

Indeed it does :-) Will make sure to run that next time.

cjihrig · 2017-01-06T13:48:39Z

test/parallel/test-cli-eval.js

Can you wrap the callback in common.mustCall(), and add assertions for status (which should probably be renamed) and stderr.

Fishrock123 · 2017-01-06T14:55:22Z

To be clear, node -e 'console.log(process.argv)' -arg1 isn't being fixed because -arg1 would be conceptually passed to node and not the script, right?

jBarz · 2017-01-06T14:56:35Z

Thats right

Fishrock123 · 2017-01-06T14:56:42Z

src/node.cc

Does this patch also work with -p?

yea, The variable "eval_string" is used by both -e and -p

Fishrock123

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

Fishrock123 · 2017-01-06T15:14:20Z

Did... the linter error with no output?

+ gmake lint-ci
./node tools/jslint.js -j 2 -f tap -o test-eslint.tap \
	benchmark lib test tools
gmake: *** [Makefile:708: jslint-ci] Error 1

@jBarz could you trying running make -j4 lint?

jBarz · 2017-01-06T15:22:54Z

Sorry, more linter errors fixed. Passes now

cjihrig · 2017-01-06T15:31:29Z

test/parallel/test-cli-eval.js

What I meant by my earlier comment about readability is that you can do this:

const cmd = `${nodejs}${evalopt} -- ${args}`; child.exec(cmd, common.mustCall(function(err, stdout, stderr) { ... });

It gets rid of that awkward looking indented function.

That is certainly more readable. Will do

jasnell · 2017-01-06T16:53:52Z

While I see the need the use of the -- just seems... hackish... Something like node -e "console.log(process.argv)" -a "-arg1" seems like a less hackish approach but has it's own range of short-comings. There aren't many great options for resolving this, however, so I won't object to using the --.

Fishrock123 · 2017-01-06T20:12:10Z

node -e "console.log(process.argv)" -a "-arg1"

I'm not sure how that is less hackish when it uses the same number of characters and -- is already fairly standard for such uses?

sam-github

One comment: I would update the description to include how things work with an arg of arg2 in addition to what you currently describe with -arg1, to show how you are making things more consistent.

Also, I'm not sure if a non - prefixed arg case already exists in the unit tests, its a bit hard to see from diffs, but if it doesn't exist, it should.

sam-github · 2017-01-09T14:52:23Z

FWIW, the behaviour implemented by this PR is how I would have assumed it worked already.

Is this semver-major, or minor, or patch?

I could call it semver-patch, since I think the existing behaviour doesn't accord with how I would assume node works.

On the other hand, I'm not sure the existing docs cover this behaviour... and @jBarz I just realized this PR doesn't update the docs or the node usage message, shouldn't it?

It would be possible for existing code to rely on the current behaviour, though, making this an incompatible semver-major change... or it could be possible to describe this just as a semver-minor, since now we have the capability of passing - prefixed arguments to -e scripts, which we didn't before.

/cc @gibfahn re: our conversation about what is or is not "API", and thus, an "API change" according to semver.

jasnell · 2017-01-09T16:39:38Z

test/parallel/test-cli-eval.js

Given that this is also supposed to work with -p, including that in the test also would be ideal.

~~Also, given that this is not supposed to work outside of using -p and -e, a test verifying that -- fails when those are not used would be good.~~

What do you mean by "fails" and "not work"? -- should continue to work as it does currently in absence of -p and -e:

% node args.js -- --help [ '/home/sam/.nvm/versions/node/v7.2.0/bin/node', '/home/sam/w/core/node/args.js', '--', '--help' ] % node -- args.js --help [ '/home/sam/.nvm/versions/node/v7.2.0/bin/node', '/home/sam/w/core/node/args.js', '--help' ] % cat args.js console.log(process.argv)

I mean that node -- aargs.js should fail.

Except that this is a bug:

core/node (master $% u=) % node -- --help Usage: node [options] [ -e script | script.js ] [arguments] node debug script.js [arguments] ...

The -- should have stopped the options processing, I would have expected to see something like

core/node (master $% u=) % node -- nexist module.js:472 throw err; ^ Error: Cannot find module '/home/sam/w/core/node/nexist'

Except with a file name of --help!

Is it out of scope to fix this bug, too, in this PR @jBarz? Should I report this as a bug?

Actually, I take that back, you're correct. Using the -- shouldn't fail. I was misreading the PR ;-)

I would still like to see the tests expanded to cover the -p option.

Before this PR:
"--" is treated as an unknown node option. Hence it is always passed on to v8.

After this PR:
"--" is treated as the end-of-options delimiter if and only if "--eval script" has been used prior to this option. Otherwise, treat it the same as before

I can change it to do what @sam-github says it must do:
"--" is treated as the end-of-options delimiter for all situations

Since node also uses a script name to indicate end-of-options, I would like to clarify what should happen in this case?
node --nodeopt1 script.js arg1 -- arg2

Should "--" be passed into the script or ignored?

passed in, so that the script.js can use it to indicate the end of its options:

node --some-node-option script.js --some-script-option -- --not-a-script-option

I will commit some additional changes and tests as @jasnell mentioned above. Thanks for the feedback!

bnoordhuis · 2017-01-24T23:20:50Z

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

silverwind · 2017-01-24T23:28:21Z

Thanks! Landed in 0a9f360. I'll set a semver-minor as I guess this qualifies as a feature.

silverwind · 2017-01-24T23:29:38Z

@bnoordhuis sorry, totally missed your post while landing. I assume it will come out green anyways.

addaleax · 2017-01-25T06:11:40Z

I think this can be closed given that it’s landed now

silverwind · 2017-01-25T08:59:42Z

Thanks, forgot to close it. CI came out green 👍

jBarz · 2017-01-25T14:23:28Z

@MylesBorins would it be possible to back-port this to v4.x ?
Edit: nevermind :-) I will open a PR for it myself and make sure it merges cleanly.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v6.x labels Jan 6, 2017

addaleax approved these changes Jan 6, 2017

View reviewed changes

jBarz force-pushed the issue9017 branch from 617de5f to 0290526 Compare January 6, 2017 03:35

nodejs-github-bot added the lts-watch-v6.x label Jan 6, 2017

jBarz force-pushed the issue9017 branch from 0290526 to 25d555f Compare January 6, 2017 03:39

nodejs-github-bot added the lts-watch-v6.x label Jan 6, 2017

mscdex added the process Issues and PRs related to the process subsystem. label Jan 6, 2017

cjihrig reviewed Jan 6, 2017

View reviewed changes

test/parallel/test-cli-eval.js Outdated

Copy link

Contributor

cjihrig Jan 6, 2017

Choose a reason for hiding this comment

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

Can you remove this blank line.

jBarz reacted with thumbs up emoji

cjihrig reviewed Jan 6, 2017

View reviewed changes

test/parallel/test-cli-eval.js Outdated

Copy link

Contributor

cjihrig Jan 6, 2017

Choose a reason for hiding this comment

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

And this one.

jBarz reacted with thumbs up emoji

cjihrig reviewed Jan 6, 2017

View reviewed changes

nodejs-github-bot added the lts-watch-v6.x label Jan 6, 2017

Fishrock123 reviewed Jan 6, 2017

View reviewed changes

Fishrock123 approved these changes Jan 6, 2017

View reviewed changes

nodejs-github-bot added the lts-watch-v6.x label Jan 6, 2017

cjihrig reviewed Jan 6, 2017

View reviewed changes

cjihrig approved these changes Jan 6, 2017

View reviewed changes

addaleax mentioned this pull request Jan 6, 2017

Filenames / dirnames with a leading hyphen: a CLI argument issue #10661

Closed

sam-github approved these changes Jan 9, 2017

View reviewed changes

jasnell requested changes Jan 9, 2017

View reviewed changes

nodejs-github-bot added the lts-watch-v6.x label Jan 10, 2017

jBarz force-pushed the issue9017 branch from 14856cc to a03981d Compare January 24, 2017 23:05

nodejs-github-bot added the dont-land-on-v7.x label Jan 24, 2017

silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 24, 2017

addaleax closed this Jan 25, 2017

gibfahn added lts-watch-v4.x and removed dont-land-on-v7.x labels Jan 25, 2017

jBarz deleted the issue9017 branch January 26, 2017 02:20

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

gibfahn mentioned this pull request Mar 17, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

vsemozhetbyt mentioned this pull request May 30, 2017

blog: add release post for 8.0.0 nodejs/nodejs.org#1241

Closed

MylesBorins added land-on-v6.x and removed lts-watch-v4.x labels Sep 19, 2017

Uh oh!

"--" after "-e <script>" means end-of-options #10651

"--" after "-e <script>" means end-of-options #10651

Uh oh!

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jan 6, 2017

Uh oh!

jBarz commented Jan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jan 6, 2017

Uh oh!

jBarz commented Jan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 6, 2017

Uh oh!

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

Uh oh!

Uh oh!

sam-github 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!

sam-github commented Jan 9, 2017 • edited by gibfahn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell Jan 9, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jBarz Jan 9, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jan 24, 2017

Uh oh!

silverwind commented Jan 24, 2017

jBarz commented Jan 6, 2017 •

edited

Loading

Fishrock123 commented Jan 6, 2017 •

edited

Loading

sam-github left a comment •

edited

Loading

sam-github commented Jan 9, 2017 •

edited by gibfahn

Loading

jasnell Jan 9, 2017 •

edited

Loading

jBarz Jan 9, 2017 •

edited

Loading

jBarz commented Jan 25, 2017 •

edited

Loading