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

cmd: support dash as stdin alias #13012

Closed
wants to merge 1 commit into
base: master
from

Conversation

@ebraminio
Member

ebraminio commented May 13, 2017

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
    (some of the tests are failing regardless of my change AFAICS so I assume this as passed)
  • tests and/or benchmarks are included
    (an available test is modified to cover this, and a more specific one also added)
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

cli

Dash as stdin alias is a usual convention between unix programs https://unix.stackexchange.com/a/16364 and I believe it should be available on nodejs also.

Here I've made a change that make node ignore dash as an option so it will effectively be treated like /dev/stdin

Simply, with my change this will be made possible with node:

echo "console.log(process.argv[2])" | node - --option-for-stdin-script

The reason I like to have this is because of my one-file webserver http://pad.js.org which in order to be able to pass extra option to its one-linear execution command on the fly (without having to download or install the script first, which it supports that also through npm and docker). BTW, have a look at that project also, I guess you will find it interesting, specially the way its source is distributed and several features it offers :)

Interestingly, I saw during my tests if I pass "/std/stdin" instead "-" on the above command, even without my change, it will be ran on macOS just fine but for some reasons it seems it doesn't work on Linux equally which perhaps should be filed as a bug separately (Edited: proposed another patch for that). However with this change, "-" will provide a cross platform solution for achieving this goal.

For my specific case, this patch will make this functionality possible:

curl pad.js.org | node - -h

which as mentioned before if you have access to macOS, this is already possible with the following command, but not on Linux and obviously ever Windows:

curl pad.js.org | node /dev/stdin -h

Thanks.

@refack

refack approved these changes May 13, 2017

setTimeout(function() {
child.stdin.end('console.log(process.pid)');
}, 1);
}

This comment has been minimized.

@refack

refack May 13, 2017

Member

Nit: We like an \n on the last line

This comment has been minimized.

@ebraminio

ebraminio May 13, 2017

Member

Done for both

test/parallel/test-stdin-script-child.js Outdated
let found = '';
for (const args of [[], ['-']]) {
const child = spawn(process.execPath, args, {
env: Object.assign(process.env, {

This comment has been minimized.

@refack

refack May 13, 2017

Member

Don't you want the arguments to assign the other way around?

This comment has been minimized.

@addaleax

addaleax May 13, 2017

Member

This is just copied… is this an existing bug? :/

This comment has been minimized.

@ebraminio

ebraminio May 13, 2017

Member

This is just copied from the old version, I guess better to not touch it now.

This comment has been minimized.

@refack

refack May 13, 2017

Member

🤔 this is a super weird construct. IMHO If it's to suss out a bug it should be near https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-spawn-shell.js#L51
Personaly I'd rather see this fixed here, but I won't block for it.

@@ -123,7 +123,7 @@
const internalModule = NativeModule.require('internal/module');
internalModule.addBuiltinLibsToObject(global);
evalScript('[eval]');
} else if (process.argv[1]) {
} else if (process.argv[1] && process.argv[1] !== '-') {

This comment has been minimized.

@refack

refack May 13, 2017

Member

IMHO -- is much more common alias for "the rest will be piped through stdin". If it's not too much work I'd be happy if you adjust.
So you could do echo "console.log(process.argv[2])" | node -h --

This comment has been minimized.

@refack

refack May 13, 2017

Member

Got you -h is an arg to the script. Not to node.
Retracting.

This comment has been minimized.

@addaleax

addaleax May 13, 2017

Member

We already use -- for separating arguments, and that’s what it’s commonly used for (on Unixes, at least). - is the most common choice for “stdin”, by far.

@refack

Last comment is a real nit.

src/node.cc Outdated
while (index < nargs && argv[index][0] == '-' && !short_circuit) {
while (index < nargs &&
// starts with dash but is not only a dash which is special
argv[index][0] == '-' && strlen(argv[index]) != 1 &&

This comment has been minimized.

@addaleax

addaleax May 13, 2017

Member

you could replace the strlen check with argv[index][1] == '\0'

test/parallel/test-stdin-script-child.js Outdated
let found = '';
for (const args of [[], ['-']]) {
const child = spawn(process.execPath, args, {
env: Object.assign(process.env, {

This comment has been minimized.

@addaleax

addaleax May 13, 2017

Member

This is just copied… is this an existing bug? :/

@refack

This comment has been minimized.

Member

refack commented May 13, 2017

@ebraminio

This comment has been minimized.

Member

ebraminio commented May 13, 2017

I think I've fixed these

@refack

refack approved these changes May 13, 2017

@refack

This comment has been minimized.

Member

refack commented May 13, 2017

@ebraminio

This comment has been minimized.

Member

ebraminio commented May 13, 2017

Thank you :) Is linter giving a true found error? I couldn't find anything useful from its console info.

test/parallel/test-stdin-script-child.js Outdated
setTimeout(function() {
child.stdin.end('console.log(process.pid)');
}, 1);
}

This comment has been minimized.

@refack

refack May 13, 2017

Member

Same here just one blank line, i.e. file should end }\n

This comment has been minimized.

@ebraminio

ebraminio May 13, 2017

Member

Super weird, I really didn't want to put two... I guess done now

@ebraminio

This comment has been minimized.

Member

ebraminio commented May 13, 2017

Great, it seems CI don't dislike this anymore

@refack

This comment has been minimized.

Member

refack commented May 13, 2017

@refack

refack approved these changes May 13, 2017

@ebraminio

This comment has been minimized.

Member

ebraminio commented May 13, 2017

@refack refack added the semver-minor label May 13, 2017

@refack refack self-assigned this May 13, 2017

@refack

This comment has been minimized.

Member

refack commented May 13, 2017

I've retriggered linux-fips: https://ci.nodejs.org/job/node-test-commit-linux-fips/8382/
But even if it borks again, consider the CI green. Now we wait for this to "mature"...

I'm assuming this is your first contribution so I've put down a little explanation. If it's not ignore the rest 😉

Our "landing" policy is to wait 2 days (3-4 if it's weekend) so that stakeholders can chime in. Since I've marked this as semver-minor there shouldn't be any extra steps. If someone else judges this as semver-major then we need two members of the CTC to sign off on the change.

Good luck, and thank you very much for the contribution 🥇

@ebraminio

This comment has been minimized.

Member

ebraminio commented May 14, 2017

np, I just wanted to know if my part is complete :)

src/node.cc Outdated
@@ -3780,7 +3780,10 @@ static void ParseArgs(int* argc,
unsigned int index = 1;
bool short_circuit = false;
while (index < nargs && argv[index][0] == '-' && !short_circuit) {
while (index < nargs &&
// starts with dash but is not only a dash which is special

This comment has been minimized.

@addaleax

addaleax May 14, 2017

Member

I’d drop this comment, or change it to something like // skip "-", because currently it described how it does stuff, not what it does, so it’s directly redundant to the code.

test/parallel/test-stdin-script-child-option.js Outdated
child.stdin.end('console.log(process.argv[2])');
let actual = '';
child.stdout.on('data', (chunk) => actual += chunk);

This comment has been minimized.

@addaleax

addaleax May 14, 2017

Member

You should call child.stdout.setEncoding('utf8'); before using .on('data') with strings

@ebraminio

This comment has been minimized.

Member

ebraminio commented May 14, 2017

Done

@ebraminio ebraminio referenced this pull request May 14, 2017

Closed

fs: fix realpath{Sync} on resolving pipes/sockets #13028

4 of 4 tasks complete

@jasnell jasnell requested a review from bnoordhuis May 15, 2017

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 15, 2017

Seems reasonable.

@refack

Now we have positive momentum, we need documentation.

@refack

This comment has been minimized.

Member

refack commented May 16, 2017

Documentation locations:

  1. node/doc/api/cli.md
  2. https://github.com/nodejs/node/blob/master/src/node.cc#L3600
@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 16, 2017

Please also add documentation in doc/node.1.

In addition, we may want to update the "synopsis" in all places to change [script.js | -e "script"] to [script.js | -e "script" | - ].

@Fishrock123

LGTM other than the man formatting issue

doc/node.1 Outdated
@@ -36,7 +36,8 @@ node \- Server-side JavaScript runtime
.RI [ v8\ options ]
.RI [ script.js \ |
.B -e
.RI \&" script \&"]
.RI \&" script \&" \ |

This comment has been minimized.

@Fishrock123

Fishrock123 May 17, 2017

Member

Split the \ | to the line below and then change the line below to .RB?

This comment has been minimized.

@ebraminio

ebraminio May 17, 2017

Member

Done I guess, have a look again

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 17, 2017

.RI \&" script \&"
.R |
.B -
.R ]

This comment has been minimized.

@Fishrock123

Fishrock123 May 17, 2017

Member

This can be condensed into one line as .RB \| - ] I think. I forget how exactly the escapes work for the | character.

This comment has been minimized.

@ebraminio

ebraminio May 17, 2017

Member

Something like .RB |\ -\ ] worked but no bolding of dash, is that OK?

This comment has been minimized.

@ebraminio

ebraminio May 17, 2017

Member

Never had experience of doc writing using troff before, will apply whatever you conclude of course.

This comment has been minimized.

@Fishrock123

Fishrock123 May 17, 2017

Member

Just took a look and this does appear to be correct.

@refack

refack approved these changes May 17, 2017

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 18, 2017

@bnoordhuis Any thoughts on this? @jasnell is there a reason why you wanted Ben to look specifically?

@Fishrock123 Fishrock123 self-assigned this May 18, 2017

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 19, 2017

Going to land this at the end of today, then.

@@ -3598,6 +3598,8 @@ static void PrintHelp() {
" does not appear to be a terminal\n"
" -r, --require module to preload (option can be "
"repeated)\n"
" - script read from stdin (default; "
"interactive mode if a tty)"

This comment has been minimized.

@ebraminio

ebraminio May 20, 2017

Member

Just added this line from the python -h, please review it before the merge.

This comment has been minimized.

@refack

refack May 20, 2017

Member

You mean node -h? ahh "from"

This comment has been minimized.

@ebraminio

ebraminio May 20, 2017

Member

I meant I've copied this literally from the similar place on python -h :)

@@ -10,7 +10,7 @@ To view this documentation as a manual page in a terminal, run `man node`.
## Synopsis
`node [options] [v8 options] [script.js | -e "script"] [--] [arguments]`
`node [options] [v8 options] [script.js | -e "script" | -] [--] [arguments]`

This comment has been minimized.

@ebraminio

ebraminio May 20, 2017

Member

Removed space after the dash as its section start also is not space padded.

@ebraminio ebraminio referenced this pull request May 20, 2017

Closed

cli: accept /dev/stdin as input file on Linux #13130

3 of 3 tasks complete

@refack refack removed their assignment May 20, 2017

test/parallel/test-stdin-script-child.js Outdated
child.on('close', common.mustCall(function(c) {
assert.strictEqual(c, 0);
assert.strictEqual(found, wanted);
console.log('ok');

This comment has been minimized.

@jasnell

jasnell May 22, 2017

Member

Is the console output necessary to the test? If not, please remove :-)

This comment has been minimized.

@ebraminio

ebraminio May 22, 2017

Member

Done

@refack

This comment has been minimized.

Member

refack commented May 23, 2017

Going to land this at the end of today, then.

@Fishrock123 are you still planning on landing this?

@addaleax

This comment has been minimized.

Member

addaleax commented May 23, 2017

Landed in 594b5d7, thanks for the PR!

@addaleax addaleax closed this May 23, 2017

addaleax added a commit that referenced this pull request May 23, 2017

cmd: support dash as stdin alias
PR-URL: #13012
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 23, 2017

@refack ok not really, go ahead and land it please

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 23, 2017

huh, guess github didn't update...

Thanks @addaleax 😬 😓

jasnell added a commit that referenced this pull request May 24, 2017

cmd: support dash as stdin alias
PR-URL: #13012
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

jasnell added a commit that referenced this pull request May 28, 2017

cmd: support dash as stdin alias
PR-URL: #13012
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn

This comment has been minimized.

Member

gibfahn commented Jan 15, 2018

Release team were +1 on backporting to v6.x if someone would be willing to raise a backport PR, see the guide for more info.

@silverwind

This comment has been minimized.

Contributor

silverwind commented Jan 29, 2018

If this gets backported, #18440 should probably also be applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment