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

'Fatal error: spawn <cmd> ENOENT' under windows #14

Closed
jdoose opened this issue Sep 3, 2019 · 10 comments · Fixed by #15
Closed

'Fatal error: spawn <cmd> ENOENT' under windows #14

jdoose opened this issue Sep 3, 2019 · 10 comments · Fixed by #15

Comments

@jdoose
Copy link

jdoose commented Sep 3, 2019

When using https://www.npmjs.com/package/grunt-simple-nyc to add nyc to grunt, an error was displayed:

'nyc' is not recognized as an internal or external command,
operable program or batch file.
Fatal error: spawn nyc ENOENT

grunt-simple-nyc internally uses simple-cli to call the nyc command line tool, which is an npm module residing in the ./node_modules/.bin directory.

simple-cli has a feature to add this directory to the path variable; this fails under Windows, since it is added with the wrong casing.
The constructor adds it with these lines:

      let localPath = `${resolve(this.cmd).split(this.cmd)[0]}.bin`;
      const pathDelimiter = process.platform === 'win32' ? ';' : ':';
      const path = [ localPath, process.env.PATH ].join(pathDelimiter);
      this.env = Object.assign(this.env, { PATH: path });

the last line uses "Path" in all upper case letters; the windows version of node seems to use the casing `Path'
(it looks like there is an issue for this: nodejs/node#20605)

Changing the last line of the above code block to

this.env = Object.assign(this.env, { Path: path });

makes simple-cli work as expected.

@tandrewnichols
Copy link
Owner

Okay, that makes sense. I'm not a big windows user, so I don't always think about these corner cases. A simple ternary on process.platform would be sufficient.

@tandrewnichols
Copy link
Owner

tandrewnichols commented Sep 4, 2019

@jdoose Can you verify whether #15 fixes this issue for you?

@jdoose
Copy link
Author

jdoose commented Sep 4, 2019

@jdoose Can you verify whether #15 fixes this issue for you?

Yes, it works now as expected. Thanks for the quick response!

One more idea:
Maybe it can be extended by a check if the property this.env.Path really exists before adding it? Just as a security check in case node.js ever changes it's implementation?
Around this line:
this.env = Object.assign(this.env, { [isWin32 ? 'Path' : 'PATH']: path });

@tandrewnichols
Copy link
Owner

I did wonder if they would attempt to preserve backward compatibility with that issue. I guess it's a simple check either way...

@tandrewnichols
Copy link
Owner

Do you mean checking if process.env.Path or process.env.PATH is canonical? Like isWin32 && ('Path' in process.env)?

@jdoose
Copy link
Author

jdoose commented Sep 4, 2019

Well, i meant, just to check if the env object already has a Path property.

@tandrewnichols
Copy link
Owner

Oh, I see. Like, don't overwrite it if it's already there.

@jdoose
Copy link
Author

jdoose commented Sep 4, 2019

No, the other way round.
We assume, node.js is using process.env.Path.
In that case, we want to make sure it is already there before we modify it.
This way it would become obvious if the node.js implementation changes someday.

But I think you are right, probably they will never change it. So it is not a big deal and the check is not necessary.

@tandrewnichols
Copy link
Owner

Published changes as 5.0.5

@jdoose
Copy link
Author

jdoose commented Sep 4, 2019

Great work, thanks!

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

Successfully merging a pull request may close this issue.

2 participants