node-gyp-build icon indicating copy to clipboard operation
node-gyp-build copied to clipboard

fix windows spawn shell

Open cb1kenobi opened this issue 1 year ago • 1 comments

You only need shell: true on Windows when you are spawning a .cmd file like node-gyp or npm. Since node is an .exe and not a .cmd, we should set shell = false.

fixes #71

cb1kenobi avatar Jun 29 '24 04:06 cb1kenobi

I just spent 4 hours to get to this point, to only find, that fix already exists but waiting for review.

But I think that shell parameter should be completely avoided, because it is obviously dangerous and not forgiving, so something like:

var args;
if (os.platform() === 'win32') {
  args = [process.env.ComSpec, SOME SHELL PARAMETERS, 'node-gyp.cmd']
} else {
  args = ['node-gyp']
}
args.push('rebuild')

try {
  var pkg = require('node-gyp/package.json')
  args = [
    process.execPath,
    path.join(require.resolve('node-gyp/package.json'), '..', typeof pkg.bin === 'string' ? pkg.bin : pkg.bin['node-gyp']),
    'rebuild'
  ]
} catch (_) {}

proc.spawn(args[0], args.slice(1), { stdio: 'inherit', windowsHide: true }).on('exit', function (code) {
  if (code || !process.argv[3]) process.exit(code)
  exec(process.argv[3]).on('exit', function (code) {
    process.exit(code)
  })
})

polomsky avatar Jul 10 '24 11:07 polomsky

think current fix makes sense, thanks!

mafintosh avatar Aug 28 '24 13:08 mafintosh

4.8.2

mafintosh avatar Aug 28 '24 13:08 mafintosh