diff --git a/src/cli.ts b/src/cli.ts index 4ef156c7a..2232e8454 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -100,7 +100,7 @@ const run = async (): Promise> => { options.failureMode, abort ); - const result = await executor.execute(config.value); + const result = await executor.getExecution(config.value).execute(); if (!result.ok) { return result; } diff --git a/src/execution/base.ts b/src/execution/base.ts index 61ea7304e..946411aa3 100644 --- a/src/execution/base.ts +++ b/src/execution/base.ts @@ -29,30 +29,41 @@ export type FailureMode = 'no-new' | 'continue' | 'kill'; * A single execution of a specific script. */ export abstract class BaseExecution { - protected readonly script: T; - protected readonly executor: Executor; - protected readonly logger: Logger; + protected readonly _config: T; + protected readonly _executor: Executor; + protected readonly _logger: Logger; + private _fingerprint?: Promise; - protected constructor(script: T, executor: Executor, logger: Logger) { - this.script = script; - this.executor = executor; - this.logger = logger; + constructor(config: T, executor: Executor, logger: Logger) { + this._config = config; + this._executor = executor; + this._logger = logger; } + /** + * Execute this script and return its fingerprint. Cached, so safe to call + * multiple times. + */ + execute(): Promise { + return (this._fingerprint ??= this._execute()); + } + + protected abstract _execute(): Promise; + /** * Execute all of this script's dependencies. */ - protected async executeDependencies(): Promise< + protected async _executeDependencies(): Promise< Result, Failure[]> > { // Randomize the order we execute dependencies to make it less likely for a // user to inadvertently depend on any specific order, which could indicate // a missing edge in the dependency graph. - shuffle(this.script.dependencies); + shuffle(this._config.dependencies); const dependencyResults = await Promise.all( - this.script.dependencies.map((dependency) => { - return this.executor.execute(dependency.config); + this._config.dependencies.map((dependency) => { + return this._executor.getExecution(dependency.config).execute(); }) ); const results: Array<[ScriptReference, Fingerprint]> = []; @@ -64,7 +75,7 @@ export abstract class BaseExecution { errors.add(error); } } else { - results.push([this.script.dependencies[i].config, result.value]); + results.push([this._config.dependencies[i].config, result.value]); } } if (errors.size > 0) { diff --git a/src/execution/no-command.ts b/src/execution/no-command.ts index b96ace551..5bbe2f502 100644 --- a/src/execution/no-command.ts +++ b/src/execution/no-command.ts @@ -8,33 +8,23 @@ import {BaseExecution} from './base.js'; import {Fingerprint} from '../fingerprint.js'; import type {ExecutionResult} from './base.js'; -import type {Executor} from '../executor.js'; import type {NoCommandScriptConfig} from '../config.js'; -import type {Logger} from '../logging/logger.js'; /** * Execution for a {@link NoCommandScriptConfig}. */ export class NoCommandScriptExecution extends BaseExecution { - static execute( - script: NoCommandScriptConfig, - executor: Executor, - logger: Logger - ): Promise { - return new NoCommandScriptExecution(script, executor, logger)._execute(); - } - - private async _execute(): Promise { - const dependencyFingerprints = await this.executeDependencies(); + protected override async _execute(): Promise { + const dependencyFingerprints = await this._executeDependencies(); if (!dependencyFingerprints.ok) { return dependencyFingerprints; } const fingerprint = await Fingerprint.compute( - this.script, + this._config, dependencyFingerprints.value ); - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'success', reason: 'no-command', }); diff --git a/src/execution/service.ts b/src/execution/service.ts index 20311f624..b2e91dfec 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -8,29 +8,23 @@ import {BaseExecution} from './base.js'; import {Fingerprint} from '../fingerprint.js'; import type {ExecutionResult} from './base.js'; -import type {Executor} from '../executor.js'; import type {ServiceScriptConfig} from '../config.js'; -import type {Logger} from '../logging/logger.js'; /** * Execution for a {@link ServiceScriptConfig}. */ export class ServiceScriptExecution extends BaseExecution { - static execute( - script: ServiceScriptConfig, - executor: Executor, - logger: Logger - ): Promise { - return new ServiceScriptExecution(script, executor, logger)._execute(); - } - - private async _execute(): Promise { - const dependencyFingerprints = await this.executeDependencies(); + /** + * Note `execute` is a bit of a misnomer here, because we don't actually + * execute the command at this stage in the case of services. + */ + protected override async _execute(): Promise { + const dependencyFingerprints = await this._executeDependencies(); if (!dependencyFingerprints.ok) { return dependencyFingerprints; } const fingerprint = await Fingerprint.compute( - this.script, + this._config, dependencyFingerprints.value ); return {ok: true, value: fingerprint}; diff --git a/src/execution/standard.ts b/src/execution/standard.ts index c9ff94d86..adaef1acc 100644 --- a/src/execution/standard.ts +++ b/src/execution/standard.ts @@ -37,34 +37,18 @@ type StandardScriptExecutionState = * Execution for a {@link StandardScriptConfig}. */ export class StandardScriptExecution extends BaseExecution { - static execute( - script: StandardScriptConfig, - executor: Executor, - workerPool: WorkerPool, - cache: Cache | undefined, - logger: Logger - ): Promise { - return new StandardScriptExecution( - script, - executor, - workerPool, - cache, - logger - )._execute(); - } - private _state: StandardScriptExecutionState = 'before-running'; private readonly _cache?: Cache; private readonly _workerPool: WorkerPool; - private constructor( - script: StandardScriptConfig, + constructor( + config: StandardScriptConfig, executor: Executor, workerPool: WorkerPool, cache: Cache | undefined, logger: Logger ) { - super(script, executor, logger); + super(config, executor, logger); this._workerPool = workerPool; this._cache = cache; } @@ -75,10 +59,10 @@ export class StandardScriptExecution extends BaseExecution } } - private async _execute(): Promise { + protected async _execute(): Promise { this._ensureState('before-running'); - const dependencyFingerprints = await this.executeDependencies(); + const dependencyFingerprints = await this._executeDependencies(); if (!dependencyFingerprints.ok) { dependencyFingerprints.error.push(this._startCancelledEvent); return dependencyFingerprints; @@ -95,7 +79,7 @@ export class StandardScriptExecution extends BaseExecution // cache key, because a dependency could create or modify an input file to // this script, which would affect the key. const fingerprint = await Fingerprint.compute( - this.script, + this._config, dependencyFingerprints.value ); if (await this._fingerprintIsFresh(fingerprint)) { @@ -116,7 +100,7 @@ export class StandardScriptExecution extends BaseExecution } const cacheHit = fingerprint.data.fullyTracked - ? await this._cache?.get(this.script, fingerprint) + ? await this._cache?.get(this._config, fingerprint) : undefined; if (this._shouldNotStart) { return {ok: false, error: [this._startCancelledEvent]}; @@ -136,7 +120,7 @@ export class StandardScriptExecution extends BaseExecution * significant amount of time might have elapsed. */ private get _shouldNotStart(): boolean { - return this.executor.shouldStopStartingNewScripts; + return this._executor.shouldStopStartingNewScripts; } /** @@ -144,7 +128,7 @@ export class StandardScriptExecution extends BaseExecution */ private get _startCancelledEvent(): StartCancelled { return { - script: this.script, + script: this._config, type: 'failure', reason: 'start-cancelled', }; @@ -157,7 +141,7 @@ export class StandardScriptExecution extends BaseExecution private async _acquireSystemLockIfNeeded( workFn: () => Promise ): Promise { - if (this.script.output?.values.length === 0) { + if (this._config.output?.values.length === 0) { return workFn(); } @@ -203,8 +187,8 @@ export class StandardScriptExecution extends BaseExecution if ((error as {code: string}).code === 'ELOCKED') { if (!loggedLocked) { // Only log this once. - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'info', detail: 'locked', }); @@ -240,8 +224,8 @@ export class StandardScriptExecution extends BaseExecution * Handle the outcome where the script is already fresh. */ private _handleFresh(fingerprint: Fingerprint): ExecutionResult { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'success', reason: 'fresh', }); @@ -285,8 +269,8 @@ export class StandardScriptExecution extends BaseExecution } await writeFingerprintPromise; - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'success', reason: 'cached', }); @@ -325,8 +309,8 @@ export class StandardScriptExecution extends BaseExecution } this._state = 'running'; - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'info', detail: 'running', }); @@ -334,16 +318,16 @@ export class StandardScriptExecution extends BaseExecution const child = new ScriptChildProcess( // Unfortunately TypeScript doesn't automatically narrow this type // based on the undefined-command check we did just above. - this.script + this._config ); - void this.executor.shouldKillRunningScripts.then(() => { + void this._executor.shouldKillRunningScripts.then(() => { child.kill(); }); child.stdout.on('data', (data: string | Buffer) => { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'output', stream: 'stdout', data, @@ -351,8 +335,8 @@ export class StandardScriptExecution extends BaseExecution }); child.stderr.on('data', (data: string | Buffer) => { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'output', stream: 'stderr', data, @@ -361,8 +345,8 @@ export class StandardScriptExecution extends BaseExecution const result = await child.completed; if (result.ok) { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'success', reason: 'exit-zero', }); @@ -378,7 +362,7 @@ export class StandardScriptExecution extends BaseExecution // By directly notifying the Executor about the failure while we are // still inside the WorkerPool callback, we prevent this race // condition. - this.executor.notifyFailure(); + this._executor.notifyFailure(); } return result; }); @@ -412,7 +396,7 @@ export class StandardScriptExecution extends BaseExecution } private async _shouldClean(fingerprint: Fingerprint) { - const cleanValue = this.script.clean; + const cleanValue = this._config.clean; switch (cleanValue) { case true: { return true; @@ -479,7 +463,7 @@ export class StandardScriptExecution extends BaseExecution if (paths.value === undefined) { return {ok: true, value: undefined}; } - await this._cache.set(this.script, fingerprint, paths.value); + await this._cache.set(this._config, fingerprint, paths.value); return {ok: true, value: undefined}; } @@ -518,14 +502,14 @@ export class StandardScriptExecution extends BaseExecution private async _globOutputFiles(): Promise< Result > { - if (this.script.output === undefined) { + if (this._config.output === undefined) { return {ok: true, value: undefined}; } try { return { ok: true, - value: await glob(this.script.output.values, { - cwd: this.script.packageDir, + value: await glob(this._config.output.values, { + cwd: this._config.packageDir, followSymlinks: false, includeDirectories: true, expandDirectories: true, @@ -542,15 +526,15 @@ export class StandardScriptExecution extends BaseExecution error: { type: 'failure', reason: 'invalid-config-syntax', - script: this.script, + script: this._config, diagnostic: { severity: 'error', message: `Output files must be within the package: ${error.message}`, location: { - file: this.script.declaringFile, + file: this._config.declaringFile, range: { - offset: this.script.output.node.offset, - length: this.script.output.node.length, + offset: this._config.output.node.offset, + length: this._config.output.node.length, }, }, }, @@ -565,7 +549,7 @@ export class StandardScriptExecution extends BaseExecution * Get the directory name where Wireit data can be saved for this script. */ private get _dataDir(): string { - return getScriptDataDir(this.script); + return getScriptDataDir(this._config); } /** @@ -676,8 +660,8 @@ export class StandardScriptExecution extends BaseExecution } const equal = newManifest === oldManifest; if (!equal) { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'info', detail: 'output-modified', }); diff --git a/src/executor.ts b/src/executor.ts index b4a0cdc63..0edcc73d0 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -7,14 +7,31 @@ import {NoCommandScriptExecution} from './execution/no-command.js'; import {StandardScriptExecution} from './execution/standard.js'; import {ServiceScriptExecution} from './execution/service.js'; -import {ScriptConfig, scriptReferenceToString} from './config.js'; +import {ScriptReferenceString, scriptReferenceToString} from './config.js'; import {WorkerPool} from './util/worker-pool.js'; import {Deferred} from './util/deferred.js'; -import {convertExceptionToFailure} from './error.js'; -import type {ExecutionResult} from './execution/base.js'; import type {Logger} from './logging/logger.js'; import type {Cache} from './caching/cache.js'; +import type { + ScriptConfig, + NoCommandScriptConfig, + ServiceScriptConfig, + StandardScriptConfig, +} from './config.js'; + +type Execution = + | NoCommandScriptExecution + | StandardScriptExecution + | ServiceScriptExecution; + +type ConfigToExecution = T extends NoCommandScriptConfig + ? NoCommandScriptExecution + : T extends StandardScriptConfig + ? StandardScriptExecution + : T extends ServiceScriptConfig + ? ServiceScriptExecution + : never; /** * What to do when a script failure occurs: @@ -30,7 +47,7 @@ export type FailureMode = 'no-new' | 'continue' | 'kill'; * Executes a script that has been analyzed and validated by the Analyzer. */ export class Executor { - private readonly _executions = new Map>(); + private readonly _executions = new Map(); private readonly _logger: Logger; private readonly _workerPool: WorkerPool; private readonly _cache?: Cache; @@ -112,38 +129,32 @@ export class Executor { return this._killRunningScripts.promise; } - async execute(script: ScriptConfig): Promise { - const executionKey = scriptReferenceToString(script); - let promise = this._executions.get(executionKey); - if (promise === undefined) { - promise = this._executeAccordingToKind(script) - .catch((error) => convertExceptionToFailure(error, script)) - .then((result) => { - if (!result.ok) { - this.notifyFailure(); - } - return result; - }); - this._executions.set(executionKey, promise); - } - return promise; - } - - private _executeAccordingToKind( - script: ScriptConfig - ): Promise { - if (script.command === undefined) { - return NoCommandScriptExecution.execute(script, this, this._logger); - } - if (script.service) { - return ServiceScriptExecution.execute(script, this, this._logger); + /** + * Get the execution instance for a script config, creating one if it doesn't + * already exist. + */ + getExecution(config: T): ConfigToExecution { + const key = scriptReferenceToString(config); + let execution = this._executions.get(key); + if (execution === undefined) { + if (config.command === undefined) { + execution = new NoCommandScriptExecution(config, this, this._logger); + } else if (config.service) { + execution = new ServiceScriptExecution(config, this, this._logger); + } else { + execution = new StandardScriptExecution( + config, + this, + this._workerPool, + this._cache, + this._logger + ); + } + this._executions.set(key, execution); } - return StandardScriptExecution.execute( - script, - this, - this._workerPool, - this._cache, - this._logger - ); + // Cast needed because our Map type doesn't know about the config -> + // execution type guarantees. We could make a smarter Map type, but not + // really worth it here. + return execution as ConfigToExecution; } } diff --git a/src/test/cache-github.test.ts b/src/test/cache-github.test.ts index d8060d1ec..531bbd4c5 100644 --- a/src/test/cache-github.test.ts +++ b/src/test/cache-github.test.ts @@ -20,9 +20,13 @@ const __filename = fileURLToPath(import.meta.url); const __dirname = pathlib.dirname(__filename); const repoRoot = pathlib.resolve(__dirname, '..', '..'); -const SELF_SIGNED_CERT = selfsigned.generate([ - {name: 'commonName', value: 'localhost'}, -]); +const SELF_SIGNED_CERT = selfsigned.generate( + [{name: 'commonName', value: 'localhost'}], + // More recent versions of TLS require a larger minimum key size than the + // default of this library (1024). Let's also upgrade from sha1 to sha256 + // while we're at it. + {keySize: 2048, algorithm: 'sha256'} +); const SELF_SIGNED_CERT_PATH = pathlib.resolve( repoRoot, 'temp', diff --git a/src/test/util/filesystem-test-rig.ts b/src/test/util/filesystem-test-rig.ts index 732039aed..bd9d551ba 100644 --- a/src/test/util/filesystem-test-rig.ts +++ b/src/test/util/filesystem-test-rig.ts @@ -21,7 +21,7 @@ export class FilesystemTestRig { readonly temp = pathlib.resolve(repoRoot, 'temp', String(Math.random())); private _state: 'uninitialized' | 'running' | 'done' = 'uninitialized'; - protected assertState(expected: 'uninitialized' | 'running' | 'done') { + protected _assertState(expected: 'uninitialized' | 'running' | 'done') { if (this._state !== expected) { throw new Error( `Expected state to be ${expected} but was ${this._state}` @@ -33,7 +33,7 @@ export class FilesystemTestRig { * Initialize the temporary filesystem. */ async setup() { - this.assertState('uninitialized'); + this._assertState('uninitialized'); this._state = 'running'; await this.mkdir('.'); } @@ -42,7 +42,7 @@ export class FilesystemTestRig { * Delete the temporary filesystem. */ async cleanup(): Promise { - this.assertState('running'); + this._assertState('running'); await this.delete('.'); this._state = 'done'; } @@ -66,7 +66,7 @@ export class FilesystemTestRig { fileOrFiles: string | {[filename: string]: unknown}, data?: string ): Promise { - this.assertState('running'); + this._assertState('running'); if (typeof fileOrFiles === 'string') { const absolute = pathlib.resolve(this.temp, fileOrFiles); await fs.mkdir(pathlib.dirname(absolute), {recursive: true}); @@ -92,7 +92,7 @@ export class FilesystemTestRig { fileOrFiles: string | {[filename: string]: unknown}, data?: string ): Promise { - this.assertState('running'); + this._assertState('running'); if (typeof fileOrFiles === 'string') { const actual = pathlib.resolve(this.temp, fileOrFiles); const temp = actual + '.tmp'; @@ -125,7 +125,7 @@ export class FilesystemTestRig { * Read a file from the temporary filesystem. */ async read(filename: string): Promise { - this.assertState('running'); + this._assertState('running'); return fs.readFile(this.resolve(filename), 'utf8'); } @@ -133,7 +133,7 @@ export class FilesystemTestRig { * Check whether a file exists in the temporary filesystem. */ async exists(filename: string): Promise { - this.assertState('running'); + this._assertState('running'); try { await fs.access(this.resolve(filename)); return true; @@ -149,7 +149,7 @@ export class FilesystemTestRig { * Get filesystem metadata for the given path in the temporary filesystem. */ async lstat(path: string): Promise { - this.assertState('running'); + this._assertState('running'); return fs.lstat(this.resolve(path)); } @@ -158,7 +158,7 @@ export class FilesystemTestRig { * Return false if it is another kind of file, or if it doesn't exit. */ async isDirectory(path: string): Promise { - this.assertState('running'); + this._assertState('running'); try { const stats = await this.lstat(path); return stats.isDirectory(); @@ -176,7 +176,7 @@ export class FilesystemTestRig { * or undefined if it doesn't exist. */ async readlink(path: string): Promise { - this.assertState('running'); + this._assertState('running'); try { return await fs.readlink(this.resolve(path)); } catch (error) { @@ -193,7 +193,7 @@ export class FilesystemTestRig { * directories. */ async mkdir(dirname: string): Promise { - this.assertState('running'); + this._assertState('running'); await fs.mkdir(this.resolve(dirname), {recursive: true}); } @@ -201,7 +201,7 @@ export class FilesystemTestRig { * Delete a file or directory in the temporary filesystem. */ async delete(filename: string): Promise { - this.assertState('running'); + this._assertState('running'); await fs.rm(this.resolve(filename), {force: true, recursive: true}); } @@ -213,7 +213,7 @@ export class FilesystemTestRig { filename: string, windowsType: 'file' | 'dir' | 'junction' ): Promise { - this.assertState('running'); + this._assertState('running'); const absolute = this.resolve(filename); try { await fs.unlink(absolute); diff --git a/src/test/util/test-rig.ts b/src/test/util/test-rig.ts index 0a504b8ca..997bb170e 100644 --- a/src/test/util/test-rig.ts +++ b/src/test/util/test-rig.ts @@ -38,7 +38,7 @@ export class WireitTestRig extends FilesystemTestRig { * Initialize the temporary filesystem, and set up the wireit binary to be * runnable as though it had been installed there through npm. */ - async setup() { + override async setup() { await super.setup(); const absWireitBinaryPath = pathlib.resolve(repoRoot, 'bin', 'wireit.js'); const absWireitTempInstallPath = pathlib.resolve( @@ -78,7 +78,7 @@ export class WireitTestRig extends FilesystemTestRig { binaryPath: string; installPath: string; }) { - this.assertState('running'); + this._assertState('running'); binaryPath = this._resolve(binaryPath); installPath = this._resolve(installPath); @@ -110,7 +110,7 @@ export class WireitTestRig extends FilesystemTestRig { /** * Delete the temporary filesystem and perform other cleanup. */ - async cleanup(): Promise { + override async cleanup(): Promise { await Promise.all(this._commands.map((command) => command.close())); for (const child of this._activeChildProcesses) { child.kill(); @@ -130,7 +130,7 @@ export class WireitTestRig extends FilesystemTestRig { command: string, opts?: {cwd?: string; env?: Record} ): ExecResult { - this.assertState('running'); + this._assertState('running'); const cwd = this._resolve(opts?.cwd ?? '.'); const result = new ExecResult(command, cwd, { // We hard code the parallelism here because by default we infer a value @@ -180,7 +180,7 @@ export class WireitTestRig extends FilesystemTestRig { * Create a new test command. */ async newCommand(): Promise { - this.assertState('running'); + this._assertState('running'); // On Windows, Node IPC is implemented with named pipes, which must be // prefixed by "\\?\pipe\". On Linux/macOS it's a unix domain socket, which // can be any filepath. See https://nodejs.org/api/net.html#ipc-support for diff --git a/src/watcher.ts b/src/watcher.ts index 0283767c4..f5290336e 100644 --- a/src/watcher.ts +++ b/src/watcher.ts @@ -282,7 +282,7 @@ export class Watcher { this._failureMode, this._abort ); - const result = await executor.execute(script); + const result = await executor.getExecution(script).execute(); if (!result.ok) { for (const error of result.error) { this._logger.log(error); diff --git a/tsconfig.json b/tsconfig.json index b7b3ced3e..b3b5b8a87 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -15,6 +15,7 @@ "forceConsistentCasingInFileNames": true, "allowSyntheticDefaultImports": true, "useUnknownInCatchVariables": true, + "noImplicitOverride": true, "incremental": true, "tsBuildInfoFile": ".tsbuildinfo", "composite": true