prebuild / prebuildify

Create and package prebuilds for native modules
MIT License
200 stars 37 forks source link

[BUG] Error: spawn EINVAL #83

Closed Badisi closed 7 months ago

Badisi commented 7 months ago

Issue

I'm playing with the following github action:

  prebuild:
    runs-on: ${{ matrix.os }}
    ...
    - name: Setup node
      uses: actions/setup-node@v4
      with:
        node-version: 20
        architecture: ${{ matrix.arch }}
    ...
    - name: Prebuild
      run: prebuildify --target 12.0.0 --napi --strip

The main idea is to run prebuildify on macos-latest:x64, ubuntu-latest:x64, windows-latest:x64 and windows-latest:x86.

Everything works fine except for the windows-latest:x86 where I get the following issue:

node:internal/child_process:414
    throw errnoException(err, 'spawn');
    ^

Error: spawn EINVAL
    at ChildProcess.spawn (node:internal/child_process:414:11)
    at Object.spawn (node:child_process:761:9)
    at D:\a\my-project\node_modules\prebuildify\index.js:230:22
    at D:\a\my-project\node_modules\mkdirp-classic\index.js:30:20
    at FSReqCallback.oncomplete (node:fs:192:23) {
  errno: -4071,
  code: 'EINVAL',
  syscall: 'spawn'

Investigation

The code in prebuildify index.js:230 is:

var child = proc.spawn(opts.nodeGyp, args, {
  cwd: opts.cwd,
  env: opts.env,
  stdio: opts.quiet ? 'ignore' : 'inherit'
})

After debugging I can see that the actual value used for opts.nodeGyp is "node-gyp.cmd":

{
  ...
  stdio: 'inherit',
  args: [
    'node-gyp.cmd',
    'rebuild',
    '--target=12.0.0',
    '--devdir=C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\prebuildify\\node',
    '--arch=ia32',
    '--release'
  ],
  detached: false,
  envPairs: [
    ...,
    ... 98 more items
  ],
  file: 'node-gyp.cmd',
  windowsHide: false,
  windowsVerbatimArguments: false
}

Solution

There seems to have some issues with spawn on Windows and most of the time people suggests to use shell: true to solve it.

So I monkey patched the previous code in prebuildify like this:

var child = proc.spawn(opts.nodeGyp, args, {
  shell: true,
  cwd: opts.cwd,
  env: opts.env,
  stdio: opts.quiet ? 'ignore' : 'inherit'
})

and it indeed solved the issue.

kylegoetz commented 7 months ago

I am also getting this when using tree-sitter's package-npm workflow. Exact same issue, only happening when building for windows 64bit. Not an issue for macOS and linux ubuntu

vweevers commented 7 months ago

Which Node.js versions have this issue? Only 20?

derevnjuk commented 7 months ago

@vweevers It relates to versions 18 and 20, which received some security fixes.

vweevers commented 7 months ago

PR is welcome to add shell: true. We might want to investigate why it happens, but shell shouldn't have side effects AFAICT so that's a good short-term fix.

Badisi commented 7 months ago

Same as @derevnjuk, I can confirm the issue exists with node 18 and 20.

As for the side effects, I can also confirm that using shell: true on every platforms worked fine in my case (so no side effects).

derevnjuk commented 7 months ago

@Badisi It seems it only affects Windows x86, but I'm not sure. Have you tested it?

Badisi commented 7 months ago

@derevnjuk, Yes, my github action is running on macos-latest:x64, ubuntu-latest:x64, windows-latest:x64 and windows-latest:x86, and only windows-latest:x86 was failing.

Adding shell: true was enough to make it work and had no impacts on the other platforms.

derevnjuk commented 7 months ago

@Badisi, thanks! 🙏🏾 I have opened the PR and am waiting for the review and feedback.

derevnjuk commented 7 months ago

@vweevers the root cause is straightforward: Node.js team forbids the use of .bat and .cmd without the shell option, as seen here: https://github.com/nodejs/node/commit/6627222409. For details, please refer to the following issues: https://github.com/nodejs/node/issues/52470 and https://github.com/node-red/node-red/pull/4652.

kylegoetz commented 7 months ago

Which Node.js versions have this issue? Only 20?

21.7.3 is what's used in my workflow

Screen Shot 2024-04-15 at 12 03 11 PM
derevnjuk commented 7 months ago

Could you please provide an update on this issue? I believe this, along with the similar issue prebuild/node-gyp-build#68, should be considered critical bugs.