Skip to content
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.

Sign up for GitHub

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ To view this documentation as a manual page in your terminal, run `man node`.

## Synopsis

`node [options] [v8 options] [script.js | -e "script"] [arguments]`
`node [options] [v8 options] [script.js | -e "script"] [--] [arguments]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also update the man page at doc/node.1 with this change.


`node debug [script.js | -e "script" | <host>:<port>] …`

Expand Down Expand Up @@ -265,6 +265,15 @@ added: v0.11.15

Specify ICU data load path. (overrides `NODE_ICU_DATA`)

### `--`
<!-- YAML
added: REPLACEME
-->

Indicate the end of node options. Pass the rest of the arguments to the script.
If no script filename or eval/print script is supplied prior to this, then
the next argument will be used as a script filename.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also add this to the man page.

## Environment Variables

### `NODE_DEBUG=module[,…]`
Expand Down
8 changes: 8 additions & 0 deletions doc/node.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ node \- Server-side JavaScript runtime
.RI [ script.js \ |
.B -e
.RI \&" script \&"]
.B [--]
.RI [ arguments ]
.br
.B node debug
Expand Down Expand Up @@ -184,6 +185,13 @@ used to enable FIPS-compliant crypto if Node.js is built with
.BR \-\-icu\-data\-dir =\fIfile\fR
Specify ICU data load path. (overrides \fBNODE_ICU_DATA\fR)

.TP
.BR \-\-\fR
Indicate the end of node options. Pass the rest of the arguments to the script.

If no script filename or eval/print script is supplied prior to this, then
the next argument will be used as a script filename.

.SH ENVIRONMENT VARIABLES

.TP
Expand Down
3 changes: 3 additions & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3697,6 +3697,9 @@ static void ParseArgs(int* argc,
} else if (strcmp(arg, "--expose-internals") == 0 ||
strcmp(arg, "--expose_internals") == 0) {
// consumed in js
} else if (strcmp(arg, "--") == 0) {
index += 1;
break;
} else {
// V8 option. Pass through as-is.
new_v8_argv[new_v8_argc] = arg;
Expand Down
40 changes: 40 additions & 0 deletions test/parallel/test-cli-eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ const child = require('child_process');
const path = require('path');
const nodejs = `"${process.execPath}"`;

if (process.argv.length > 2) {
console.log(process.argv.slice(2).join(' '));
process.exit(0);
}

// Assert that nothing is written to stdout.
child.exec(`${nodejs} --eval 42`, common.mustCall((err, stdout, stderr) => {
assert.ifError(err);
Expand Down Expand Up @@ -133,3 +138,38 @@ child.exec(`${nodejs} --use-strict -p process.execArgv`,
assert.strictEqual(proc.stderr, '');
assert.strictEqual(proc.stdout, 'start\nbeforeExit\nexit\n');
}

[ '-arg1',
'-arg1 arg2 --arg3',
'--',
'arg1 -- arg2',
].forEach(function(args) {

// Ensure that arguments are successfully passed to eval.
const opt = ' --eval "console.log(process.argv.slice(1).join(\' \'))"';
const cmd = `${nodejs}${opt} -- ${args}`;
child.exec(cmd, common.mustCall(function(err, stdout, stderr) {
assert.strictEqual(stdout, args + '\n');
assert.strictEqual(stderr, '');
assert.strictEqual(err, null);
}));

Copy link
Member

Choose a reason for hiding this comment

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

Can you drop the blank line here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// Ensure that arguments are successfully passed to print.
const popt = ' --print "process.argv.slice(1).join(\' \')"';
const pcmd = `${nodejs}${popt} -- ${args}`;
child.exec(pcmd, common.mustCall(function(err, stdout, stderr) {
assert.strictEqual(stdout, args + '\n');
assert.strictEqual(stderr, '');
assert.strictEqual(err, null);
}));

// Ensure that arguments are successfully passed to a script.
// The first argument after '--' should be interpreted as a script
// filename.
const filecmd = `${nodejs} -- ${__filename} ${args}`;
child.exec(filecmd, common.mustCall(function(err, stdout, stderr) {
assert.strictEqual(stdout, args + '\n');
assert.strictEqual(stderr, '');
assert.strictEqual(err, null);
}));
});
Copy link
Member

@jasnell jasnell Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jBarz jBarz Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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