phetsims / perennial

Maintenance tools that won't change with different versions of chipper checked out
MIT License
2 stars 5 forks source link

'grunt create-sim' fails on Windows with "Error: spawn EINVAL" #359

Closed fgamador closed 1 month ago

fgamador commented 1 month ago

When using a recent version of Node on Windows, 'grunt create-sim' fails with "Error: spawn EINVAL". I found that this is a known issue with recent versions of Node and can be fixed by adding shell: true to the options passed to child_process.spawn in perennial/js/common/execute.js (line 52). I tried it, and it works.

C:\Users\franz\Dev\NodeProjects\phetsims\perennial (main -> origin) (perennial@0.0.0) λ grunt create-sim --repo=my-first-sim --author="Franz Amador" Running "create-sim" task Greetings Franz Amador! creating sim with repository name my-first-sim wrote ../my-first-sim/.gitignore [...] wrote ../my-first-sim/tsconfig.json info: npm update in ../my-first-sim Fatal error: Perennial task failed: Error: spawn EINVAL at ChildProcess.spawn (node:internal/child_process:421:11) at Object.spawn (node:child_process:761:9) at C:\Users\franz\Dev\NodeProjects\phetsims\perennial\js\common\execute.js:52:40 at new Promise () at module.exports (C:\Users\franz\Dev\NodeProjects\phetsims\perennial\js\common\execute.js:45:10) at C:\Users\franz\Dev\NodeProjects\phetsims\perennial\js\common\npmUpdateDirectory.js:28:11 at C:\Users\franz\Dev\NodeProjects\phetsims\perennial\node_modules\async-mutex\lib\Mutex.js:23:66 at Semaphore. (C:\Users\franz\Dev\NodeProjects\phetsims\perennial\node_modules\async-mutex\lib\Semaphore.js:37:46) at step (C:\Users\franz\Dev\NodeProjects\phetsims\perennial\node_modules\tslib\tslib.js:195:27) at Object.next (C:\Users\franz\Dev\NodeProjects\phetsims\perennial\node_modules\tslib\tslib.js:176:57) Full Error details: Error: spawn EINVAL

fgamador commented 1 month ago

This may also fix #357.

zepumph commented 1 month ago

I was able to reproduce after upgrading my node version which was on 18.16.

Here is a limited patch that gets much working, but there are other usages of child processes that may want coverage (quake/aqua/weddell).

Furthermore, we likely want to test on windows and only optionally add this based on platform.

```diff Subject: [PATCH] shell:true, https://github.com/phetsims/density-buoyancy-common/issues/256 --- Index: chipper/js/scripts/precommit-hook-multi.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/scripts/precommit-hook-multi.js b/chipper/js/scripts/precommit-hook-multi.js --- a/chipper/js/scripts/precommit-hook-multi.js (revision facc2134c559512fa92b8fd827474fccd75b935a) +++ b/chipper/js/scripts/precommit-hook-multi.js (date 1721060039292) @@ -37,7 +37,7 @@ const execOnRepos = async ( repoSubset, command ) => { const promises = repoSubset.map( repo => { - return new Promise( resolve => child_process.exec( command, { cwd: repo }, error => resolve( error ) ) ); + return new Promise( resolve => child_process.exec( command, { cwd: repo, shell: true }, error => resolve( error ) ) ); } ); const results = await Promise.all( promises ); Index: perennial-alias/js/grunt/checkoutMainAll.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/perennial-alias/js/grunt/checkoutMainAll.js b/perennial-alias/js/grunt/checkoutMainAll.js --- a/perennial-alias/js/grunt/checkoutMainAll.js (revision 2557afe5afbed8d751a67156d99087a79a658314) +++ b/perennial-alias/js/grunt/checkoutMainAll.js (date 1721060039288) @@ -24,7 +24,7 @@ for ( let i = 0; i < gitRoots.length; i++ ) { const filename = gitRoots[ i ]; // Don't change to const without rewrapping usages in the closure if ( filename !== 'babel' && grunt.file.isDir( `../${filename}` ) && grunt.file.exists( `../${filename}/.git` ) ) { - child_process.exec( command, { cwd: `../${filename}` }, error => { + child_process.exec( command, { cwd: `../${filename}`, shell: true }, error => { if ( error ) { grunt.log.writeln( `error in ${command} for repo ${filename}` ); } Index: chipper/js/grunt/lint.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/lint.js b/chipper/js/grunt/lint.js --- a/chipper/js/grunt/lint.js (revision facc2134c559512fa92b8fd827474fccd75b935a) +++ b/chipper/js/grunt/lint.js (date 1721059933191) @@ -120,6 +120,7 @@ // It is nice to use our own spawn here instead of execute() so we can stream progress updates as it runs. const eslint = spawn( nxpCommand, args, { cwd: '../chipper', + shell: true, env: env // Use the prepared environment } ); Index: chipper/js/grunt/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/Gruntfile.js b/chipper/js/grunt/Gruntfile.js --- a/chipper/js/grunt/Gruntfile.js (revision facc2134c559512fa92b8fd827474fccd75b935a) +++ b/chipper/js/grunt/Gruntfile.js (date 1721059720503) @@ -732,7 +732,8 @@ const args = [ `--repo=${repo}`, ...process.argv.slice( 2 ) ]; const argsString = args.map( arg => `"${arg}"` ).join( ' ' ); const spawned = child_process.spawn( /^win/.test( process.platform ) ? 'grunt.cmd' : 'grunt', args, { - cwd: '../perennial' + cwd: '../perennial', + shell: true } ); grunt.log.debug( `running grunt ${argsString} in ../${repo}` ); Index: perennial-alias/js/common/execute.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/perennial-alias/js/common/execute.js b/perennial-alias/js/common/execute.js --- a/perennial-alias/js/common/execute.js (revision 2557afe5afbed8d751a67156d99087a79a658314) +++ b/perennial-alias/js/common/execute.js (date 1721059665376) @@ -51,7 +51,8 @@ const childProcess = child_process.spawn( cmd, args, { cwd: cwd, - env: options.childProcessEnv + env: options.childProcessEnv, + shell: true } ); childProcess.on( 'error', error => { Index: perennial-alias/js/grunt/checkoutShas.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/perennial-alias/js/grunt/checkoutShas.js b/perennial-alias/js/grunt/checkoutShas.js --- a/perennial-alias/js/grunt/checkoutShas.js (revision 2557afe5afbed8d751a67156d99087a79a658314) +++ b/perennial-alias/js/grunt/checkoutShas.js (date 1721060039284) @@ -39,7 +39,7 @@ //cp.exec('foocommand', { cwd: 'path/to/dir/' }, callback); //http://stackoverflow.com/questions/14026967/calling-child-process-exec-in-node-as-though-it-was-executed-in-a-specific-folde const command = `git checkout ${toMain ? 'main' : dependencies[ property ].sha}`; - child_process.exec( command, { cwd: `../${property}` }, ( error1, stdout1, stderr1 ) => { + child_process.exec( command, { cwd: `../${property}`, shell: true }, ( error1, stdout1, stderr1 ) => { assert( !error1, `error in ${command} for repo ${property}` ); grunt.log.writeln( 'Finished checkout.' ); grunt.log.writeln( stdout1 );
zepumph commented 1 month ago

Elevating priority since this totally breaks all our build tooling on windows after node upgrade.

zepumph commented 1 month ago

@samreid investigated and were able to trace it down to runnables that were not binaries (like bash scripts). For example grunt and npx. This helped us target exactly when we need the shell:true workaround. @fgamador, can you please check to see if this is fixed for you on main?

fgamador commented 1 month ago

Yup, that fixes it. Thanks!

zepumph commented 1 month ago

Excellent! I appreciate your quick response. Please do let us know if you run into anything else. We definitely want to continue improving our open source experience.