Improve execute and add ExecuteResult

[samreid]

From https://github.com/phetsims/perennial/issues/206#issuecomment-793031078,

I recommend the following changes for execute.js.

• Move ExecuteError out of the closure (to a top level, non-exported type)
• Rename ExecuteError to ExecuteResult, and add message
• All reject/resolve should return new ExecuteResult.
• ExecuteResult should have an optional message

Note that we raised reasons not to put files in dual/ which are under discussion in https://github.com/phetsims/phet-io/issues/1733#issuecomment-877301298

[samreid]

Here are my main concerns before starting on this:

• Is it OK to put execute in dual/? That means to make an improvement for chipper, you have to commit to perennial, run a task, commit to chipper and push both. If you commit directly to chipper, your changes will be overwritten. Commit history for chipper will not be idiomatic. I'd like to find a better way, but I'm not sure what it would be.
• How to get uniform winston support? Is it ok for perennial clients to have to pass in a winston option for every execute?

Would it help if we use util.promisify as described in https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback ?

@jonathanolson or @zepumph can you please advise? Or let me know if we should request other volunteers at dev meeting.

[jonathanolson]

Is it ok for perennial clients to have to pass in a winston option for every execute?

That seems less than ideal, preference against that. Could we add winston to chipper?

The other consistency sounds good to me. My main question is... would ExecuteResult extend Error? I see it either as "we are throwing something that isn't an error" or "we are returning something normal that is subtyped as an Error". Thoughts?

[zepumph]

Is it OK to put execute in dual/?

How possible is it to instead devote our energy into converting all our tooling to modules? Perhaps that isn't worth it because of the soon-to-come (???) typescript conversion. No matter, this feels more and more like a headache in the making.

[samreid]

How possible is it to instead devote our energy into converting all our tooling to modules?

Can you please elaborate on how converting tooling to modules would help? execute is already a module and suffers from the "perennial always wants to be in master" problem.

[zepumph]

Oh, a simple answer: because I was being dump and incorrect. Do you want to go all in on dual/ as our strategy to use files in perennial and chipper. I would really like to.

[samreid]

I added winston to chipper and moved execute to dual as part of https://github.com/phetsims/chipper/issues/1018. Next steps:

• Move ExecuteError out of the closure (to a top level, non-exported type)
• Rename ExecuteError to ExecuteResult, and add message
• All reject/resolve should return new ExecuteResult.
• ExecuteResult should have an optional message

My main question is... would ExecuteResult extend Error? I see it either as "we are throwing something that isn't an error" or "we are returning something normal that is subtyped as an Error".

I don't think ExecuteResult should extend Error--that sounds confusing for the normal non-error case. Maybe ExecuteError can extend Error and contain ExecuteResult? But maybe that would be an awkward API.

[samreid]

Numerous occurrences rely on the success output just resolving with stdout. Do we want to change all of them to resolve with the ExecuteResult and they can take the stdout? Or do we like the convenience of resolving with stdout even though sometimes in resolves with ExecuteResult? For instance, getBranch currently reads:

  return execute( 'git', [ 'symbolic-ref', '-q', 'HEAD' ], `../${repo}` ).then( stdout => stdout.trim().replace( 'refs/heads/', '' ) );

But if we resolve with ExecuteResult it would be more like:

return execute( 'git', [ 'symbolic-ref', '-q', 'HEAD' ], `../${repo}` ).then( result => result.stdout.trim().replace( 'refs/heads/', '' ) );

@jonathanolson can you please advise?

[jonathanolson]

Do we want to change all of them to resolve with the ExecuteResult and they can take the stdout?

That seems fine/expected to me.

[samreid]

I got partway through in this patch:

```diff Index: main/perennial/js/common/getRemoteBranchSHAs.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/perennial/js/common/getRemoteBranchSHAs.js b/main/perennial/js/common/getRemoteBranchSHAs.js --- a/main/perennial/js/common/getRemoteBranchSHAs.js (revision e3618c303791070b3f3e38f52e57192e6975d3d3) +++ b/main/perennial/js/common/getRemoteBranchSHAs.js (date 1627436860785) @@ -23,7 +23,7 @@ const map = {}; - ( await execute( 'git', [ 'ls-remote' ], `../${repo}` ) ).split( '\n' ).forEach( line => { + ( await execute( 'git', [ 'ls-remote' ], `../${repo}` ) ).stdout.split( '\n' ).forEach( line => { const match = line.trim().match( /^(\S+)\s+refs\/heads\/(\S+)$/ ); if ( match ) { map[ match[ 2 ] ] = match[ 1 ]; Index: main/chipper/js/grunt/copySupplementalPhetioFiles.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/chipper/js/grunt/copySupplementalPhetioFiles.js b/main/chipper/js/grunt/copySupplementalPhetioFiles.js --- a/main/chipper/js/grunt/copySupplementalPhetioFiles.js (revision e476780b824a2ae181e73844c4e7377c98b4af20) +++ b/main/chipper/js/grunt/copySupplementalPhetioFiles.js (date 1627436740612) @@ -406,7 +406,7 @@ // Running with explanation -X appears to not output the files, so we have to run it twice. const explanation = ( await execute( 'node', getArgs( true ), process.cwd(), { shell: true - } ) ).trim(); + } ) ).stdout.trim(); // Copy the logo file const imageDir = `${buildDir}doc/images`; Index: main/perennial/js/common/gitCherryPick.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/perennial/js/common/gitCherryPick.js b/main/perennial/js/common/gitCherryPick.js --- a/main/perennial/js/common/gitCherryPick.js (revision e3618c303791070b3f3e38f52e57192e6975d3d3) +++ b/main/perennial/js/common/gitCherryPick.js (date 1627436954879) @@ -21,10 +21,10 @@ module.exports = function( repo, target ) { winston.info( `git cherry-pick ${target} on ${repo}` ); - return execute( 'git', [ 'cherry-pick', target ], `../${repo}` ).then( stdout => Promise.resolve( true ), cherryPickError => { + return execute( 'git', [ 'cherry-pick', target ], `../${repo}` ).then( result => Promise.resolve( true ), cherryPickError => { winston.info( `git cherry-pick failed (aborting): ${target} on ${repo}` ); - return execute( 'git', [ 'cherry-pick', '--abort' ], `../${repo}` ).then( stdout => Promise.resolve( false ), abortError => { + return execute( 'git', [ 'cherry-pick', '--abort' ], `../${repo}` ).then( result => Promise.resolve( false ), abortError => { winston.error( `git cherry-pick --abort failed: ${target} on ${repo}` ); return Promise.reject( abortError ); } ); Index: main/aqua/js/server/Snapshot.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/aqua/js/server/Snapshot.js b/main/aqua/js/server/Snapshot.js --- a/main/aqua/js/server/Snapshot.js (revision 07ffa27a2f67eb6832e4452d29cf7a2e47102059) +++ b/main/aqua/js/server/Snapshot.js (date 1627436740627) @@ -97,8 +97,8 @@ for ( const repo of getRepoList( 'active-runnables' ) ) { this.setStatus( `Scanning dependencies for timestamps: ${repo}` ); try { - const output = await execute( 'node', [ 'js/scripts/print-dependencies.js', repo ], `${this.rootDir}/chipper` ); - const dependencies = output.trim().split( ',' ); + const result = await execute( 'node', [ 'js/scripts/print-dependencies.js', repo ], `${this.rootDir}/chipper` ); + const dependencies = result.stdout.trim().split( ',' ); let timestamp = 0; for ( const dependency of dependencies ) { const dependencyTime = lastRepoTimestamps[ dependency ]; @@ -118,7 +118,7 @@ this.setStatus( 'Loading tests from perennial' ); // @public {Array.} - this.tests = JSON.parse( await execute( 'node', [ 'js/listContinuousTests.js' ], '../perennial' ) ).map( description => { + this.tests = JSON.parse( await execute( 'node', [ 'js/listContinuousTests.js' ], '../perennial' ).stdout ).map( description => { const potentialRepo = description && description.test && description.test[ 0 ]; return new Test( this, description, lastRepoTimestamps[ potentialRepo ] || 0, lastRunnableTimestamps[ potentialRepo ] || 0 ); Index: main/perennial/js/dual/execute.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/perennial/js/dual/execute.js b/main/perennial/js/dual/execute.js --- a/main/perennial/js/dual/execute.js (revision e3618c303791070b3f3e38f52e57192e6975d3d3) +++ b/main/perennial/js/dual/execute.js (date 1627436740615) @@ -13,13 +13,43 @@ * @author Jonathan Olson */ - const child_process = require( 'child_process' ); const winston = require( 'winston' ); const _ = require( 'lodash' ); // eslint-disable-line const assert = require( 'assert' ); const grunt = require( 'grunt' ); +class ExecuteResult { + + /** + * @param {string} cmd + * @param {Array.} args + * @param {string} cwd + * @param {string} stdout + * @param {string} stderr + * @param {number|null} code - exit code or null if it didn't run at all + */ + constructor( cmd, args, cwd, stdout, stderr, code ) { + + // @public + this.cmd = cmd; + this.args = args; + this.cwd = cwd; + this.stdout = stdout; + this.stderr = stderr; + this.code = code; + } +} + +class ExecuteError extends Error { + constructor( executeResult ) { + super( `${executeResult.cmd} ${executeResult.args.join( ' ' )} in ${executeResult.cwd} failed with exit code ${executeResult.code}${executeResult.stdout ? `\nstdout:\n${executeResult.stdout}` : ''}${executeResult.stderr ? `\nstderr:\n${executeResult.stderr}` : ''}` ); + + // @public + this.executeResult = executeResult; + } +} + /** * Executes a command, with specific arguments and in a specific directory (cwd). * @public @@ -32,7 +62,7 @@ * @param {string} cwd - The working directory where the process should be run from * @param {Object} [options] * @returns {Promise.} - Stdout - * @rejects {ExecuteError} + * @rejects {ExecuteResult} */ module.exports = function( cmd, args, cwd, options ) { @@ -45,28 +75,6 @@ }, options ); assert( options.errors === 'reject' || options.errors === 'resolve', 'Errors must reject or resolve' ); - class ExecuteError extends Error { - /** - * @param {string} cmd - * @param {Array.} args - * @param {string} cwd - * @param {string} stdout - * @param {string} stderr - * @param {number} code - exit code - */ - constructor( cmd, args, cwd, stdout, stderr, code ) { - super( `${cmd} ${args.join( ' ' )} in ${cwd} failed with exit code ${code}${stdout ? `\nstdout:\n${stdout}` : ''}${stderr ? `\nstderr:\n${stderr}` : ''}` ); - - // @public - this.cmd = cmd; - this.args = args; - this.cwd = cwd; - this.stdout = stdout; - this.stderr = stderr; - this.code = code; - } - } - return new Promise( ( resolve, reject ) => { let rejectedByError = false; @@ -78,15 +86,16 @@ cwd: cwd } ); - process.on( 'error', error => { + process.on( 'error', () => { rejectedByError = true; + const errorResult = new ExecuteResult( cmd, args, cwd, stdout, stderr, null ); if ( options.errors === 'resolve' ) { - resolve( { code: 1, stdout: stdout, stderr: stderr, cwd: cwd, error: error } ); + resolve( errorResult ); } else { - reject( new ExecuteError( cmd, args, cwd, stdout, stderr, -1 ) ); + reject( new ExecuteError( errorResult ) ); } } ); winston.debug( `Running ${cmd} ${args.join( ' ' )} from ${cwd}` ); @@ -107,16 +116,12 @@ winston.debug( stdout && `stdout: ${stdout}` || 'stdout is empty.' ); if ( !rejectedByError ) { - if ( options.errors === 'resolve' ) { - resolve( { code: code, stdout: stdout, stderr: stderr, cwd: cwd } ); + const result = new ExecuteResult( cmd, args, cwd, stdout, stderr, code ); + if ( options.errors === 'resolve' || code === 0 ) { + resolve( result ); } else { - if ( code !== 0 ) { - reject( new ExecuteError( cmd, args, cwd, stdout, stderr, code ) ); - } - else { - resolve( stdout ); - } + reject( new ExecuteError( result ) ); } } } ); Index: main/aqua/js/server/ContinuousServer.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/aqua/js/server/ContinuousServer.js b/main/aqua/js/server/ContinuousServer.js --- a/main/aqua/js/server/ContinuousServer.js (revision 07ffa27a2f67eb6832e4452d29cf7a2e47102059) +++ b/main/aqua/js/server/ContinuousServer.js (date 1627436740621) @@ -591,12 +591,12 @@ test.complete = true; this.saveToFile(); try { - const output = await execute( gruntCommand, [ 'lint' ], `${snapshot.directory}/${test.repo}` ); + const result = await execute( gruntCommand, [ 'lint' ], `${snapshot.directory}/${test.repo}` ); - ContinuousServer.testPass( test, Date.now() - startTimestamp, output ); + ContinuousServer.testPass( test, Date.now() - startTimestamp, result.stdout ); } catch( e ) { - ContinuousServer.testFail( test, Date.now() - startTimestamp, `Lint failed with status code ${e.code}:\n${e.stdout}\n${e.stderr}`.trim() ); + ContinuousServer.testFail( test, Date.now() - startTimestamp, `Lint failed with status code ${e.executeResult.code}:\n${e.executeResult.stdout}\n${e.executeResult.stderr}`.trim() ); } this.saveToFile(); } @@ -604,12 +604,12 @@ test.complete = true; this.saveToFile(); try { - const output = await execute( gruntCommand, [ 'lint-everything' ], `${snapshot.directory}/perennial` ); + const result = await execute( gruntCommand, [ 'lint-everything' ], `${snapshot.directory}/perennial` ); - ContinuousServer.testPass( test, Date.now() - startTimestamp, output ); + ContinuousServer.testPass( test, Date.now() - startTimestamp, result.stdout ); } catch( e ) { - ContinuousServer.testFail( test, Date.now() - startTimestamp, `Lint-everything failed with status code ${e.code}:\n${e.stdout}\n${e.stderr}`.trim() ); + ContinuousServer.testFail( test, Date.now() - startTimestamp, `Lint-everything failed with status code ${e.executeResult.code}:\n${e.executeResult.stdout}\n${e.executeResult.stderr}`.trim() ); } this.saveToFile(); } @@ -617,13 +617,13 @@ test.complete = true; this.saveToFile(); try { - const output = await execute( gruntCommand, [ `--brands=${test.brands.join( ',' )}`, '--lint=false' ], `${snapshot.directory}/${test.repo}` ); + const result = await execute( gruntCommand, [ `--brands=${test.brands.join( ',' )}`, '--lint=false' ], `${snapshot.directory}/${test.repo}` ); - ContinuousServer.testPass( test, Date.now() - startTimestamp, output ); + ContinuousServer.testPass( test, Date.now() - startTimestamp, result.stdout ); test.success = true; } catch( e ) { - ContinuousServer.testFail( test, Date.now() - startTimestamp, `Build failed with status code ${e.code}:\n${e.stdout}\n${e.stderr}`.trim() ); + ContinuousServer.testFail( test, Date.now() - startTimestamp, `Build failed with status code ${e.executeResult.code}:\n${e.executeResult.stdout}\n${e.executeResult.stderr}`.trim() ); } this.saveToFile(); } Index: main/perennial/js/common/getBranches.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/perennial/js/common/getBranches.js b/main/perennial/js/common/getBranches.js --- a/main/perennial/js/common/getBranches.js (revision e3618c303791070b3f3e38f52e57192e6975d3d3) +++ b/main/perennial/js/common/getBranches.js (date 1627436860789) @@ -21,7 +21,7 @@ module.exports = async function( repo ) { winston.debug( `retrieving branches from ${repo}` ); - return ( await execute( 'git', [ 'ls-remote' ], `../${repo}` ) ).split( '\n' ).filter( line => line.includes( 'refs/heads/' ) ).map( line => { + return ( await execute( 'git', [ 'ls-remote' ], `../${repo}` ) ).stdout.split( '\n' ).filter( line => line.includes( 'refs/heads/' ) ).map( line => { return line.match( /refs\/heads\/(.*)/ )[ 1 ].trim(); } ); }; Index: main/perennial/js/common/build.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/perennial/js/common/build.js b/main/perennial/js/common/build.js --- a/main/perennial/js/common/build.js (revision e3618c303791070b3f3e38f52e57192e6975d3d3) +++ b/main/perennial/js/common/build.js (date 1627436740607) @@ -100,9 +100,9 @@ // Examine output to see if getDependencies (in chipper) notices any missing phet-io things. // Fail out if so. Detects that specific error message. - if ( includesPhetio && result.includes( 'WARNING404' ) ) { + if ( includesPhetio && result.stdout.includes( 'WARNING404' ) ) { throw new Error( 'phet-io dependencies missing' ); } - return result; + return result.stdout; }; Index: main/chipper/js/grunt/getCopyrightLine.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/chipper/js/grunt/getCopyrightLine.js b/main/chipper/js/grunt/getCopyrightLine.js --- a/main/chipper/js/grunt/getCopyrightLine.js (revision e476780b824a2ae181e73844c4e7377c98b4af20) +++ b/main/chipper/js/grunt/getCopyrightLine.js (date 1627436740623) @@ -21,11 +21,11 @@ let startDate = ( await execute( 'git', [ 'log', '--diff-filter=A', '--follow', '--date=short', '--format=%cd', '-1', '--', relativeFile - ], `../${repo}` ) ).trim().split( '-' )[ 0 ]; + ], `../${repo}` ) ).stdout.trim().split( '-' )[ 0 ]; const endDate = ( await execute( 'git', [ 'log', '--follow', '--date=short', '--format=%cd', '-1', '--', relativeFile - ], `../${repo}` ) ).trim().split( '-' )[ 0 ]; + ], `../${repo}` ) ).stdout.trim().split( '-' )[ 0 ]; let dateString = ''; Index: main/perennial/js/common/getBranch.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/perennial/js/common/getBranch.js b/main/perennial/js/common/getBranch.js --- a/main/perennial/js/common/getBranch.js (revision e3618c303791070b3f3e38f52e57192e6975d3d3) +++ b/main/perennial/js/common/getBranch.js (date 1627436860780) @@ -17,5 +17,5 @@ * @returns {Promise} - Resolves to the branch name (or the empty string if not on a branch) */ module.exports = function( repo ) { - return execute( 'git', [ 'symbolic-ref', '-q', 'HEAD' ], `../${repo}` ).then( stdout => stdout.trim().replace( 'refs/heads/', '' ) ); + return execute( 'git', [ 'symbolic-ref', '-q', 'HEAD' ], `../${repo}` ).then( result => result.stdout.trim().replace( 'refs/heads/', '' ) ); }; Index: main/chipper/js/grunt/getDependencies.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/chipper/js/grunt/getDependencies.js b/main/chipper/js/grunt/getDependencies.js --- a/main/chipper/js/grunt/getDependencies.js (revision e476780b824a2ae181e73844c4e7377c98b4af20) +++ b/main/chipper/js/grunt/getDependencies.js (date 1627436740625) @@ -55,8 +55,8 @@ let branch = null; try { - sha = ( await execute( 'git', [ 'rev-parse', 'HEAD' ], `../${dependency}` ) ).trim(); - branch = ( await execute( 'git', [ 'rev-parse', '--abbrev-ref', 'HEAD' ], `../${dependency}` ) ).trim(); + sha = ( await execute( 'git', [ 'rev-parse', 'HEAD' ], `../${dependency}` ) ).stdout.trim(); + branch = ( await execute( 'git', [ 'rev-parse', '--abbrev-ref', 'HEAD' ], `../${dependency}` ) ).stdout.trim(); } catch( e ) { // We support repos that are not git repositories, see https://github.com/phetsims/chipper/issues/1011 ```

However, I cannot proceed confidently because there are too many occurrences that I have no familiarity with. For instance, there are 22 files with return execute( and all of these usage cases must be traced back to make sure return values and thrown errors are handled with the correct types. I would feel better if someone familiar with that side could take the lead or pair program during these changes. Checking many of these files, it seems @jonathanolson is the responsible developer. @jonathanolson can you please recommend how to proceed?