prebuild / node-gyp-build

Build tool and bindings loader for node-gyp that supports prebuilds
MIT License
104 stars 35 forks source link

node-gyp-build fails on windows if path to node.exe contains a space #71

Closed Lesstat closed 1 week ago

Lesstat commented 3 months ago

Since upgrading to node 18.20 and node-gyp-build 4.8.1 node-gyp-build fails on a clean npm install on windows with the message:

'C:\Program' is not recognized as an internal or extrernal command, operable program or batch file

After debugging the issue locally I believe the issue originates in the build function in bin.js.

For windows, the command is run inside a shell and the path to node.exe is passed unquoted. Therefore, the shell interprets and splits the path. Locally, I could fix the issue by replacing line 25 with:

`"${process.execPath}"`,

I am hesitant to make a PR as I cannot currently test this on other platforms, but I am happy to do so if you want me to.

maheshsivast commented 3 months ago

Facing the same problem This commit should be the cause https://github.com/prebuild/node-gyp-build/commit/692860ef14f635cd9ab2cf06e879e964ec42deec

Revert line 30 to proc.spawn(args[0], args.slice(1), { stdio: 'inherit' }) is working value of args[0] in my case is 'C:\Program Files\nodejs\node.exe'

Installing the previous version solves the problem npm install --save-dev node-gyp-build@4.8.0

Lesstat commented 2 months ago

I just tested the fix in #72 on Windows with node versions 18.16.0, 18.18.2, 18.20.3 and 20.11.1. It worked for all those versions.

lolPlatinumPlayer commented 1 month ago

Its Crazy! This package weekly downloads 12,530,231 times. Surprisingly, there is such a fundamental and serious mistake. And this error message in Chinese computer will show like npm error 'C:\Program' �����ڲ����ⲿ���Ҳ���ǿ����еij��� npm error ���������ļ���. Its real hard for me to get to the root of the problem.

GustavPS commented 1 week ago

How come this issue has not been resolved yet? It has huge consequences and sounds like an easy fix?

mafintosh commented 1 week ago

There is a PR with the fix we can merge? If so happy to take a look.