Please add switch to opt-out of special ENOENT behavior for Windows if desired

[mattflix]

In my use of cross-spawn (actually via the cross-env package), I have discovered that there is some special error-emission behavior (apparently for Windows and cmd.exe?) implemented by the enoent.js source file, which is described on the following line:

https://github.com/moxystudio/node-cross-spawn/blob/master/lib/enoent.js#L25

This line also contains a comment which refers the original issue.

Basically, the special behavior is: If the spawned process exits with 1 and the command line cannot be found in file system, emit an Error describing an ENOENT error condition (i.e., "command not found").

(It may be helpful to familiarize yourself with the source code and the details of the original issue before reading on.)

But, there is (and IMO has always been) a problem: The logic in enoent.js assumes that exit code 1 should aways trigger the special ENOENT error-emission behavior... but the assumptions behind that special behavior are only correct in certain cases, wrong in others.

Firstly, cmd.exe may not even be in the picture, so any behavior based on the assumption that it is, is flawed. (Windows has a new shell, now the default, called PowerShell.exe, so the chances that cmd.exe is not being used is actually quite high.)

Secondly, there are many used-cases in which an exit code of 1 does not implicitly mean "command not found". Even with cmd.exe this could easily be the case... any Windows command script, or process launched by the script, could simply choose to exit the script with 1, or any other code, for any purpose.

Thirdly, the command passed to cross-spawn may not be an actual file-system location that can be "verified" exists or not. This is especially true with PowerShell, since the command might refer to a "cmdlet" (callable entities which are named independently of the file system) that ran successfully, but simply chose to exit with 1.

In short, this special hard-coded behavior can easily do the wrong thing. For some cases, it may be desirable to convert exit code 1 + "command not found" into an emission of ENOENT, but in other cases, this logic is wrong, and only creates confusion and annoyance.

Case in point: Unit test frameworks (mocha for example) exit with the number of failed tests (i.e., 0 for success, any other number for failure). When cross-spawn is used in such a case, a confusing ENOENT error occurs whenever 1 test fails, but not when 2 more tests fail.

For real-world cases like this, where the special ENOENT behavior is not appropriate, would you consider adding a flag to allow the caller to opt-out of the special logic? That is, just pass any exit code straight through. (Better yet, IMO, place the current special behavior behind an opt-in flag instead.)

Thanks.

[satazor]

@mattflix are you willing to do a PR to add such an feature via an option named shimENOENT? The option should be well documented with some examples where it's useful to be false, such as when using "cmdlet" commands.

[aoberoi]

I just wanted to express a big thanks to @mattflix for describing this issue so well.

I am also running into a problem that could be solved by an option for disabling the ENOENT shimming behavior. Specifically, I'm sending the process a SIGKILL and I expect the process to exit with the code 1. However, the shim is masking my ability to handle that. The 'close' event never fires, and therefore my handling of the exit code is not effective (I'd like to throw the error if its not the result of a SIGKILL).

I don't want to volunteer to fix this, because I don't have a Windows machine handy, nor do I have the interest in understanding this issue any more intimately. I have a workaround that I think is good enough for now. I just wanted to make the community aware of this usage and the need.

[mattflix]

@satazor, I could take a stab at it.

I see the modifications needed in node-cross-spawn should be pretty simple. Something like... just avoid branching to the existing ENOENT shimming logic when the proposed parsed.options.shimENOENT value is falsy?

I have some concerns/questions:

• What should the default value for shimENOENT be?

  Should we make the shimming an opt-in behavior (default the value to false) or opt-out (default true)? Personally, I would prefer to opt-in rather than opt-out, but I realize this might break existing clients reliant on the current unconditional shimming, and who would have to update their code to opt-in (although it would certainly fix me and @aoberoi ;).

• For my case (indirect usage of node-cross-spawn via the cross-env package) how should we go about getting the owner of that package to move to an updated version node-cross-spawn?

  Also, depending on how our proposed shimENOENT options ends up working, that package may or may not require code changes (especially if they want their clients to be able to control the value of the shimENOENT option -- and I would be one such client).

• Lastly, what do you think of allowing the shimming to be (or also be) controlled by an environment variable? (Perhaps named NODE_CROSS_SPAWN_SHIM_ENOENT?)

  That way, the shimming could be controlled independently of any third-party code. If we release the optional shimming as a patch release, dependents like cross-env would simply pick it up and allow folks like @aoberoi and me to fix ourselves by setting NODE_CROSS_SPAWN_SHIM_ENOENT=false.

Any thoughts or advice?

[satazor]

Sorry for the late answer, I'm reading all the issues again and I would like to refresh this one.

Secondly, there are many used-cases in which an exit code of 1 does not implicitly mean "command not found". Even with cmd.exe this could easily be the case... any Windows command script, or process launched by the script, could simply choose to exit the script with 1, or any other code, for any purpose.

Correct, that's why there's a check in place to also see if the actual command file exists and only throw ENOENT if it doesn't.

Thirdly, the command passed to cross-spawn may not be an actual file-system location that can be "verified" exists or not. This is especially true with PowerShell, since the command might refer to a "cmdlet" (callable entities which are named independently of the file system) that ran successfully, but simply chose to exit with 1.

This module is not designed to call cmdlets as they are not cross-platform, meaning you shouldn't use cross-spawn in these scenarios.

Case in point: Unit test frameworks (mocha for example) exit with the number of failed tests (i.e., 0 for success, any other number for failure). When cross-spawn is used in such a case, a confusing ENOENT error occurs whenever 1 test fails, but not when 2 more tests fail.

This shouldn't happen as this module should have detect mocha as a file, meaning it shouldn't throw the ENOENT error. Is parsed.file here undefined in that case?

---

I would like to replicate the issue to understand if we could improve the ENOENT shim, reason being that this error is what is thrown in Unix when the command doesn't exist and I would like to keep consistency among Unix and Windows as much as possible.

[mattflix]

Thanks for the reply.

This module is not designed to call cmdlets as they are not cross-platform, meaning you shouldn't use cross-spawn in these scenarios.

It is not PowerShell cmdlets specifically that are "the issue" here, but rather cross-spawn's assumption that a command can only be valid if it names something in the file system. This is simply not always the case. PowerShell cmdlets are just one example that break this assumption, but other kinds of "aliasing" or methods of identifying commands (that the shell understands but cross-spawn does not) can lead to similar problems.

Not to get off track, but keep in mind there may be many legitimate ways to use cmdlets as part of an overall cross-platform solution (of which cross-spawn might only be one part). Somebody might create a cmdlet to emulate a Bash command, for example.

This shouldn't happen as this module should have detect mocha as a file, meaning it shouldn't throw the ENOENT error. Is parsed.file here undefined in that case?

Not sure if anything isundefined or not, but here is a minimal package.json to repro:

{
  "name": "enoent",
  "version": "1.0.0",
  "dependencies": {
    "cross-env": "^5.2.0",
    "mocha": "^6.0.2"
  },
  "config": {
      "tests": "--ui exports test.js"
  },
  "scripts": {
      "mocha": "cross-env-shell mocha $npm_package_config_tests",
      "premocha": "node -e \"console.log(\\\"module.exports = { test() { throw new Error } }\\\")\" > test.js"
  }
}

This repros basically the exact issue (though greatly simplified) I had running mocha via cross-env-shell (and which prompted me to initially file this issue, although I tried to frame the issue as more general, offering the cmdlet example as another way the same type of problem might occur).

Anyway, to repro, place the above text in a file named package.json in a new folder, and then run npm install and then npm -s run mocha from the command line. Try it on both Mac and Windows.

(Btw, the premocha step is my way of shoehorning this repro case into a single package.json block -- it simply generates a test suite that will cause mocha to exit with 1, which is the problematic scenario.)

On Mac, npm -s run mocha gives a nice clean output about the single test failure. The clean output is the expected behavior, and what you would get under normal circumstances without cross-spawn in the picture on Mac.

On Windows, you get the mocha output plus an unexpected, bogus stack trace about ENOENT (coming from cross-spawn). Apparently, cross-spawn saw the exit code of 1, incorrectly decided that mocha does not exist, and threw. The same thing can happen whenever cross-spawn's ENOENT shim thinks that something "does not exist" (when it is really may be a perfectly valid command that just happened to return 1, but is also not a file system entity).

In this repo case, obviously mocha is is an entity within the file system, but I think the problem is that cross-spawn does not understand how npm resolves .bin commands (like mocha). And I'm not necessarily saying it should know... I'm just trying to point out that cross-spawn's assumptions about how to decide when exit 1 means ENOENT are incorrect, or at least incomplete, and this can be problematic in fairly common cases (such as running mocha).

IMO, cross-spawn really cannot know how to validate all possible commands that might legitimately return 1 for reasons other than an underlying ENOENT ...and so maybe it should not? Or, at least perhaps make the shimming behavior configurable? Personally, I would prefer the behavior be opt-in (for folks that decided they truly want/need it). But, even if it were opt-out, that would be a welcome change.

Of course, if the shim can be fixed to just "always do the right thing", that would be great too... but I don't think that is realistically possible.

I would like to replicate the issue to understand if we could improve the ENOENT shim, reason being that this error is what is thrown in Unix when the command doesn't exist and I would like to keep consistency among Unix and Windows as much as possible.

Hopefully the above repro will give you something to go on.

[ehmicky]

+1 on everything @mattflix said.

I think the ENOENT logic should be simply removed altogether. It is flawed as @mattflix described it. Namely it is assuming the following:

• when running on Windows with shell: true (cmd.exe)
• then any exit code 1 means ENOENT

This is not true. The only way to spot ENOENT in cmd.exe is to check the error message printed on stderr: ... is not recognized as an internal or external command. And as @mattflix pointed out, this would not work on Powershell.

I.e. the ENOENT-related logic is not needed. Removing it also simplifies the code and removes the childProcess.emit() monkey patching.

Example to reproduce the issue (on Windows):

test.js:

throw new Error('This error message and stack trace will never be shown')

In Node:

const crossSpawn = require('cross-spawn')

const childProcess = crossSpawn('node', ['test.js'], {shell: true})
childProcess.on('exit', (exitCode, signal) => {
  console.log({ exitCode, signal })
})
childProcess.on('error', (error) => {
  console.log(error)
}  

Prints:

Error: spawn node ENOENT
    at notFoundError (C:\Users\ehmic\build\node_modules\cross-spawn\lib\enoent.js:6:26)
    at verifyENOENT (C:\Users\ehmic\build\node_modules\cross-spawn\lib\enoent.js:40:16)
    at ChildProcess.cp.emit (C:\Users\build\node_modules\cross-spawn\lib\enoent.js:27:25)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:272:12) {
  code: 'ENOENT',
  errno: 'ENOENT',
  syscall: 'spawn node',
  path: 'node',
  spawnargs: [ 'test.js' ]

If you're ok with removing the logic altogether, I can submit a PR.

[tarithj]

Is there a fix to this issue?

[ehmicky]

This bug was mentioned in sindresorhus/execa#446

@satazor If you think my above comment is a good solution, please let me know and I will submit a PR. Thanks!

[ehmicky]

Some additional information to add to this issue: Node.js actually implements some logic to detect ENOENT on Windows. It does so by translating many Windows system error codes to ENOENT in libuv (link to code). When this happens, an error event is emitted on the child_process (link to code).

Going back to the original issue which created the feature we are discussing, the problem is than node-cross-spawn sometimes wraps the command with cmd.exe in order to support shebangs. This makes the above Node.js behavior not work anymore.

However, libuv has access to the Windows API system error codes while node-cross-spawn only has access to the exit code, stdout and stderr. With only this information, it is unfortunately not possible to reliably detect ENOENT on Windows, so it seems to me it would be better to remove this feature since this is creating some problems as mentioned by @mattflix and @aoberoi.

[jairmilanes]

I just like to leave my +1 here.

I ran into this and it took me a day to understand why my command had the full expected output in stdout but was ending with a ENOENT error.

I'm working on a Electron app that uses NPM cli to run some commands, including the outdated command, and when there are outdated dependencies, the command ends with status code 1, but it is not actually an error, it just means there are outdated dependencies, and while running with shell=true I was getting the ENOENT error which was confused to say the least and took me a day to realize what was happening.

I'm not too familiar with spawn and only recently started to use it so for sure this played a role on the time it took for me to debug this, but I bet this isn't the only case out there form cli tools where exit code 1 means something other than command not found, so leaving here my vote to at least make this optional behavior so it is not so confusing to debug. specially for less experienced people out there like me.

Thanks for making cross-spawn great tool, helped me with other problems in Windows, and this would just be an improvement to such a helpful package. Cheers!