sindresorhus / execa

Process execution for humans
MIT License
6.76k stars 214 forks source link

Get rid of `cross-spawn` dependency #578

Open sindresorhus opened 10 months ago

sindresorhus commented 10 months ago

It's pretty unmaintained.

We do a lot to the shell, so would be simpler to do that too in execa ourselves.

Maybe some of the things it fixes are already fixed in Node.js too.

https://github.com/moxystudio/node-cross-spawn/blob/master/lib/parse.js

ehmicky commented 10 months ago

I agree that it not being maintained is a problem. Some of the bug reports seem rather important and are not currently looked into.

Breaking down cross-spawn feature by feature (which are all fixing problems with Windows):

  1. Executing files that have a shabang (since that's a Unix concept). Workaround for users would be to specify the interpreter explicitly instead. This would not work if the script location relies on the PATH environment variable, as opposed to being a file path. It does so by using which on the command, then reading the first bytes of the file to detect the shabang.
  2. Allows the command to a file path using the Unix path syntax. It does so by using path.normalize(), after having run which on the command.
  3. Some issues with escaping (commands with spaces, etc.) although I am not completely sure whether this is fixed or not in the latest version of Node. cross-spawn seems to fix this by wrapping the command in an additional cmd.exe call and manually escaping the command and arguments, using cmd.exe-specific syntax.
  4. It supports PATHEXT even when the shell option is false.

I am feeling conflicted.

What are your thoughts?

sindresorhus commented 10 months ago

Relevant: https://github.com/bcoe/awesome-cross-platform-nodejs/issues/26

sindresorhus commented 10 months ago

Maybe we could convince Bun to fix some of the things mention here to force Node.js' hand 🤣

sindresorhus commented 10 months ago

What are your thoughts?

I think it's worth doing it. cross-spawn is already buggy and unmaintained. We would release it as a major version to reduce the potential impact of any regressions.

ehmicky commented 10 months ago

Yes, that sounds good.

We would also need to add automated tests for the features covered by cross-spawn, although our current tests already cover some of it.

That's some major undertaking, so maybe we should start by just copy/pasting the code as is in an initial PR, and then start refactoring it. For example, some of it can be:

sindresorhus commented 10 months ago

That's some major undertaking, so maybe we should start by just copy/pasting the code as is in an initial PR, and then start refactoring it.

👍

ehmicky commented 3 weeks ago

I have spent some time figuring out what node-cross-spawn precisely does, and what are the problems with node:child_process on Windows it is fixing. This issue is a list of those problems.

Shebangs

Shebangs are a Unix concept. Windows does not support them.

node-cross-spawn emulates shebang support on Windows by:

Arguments splitting

In Unix, a subprocess file and arguments are specified as a file string and an arguments array of strings to the underlying C syscall. In Windows, those are passed as a single string, even when no shell is involved. The caller must escape the file and arguments with Windows-specific quoting rules so they are correctly split.

Node.js fixes this with the windowsVerbatimArguments option. When false, it performs that quoting automatically, which is basically, for each file or argument:

It does not escape all shell-specific characters, so it is only meant for arguments splitting, not full-on shell escaping. In particular, it does not prevent shell injection.

windowsVerbatimArguments is false by default. Is is always true when the shell option is true, which is good, since it allows users that execute cmd.exe manually to performs that quoting themselves.

node-cross-spawn does not do anything there: the Node.js behavior is already good.

Node modules binaries

On Windows, *.cmd and *.bat files require a shell (like cmd.exe) to be run. This also includes other file extensions, basically anything but *.exe or *.com.

OS binaries are often .exe/.com, e.g. node itself, git, etc. However, Node modules binaries (installed either globally or locally) are *.cmd/*.ps1/*.sh files.

This means that, to run Node modules binaries on Windows, one must use the shell: true option. That option wraps the command with cmd.exe /d /s /c "file arg...". It does not escape file and arg, and just joins the arguments with spaces.

This has a few problems:

node-cross-spawn fixes this by emulating shell: true when shell: false is used. In addition, it performs some full escaping of the file and arguments.

PATHEXT

Windows has an environment variable PATHEXT, which is the list of file extensions which can be omitted by users when executing files. However, Windows only supports it when using a shell.

node-cross-spawn emulates its support without shells by taking it into account when resolving the full path of the executable file.

windowsHide

The windowsHide is left as is by node-cross-spawn.

sindresorhus commented 3 weeks ago

Great summary 👍