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.

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

what's broken in spawn? #26

Open
bcoe opened this issue Apr 4, 2019 · 12 comments
Open

what's broken in spawn? #26

bcoe opened this issue Apr 4, 2019 · 12 comments

Comments

@bcoe
Copy link
Owner

bcoe commented Apr 4, 2019

@sindresorhus, @satazor in newish versions of Node.js, what issues continue to exist in spawn that would motivate the use of execa or cross-spawn. I wonder if:

  1. some of the historic bugs have been addressed in Node.js.
  2. if they haven't, could they be?

CC: @boneskull

@satazor
Copy link

satazor commented Apr 4, 2019

Hey @bcoe! You may find what issues cross-spawn solves here: https://github.com/moxystudio/node-cross-spawn#why

Also, we explain why options.shell is not an alternative: https://github.com/moxystudio/node-cross-spawn#using-optionsshell-as-an-alternative-to-cross-spawn

@ehmicky
Copy link
Collaborator

ehmicky commented Apr 4, 2019

One big reason for the use of cross-spawn for me is being able to use shabangs on Windows.

@bcoe
Copy link
Owner Author

bcoe commented Apr 4, 2019

I feel like PATHEXT might be fixed, or at least I saw a bug issue on Node.js closed related to it...

@satazor thanks for the list, it's basically a laundry list if what the Node.js tooling working group should be fixing in spawn.

@ehmicky I think we could probably update the documentation to reference @satazor's list, vs., the stale information in that section of the document.

@satazor
Copy link

satazor commented Apr 4, 2019

@bcoe I might be wrong, but the PATHEXT is still an issue. Do you have any link/references?

@bcoe
Copy link
Owner Author

bcoe commented Apr 4, 2019

@satazor we referenced this issue for PATHEXT in the doc, which has since been closed:

nodejs/node-v0.x-archive#2318

@satazor
Copy link

satazor commented Apr 4, 2019

The issue was closed by this comment: nodejs/node-v0.x-archive#2318 (comment). If you go through the comments history, the only suggestion people gave was to use options.shell true but it's not really a solution because of https://github.com/moxystudio/node-cross-spawn#using-optionsshell-as-an-alternative-to-cross-spawn.

@ehmicky
Copy link
Collaborator

ehmicky commented Apr 4, 2019

Also options.shell: true fires the command through a shell instead of directly calling it. This is less secure, slower and less cross-platform: sindresorhus/execa#182

@ehmicky
Copy link
Collaborator

ehmicky commented Apr 4, 2019

I just tried PATHEXT with Node 11.13.0 on Windows 10 and child_process.spawn(). I can confirm it does not work unless options.shell: true is used.

@sindresorhus
Copy link
Contributor

execa lists many of the reasons for its existence: https://github.com/sindresorhus/execa#why (There are however many more things than listed there.)

@ehmicky
Copy link
Collaborator

ehmicky commented Apr 5, 2019

List of cross-platform issues with child_process.spawn() (and related methods):

  1. Shabangs are not supported on Windows. I.e. you need to do spawn('node', ['./node_script.js']) instead of spawn('./node_script.js')
  2. on Unix, foreground child processes (detached: false, the default value) continue running even if their parent exits. On Windows, they don't.
  3. PATHEXT does not work on Windows. I.e. you need to do spawn('./script.bat') instead of spawn('./script') even if PATHEXT includes .bat.
  4. when using shell: true, child_process.stdout includes cmd.exe prompt and command on Windows because Node.js fires cmd.exe without the /q flag.
  5. windowsHide defaults to false where it probably should default to true instead.

Issues 1) and 3) are solved with node-cross-spawn. All of those issues are solved with execa.

Extra information on 3): it can be solved by using a shell interpreter (shell: true option). However this is:

  • less secure: it allows for command injection by passing arguments like $(rm -rf /) or && rm -rf /
  • less cross-platform: it encourages using shell-specific features such as globbing, single quote escaping or even semicolons which won't work on cmd.exe.
  • slower as it goes through an extra step (the shell interpreter)

Just to be complete, execa also brings the following features which are not cross-platform-related:

  • promise interface. Synchronous methods are still available (execa.sync()).
  • can run locally installed binaries.
  • timeout option.
  • can pass stdin as a string or buffer, instead of only as a stream.
  • returns an all property with interleaved stdout and stderr.
  • strip final newline (optionally).
  • has more descriptive error messages (which is very valuable in testing).

Overall I think using execa still makes lots of sense in both production code and in tests, but obviously I am little biased :)

@bcoe
Copy link
Owner Author

bcoe commented Apr 5, 2019

@sindresorhus @ehmicky, I'd advocate that we write-up a proposal here under initiatives, which outlines some of the issues being shimmed by exec alternatives; some of these cross-platform issues (I think) should be fixed in Node.js itself (and this is exactly the goal of the Tooling Working Group).

What do you think @boneskull?

Also, looping in @iansu, I think this could be some incredible projects for getting a few commits into Node.js.

@ehmicky
Copy link
Collaborator

ehmicky commented Apr 7, 2019

For the 5 points I was mentioning:

  1. I could see how some might object to making shebangs work on Windows. Windows uses file associations instead. And this is not a problem if the caller specifies the program to run explicitly. Finally, this is only a problem with cmd.exe: I think if someone's using msys/MinGW shebangs might work. However others might find it convenient (which is one of the main features of node-cross-spawn).
  2. This is already documented on the Node.js website. There also already was a GitHub issue but it did not end up fixing the difference of behavior between Windows and Unix.
  3. There already was a GitHub issue and PR but it did not end up fixing the problem.
  4. This one I could not find any GitHub issue. So I created one.
  5. There already was a PR but it was rejected.

Based on that, I am wondering whether most of those proposals would move forward, considering they were already somewhat attempted at being fixed? However having any cross-platform issues fixed in core Node.js seems like the best approach.

Please let me know if anyone has other cross-platform issues in mind when it comes to child_process.

Robdel12 added a commit to percy/percy-agent that referenced this issue Apr 30, 2019
This PR introduces cross platform spawning of commands. Will close #174. 

The problem is Windows users need to use the `exec` command like this: 
```
$ percy exec -- cypress.cmd run
``` 

If they don't, we can't spawn their original test command. What's happening is nodes `spawn` ignores windows `PATHEXT` which is kinda like `$PATH` & that means `spawn` only works on Windows for `.exe` files. See: 
- nodejs/node-v0.x-archive#2318
- bcoe/awesome-cross-platform-nodejs#26

Tested here: https://github.com/percy/example-percy-cypress/compare/rd/test-cross-platform

The first commit uses `cross_spawn` and successfully starts Cypress. The second commit uses `0.4.0` of agent and fails to start Cypress.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants