Use spawn instead of execFile #27

jamestalmage · 2016-04-26T08:41:48Z

childProcess.execFile pays no attention opts.stdio. This means users can't set stdio options when calling exca(). You can set stdio options when calling execa.spawn(), but that does not return a promise.

Rather than having two separate methods execa() and execa.spawn() - I think we should work towards a single method that allows you to configure how it behaves via stdio options. opts.stdio or opts.stdin, etc; Those should determine whether or not the stream is buffered and made part of the promise result, or if they are just streamed (or sent to an Observable), or ignored.

To that end, I've reimplemented much of the functionality of child_process.execFile. execFile is really just a wrapper around spawn anyways. It's main utility is that in concats stdio and stdout. Which is pretty easy for us to achieve on our own using get-stream.

This simplifies the API as we can pretty much deprecate execa.spawn(). And it offers more control over the behavior as well. Want to use streams/observables for stdout because you know it will contain lots of data and you don't want to waste memory buffering? Would it be nice if it still buffered stderr for you though? This makes that doable.

Remaining items to be feature complete with execFile:

timeout support (should be easy to add back).
errname resolution. See TODO note on that. I'm not sure we want to use process.bind('uv'). I don't really know what it does.
some stream cleanup code (see childProces.execFile's internal kill method. It calls stream.destroy(). I am not sure how that will affect get-stream)
add a bunch of tests around the logic copied out of child_process. The new code has nearly 100% coverage, but that's deceptive. Most of these aren't terribly consequential (Error properties: message, killed, code, etc).

Going forward, we'll still need to bikeshed on a few API details, like instead of just ignore, pipe and inherit, we should add buffer and observable.

childProcess.execFile pays no attention `opts.stdio`. `execa.spawn` allows you to set `opts.stdio` options, but it doesn't return a promise. I think we should forgoe using `childProcess.execFile` and just use spawn. We can reimplement much of `execFile`s utility ourself. Rather than having two separate methods `spawn` and `execFile` - I think we should work towards a single method. What options the user sets for `opts.stdio` or `opts.stdin`, etc; Those should determine whether or not the stream is buffered and made part of the promise result, or if they are just streamed (or sent to an Observable), or ignored. This simplifies the API, and offers potentially far more control. Want to use streams/observables for `stdout` because you know it will contain lots of data and you don't want to buffer, but it would nice to just have `stderr` buffered for you to print in case of an error? That would be doable this way, and really separates it from child_process. childProcess.execFile is really just a wrapper around spawn anyways. It's main utility is that in concats `stdio` and `stdout`. Which is pretty easy for us to achieve on our own using `get-stream` This reimplements nearly all of the functionality of execFile. Remaining items to be feature complete with execFile: - [ ] timeout support (should be easy to add back). - [ ] errname resolution. See TODO note on that. I'm not sure we want to use `process.bind('uv')`. I don't really know what it does. - [ ] some stream cleanup code (see childProces.execFile's internal `kill` method. It calls `stream.destroy()`. I am not sure how that will affect `get-stream`)

sindresorhus · 2016-04-26T08:48:19Z

Very cool! I like this a lot. Will give a more thorough review and response later today.

This will also make it easier to achieve #5.

jamestalmage · 2016-04-26T10:50:39Z

Can't figure out why Windows is failing. If someone else could give troubleshooting it a go, it would be appreciated

jamestalmage · 2016-04-26T11:40:25Z

Using the technique described here, I'm getting the following output:

This Branch(failing):

Z:\WebstormProjects\execa>node node_modules/ava/cli.js
spawn called
[ 'C:\\Windows\\system32\\cmd.exe',
  [ '/s',
    '/c',
    '"node "Z:\\WebstormProjects\\execa\\fixtures\\error-message.js""' ],
  { maxBuffer: 10485760,
    stripEof: true,
    preferLocal: true,
    encoding: 'utf8',
    windowsVerbatimArguments: true,
    env: { Path: 'Z:\\WebstormProjects\\execa\\node_modules\\.bin;Z:\\WebstormPr
ojects\\node_modules\\.bin;Z:\\node_modules\\.bin;C:\\Program Files\\nodejs;Z:\\
WebstormProjects\\execa\\fixtures;C:\\Program Files\\Parallels\\Parallels Tools\
\Applications;C:\\Windows\\system32;C:\\Windows;C:\\Windows\\System32\\Wbem;C:\\
Windows\\System32\\WindowsPowerShell\\v1.0\\;c:\\Program Files\\Microsoft SQL Se
rver\\100\\Tools\\Binn\\;c:\\Program Files\\Microsoft SQL Server\\100\\DTS\\Binn
\\;C:\\Program Files\\Microsoft SQL Server\\110\\Tools\\Binn\\;C:\\Users\\Admini
strator\\AppData\\Roaming\\nvm;C:\\Program Files\\nodejs;C:\\Program Files\\Git\
\cmd;C:\\Program Files\\OpenVPN\\bin;C:\\Users\\Administrator\\AppData\\Roaming\
\npm;C:\\Users\\Administrator\\AppData\\Roaming\\nvm;C:\\Program Files\\nodejs'
} } ]

   1 failed

  1. include stdout in errors for improved debugging

  t.regex(err.message, /stdout/)
              |
              "spawn undefined ENOENT"

      _callee6$ (test.js:52:4)

Z:\WebstormProjects\execa>

Master (passing)

Z:\WebstormProjects\execa>node node_modules/ava/cli.js
execFile called
[ 'C:\\Windows\\system32\\cmd.exe',
  [ '/s',
    '/c',
    '"node "Z:\\WebstormProjects\\execa\\fixtures\\error-message.js""' ],
  { maxBuffer: 10485760,
    stripEof: true,
    preferLocal: true,
    encoding: 'utf8',
    windowsVerbatimArguments: true,
    env: { Path: 'Z:\\WebstormProjects\\execa\\node_modules\\.bin;Z:\\WebstormPr
ojects\\node_modules\\.bin;Z:\\node_modules\\.bin;C:\\Program Files\\nodejs;Z:\\
WebstormProjects\\execa\\fixtures;C:\\Program Files\\Parallels\\Parallels Tools\
\Applications;C:\\Windows\\system32;C:\\Windows;C:\\Windows\\System32\\Wbem;C:\\
Windows\\System32\\WindowsPowerShell\\v1.0\\;c:\\Program Files\\Microsoft SQL Se
rver\\100\\Tools\\Binn\\;c:\\Program Files\\Microsoft SQL Server\\100\\DTS\\Binn
\\;C:\\Program Files\\Microsoft SQL Server\\110\\Tools\\Binn\\;C:\\Users\\Admini
strator\\AppData\\Roaming\\nvm;C:\\Program Files\\nodejs;C:\\Program Files\\Git\
\cmd;C:\\Program Files\\OpenVPN\\bin;C:\\Users\\Administrator\\AppData\\Roaming\
\npm;C:\\Users\\Administrator\\AppData\\Roaming\\nvm;C:\\Program Files\\nodejs'
} } ]

   1 passed

Z:\WebstormProjects\execa>

jamestalmage · 2016-04-26T12:02:51Z

I just... I can't... I don't get it. 49065ca

Why does this make the tests pass? Could you possibly make the error output any more cryptic? I don't understand you Windows! We are not friends anymore.

jamestalmage · 2016-04-26T12:37:48Z

Previous commit did not trigger travis. Force pushed another.

jamestalmage · 2016-04-27T02:51:15Z

@sindresorhus - How do you want to proceed with this?

Do you want me to keep pushing commits until it's feature complete with execFile? (the only thing of real consequence is timeout support - which I don't think is widely used).

My only concern is that it's already a pretty major refactor, and I would like to do it in smaller chunks if possible. We can convert my check list from the OP to issues, and just work to fix them before the next release.

sindresorhus · 2016-04-27T08:35:56Z

I just... I can't... I don't get it. 49065ca

Super-weird... I tried to debug this on my Windows VM, but still no idea why it happen. Fwiw it doesn't happen with childProcess.spawn().

sindresorhus · 2016-04-27T08:41:51Z

index.js

-
-	return spawned;
-};
+module.exports.spawn = util.deprecate(module.exports, 'execa.spawn: just use execa instead.');


execa.spawn() is deprecated. Use execa() instead.

sindresorhus · 2016-04-27T08:47:54Z

This looks really good. Let's merge now and open issues for what's remaining. I too prefer work done in smaller chunks. And you've already covered to important parts. Can you update the readme, removing spawn and any other changes?

kevva · 2016-04-27T08:51:06Z

The tests pass on Windows for me (even with process.exit(1)). I'm not running Windows on a VM though but as a real OS.

sindresorhus · 2016-04-27T08:52:15Z

@kevva What Windows & Node.js version?

kevva · 2016-04-27T08:53:41Z

@sindresorhus, never mind. Ran it in bash. Trying to run it in cmd gave me spawn undefined ENOENT too.

jamestalmage · 2016-04-27T17:51:45Z

Should we open an issue on the Node repo about the weird process.exit behavior? I haven't tried creating a minimal reproduction yet. That would be a first step.

sindresorhus · 2016-04-27T18:01:43Z

Definitely, if you can recreate it. Sounds like a horrible bug if valid.

kevva · 2016-04-27T19:18:31Z

I did create a simple script exiting with error code 1 and got ENOENT too. Same result using the core childProcess.spawn. Feels like a bug.

jamestalmage · 2016-04-27T20:22:58Z

@kevva - Can you share the script. I'm not able to reproduce with a simple spawn.

sindresorhus · 2016-04-29T05:54:24Z

🙌 🎉

kevva · 2016-04-29T07:35:53Z

👏

derimagia · 2016-06-12T08:02:48Z

Make sure when you add links to lines on github you press y to get the current commit -

execa/index.js

Line 160 in 56a20b1

    
           // https://github.com/nodejs/node/blob/master/lib/child_process.js#L203

is outdated at the moment.

Fixes sindresorhus#34 - Adds some tests for functionality copied from `child_process` in sindresorhus#27. - Normalises the result and error objects further. Each now has `killed`, `signal` and `cmd` properties. - NYC was not ignoring the `fixtures` directory. Fixed that, and configured nyc in package.json instead of via CLI.

sindresorhus · 2016-08-12T22:55:27Z

@jamestalmage One thing we didn't consider here is that if the user does execa('echo', ['unicorns']).stdout.pipe(process.stdout);, to use it like they used child_process.spawn, it will still buffer the result, waste memory, and eventually hit the max buffer. We should probably have a buffer: false option or something, right?

jamestalmage · 2016-08-13T01:20:20Z

Yeah. Or we could automatically discard the buffer if they pipe.

sindresorhus · 2016-08-13T01:26:49Z

Was thinking about that too, but can we detect that?

jamestalmage added 2 commits April 26, 2016 05:09

use regex test so I can see what went wrong on Windows

467268c

remove leading ./ - If this fixes windows, we should figure out why.

9b8e1c9

Why? Why? Why Windows???

49065ca

Why does this make the tests pass? Could you possibly make the error output any more cryptic? I don't understand you Windows! We are not friends anymore.

jamestalmage force-pushed the just-use-spawn branch from dcb00dc to 49065ca Compare April 26, 2016 12:36

jamestalmage added 3 commits April 26, 2016 22:25

deprecate execa.spawn

9b1c4f4

tweaks

3a461e2

more tweaks

04ca84b

sindresorhus reviewed Apr 27, 2016
View reviewed changes

sindresorhus changed the title ~~[WIP]: just use spawn instead of execFile~~ Use spawn instead of execFile Apr 27, 2016

jamestalmage mentioned this pull request Apr 28, 2016

Add a promise-getter to spawned child process objects #29

Closed

update deprecation message

f54257e

jamestalmage merged commit 31a8d87 into sindresorhus:master Apr 28, 2016

jamestalmage deleted the just-use-spawn branch April 28, 2016 22:04

jamestalmage mentioned this pull request Apr 28, 2016

Timeout option. #30

Closed

This was referenced Apr 28, 2016

re-implement errname resolution #31

Closed

reimplement stream cleanup procedures from execFile #32

Closed

Add some tests around the logic copied out of execFile #34

Closed

Hunt down weird Windows bug. #35

Closed

[WIP] stdio option handling #36

Closed

sindresorhus mentioned this pull request Jun 29, 2016

Release? #46

Closed

jamestalmage mentioned this pull request Jul 15, 2016

add tests #48

Merged

jamestalmage mentioned this pull request Jan 2, 2017

Reimplement missing features. Fixes #30 & #32 #68

Merged

okonet mentioned this pull request Jan 20, 2017

git: 'Files\Git\mingw64\libexec\git-core\git.EXE' is not a git command. See 'git --help'. lint-staged/lint-staged#114

Closed

SamVerschueren mentioned this pull request Mar 20, 2017

Fix test for exit code 1 on windows #78

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use spawn instead of execFile #27

Use spawn instead of execFile #27

jamestalmage commented Apr 26, 2016 •

edited

Loading

sindresorhus commented Apr 26, 2016 •

edited

Loading

jamestalmage commented Apr 26, 2016

jamestalmage commented Apr 26, 2016

jamestalmage commented Apr 26, 2016 •

edited

Loading

jamestalmage commented Apr 26, 2016

jamestalmage commented Apr 27, 2016

sindresorhus commented Apr 27, 2016 •

edited

Loading

sindresorhus Apr 27, 2016

sindresorhus commented Apr 27, 2016

kevva commented Apr 27, 2016 •

edited

Loading

sindresorhus commented Apr 27, 2016

kevva commented Apr 27, 2016

jamestalmage commented Apr 27, 2016

sindresorhus commented Apr 27, 2016

kevva commented Apr 27, 2016

jamestalmage commented Apr 27, 2016

sindresorhus commented Apr 29, 2016

kevva commented Apr 29, 2016

derimagia commented Jun 12, 2016

sindresorhus commented Aug 12, 2016

jamestalmage commented Aug 13, 2016

sindresorhus commented Aug 13, 2016

Use spawn instead of execFile #27

Use spawn instead of execFile #27

Conversation

jamestalmage commented Apr 26, 2016 • edited Loading

sindresorhus commented Apr 26, 2016 • edited Loading

jamestalmage commented Apr 26, 2016

jamestalmage commented Apr 26, 2016

jamestalmage commented Apr 26, 2016 • edited Loading

jamestalmage commented Apr 26, 2016

jamestalmage commented Apr 27, 2016

sindresorhus commented Apr 27, 2016 • edited Loading

sindresorhus Apr 27, 2016

Choose a reason for hiding this comment

sindresorhus commented Apr 27, 2016

kevva commented Apr 27, 2016 • edited Loading

sindresorhus commented Apr 27, 2016

kevva commented Apr 27, 2016

jamestalmage commented Apr 27, 2016

sindresorhus commented Apr 27, 2016

kevva commented Apr 27, 2016

jamestalmage commented Apr 27, 2016

sindresorhus commented Apr 29, 2016

kevva commented Apr 29, 2016

derimagia commented Jun 12, 2016

sindresorhus commented Aug 12, 2016

jamestalmage commented Aug 13, 2016

sindresorhus commented Aug 13, 2016

jamestalmage commented Apr 26, 2016 •

edited

Loading

sindresorhus commented Apr 26, 2016 •

edited

Loading

jamestalmage commented Apr 26, 2016 •

edited

Loading

sindresorhus commented Apr 27, 2016 •

edited

Loading

kevva commented Apr 27, 2016 •

edited

Loading