diff --git a/apps/heft/src/cli/HeftActionRunner.ts b/apps/heft/src/cli/HeftActionRunner.ts index d0cb2e60765..540b3a754b4 100644 --- a/apps/heft/src/cli/HeftActionRunner.ts +++ b/apps/heft/src/cli/HeftActionRunner.ts @@ -333,8 +333,8 @@ export class HeftActionRunner { executeAsync: (state: IWatchLoopState): Promise => { return this._executeOnceAsync(executionManager, state.abortSignal, state.requestRun); }, - onRequestRun: (requestor?: string) => { - terminal.writeLine(Colorize.bold(`New run requested by ${requestor || 'unknown task'}`)); + onRequestRun: (requestor: string) => { + terminal.writeLine(Colorize.bold(`New run requested by ${requestor}`)); }, onAbort: () => { terminal.writeLine(Colorize.bold(`Cancelling incremental build...`)); @@ -346,7 +346,7 @@ export class HeftActionRunner { private async _executeOnceAsync( executionManager: OperationExecutionManager, abortSignal: AbortSignal, - requestRun?: (requestor?: string) => void + requestRun?: (requestor: string) => void ): Promise { // Record this as the start of task execution. this._metricsCollector.setStartTime(); @@ -447,6 +447,7 @@ function _getOrCreatePhaseOperation( // Only create the operation. Dependencies are hooked up separately operation = new Operation({ groupName: phase.phaseName, + operationName: `${phase.phaseName} phase`, runner: new PhaseOperationRunner({ phase, internalHeftSession }) }); operations.set(key, operation); @@ -466,6 +467,7 @@ function _getOrCreateTaskOperation( if (!operation) { operation = new Operation({ groupName: task.parentPhase.phaseName, + operationName: `${task.taskName} task`, runner: new TaskOperationRunner({ internalHeftSession, task diff --git a/apps/heft/src/operations/runners/PhaseOperationRunner.ts b/apps/heft/src/operations/runners/PhaseOperationRunner.ts index f6f1ba3ed0d..7f760624fd3 100644 --- a/apps/heft/src/operations/runners/PhaseOperationRunner.ts +++ b/apps/heft/src/operations/runners/PhaseOperationRunner.ts @@ -23,7 +23,7 @@ export class PhaseOperationRunner implements IOperationRunner { private readonly _options: IPhaseOperationRunnerOptions; private _isClean: boolean = false; - public get name(): string { + public get operationName(): string { return `Phase ${JSON.stringify(this._options.phase.phaseName)}`; } diff --git a/apps/heft/src/operations/runners/TaskOperationRunner.ts b/apps/heft/src/operations/runners/TaskOperationRunner.ts index 66274a696a1..18c0143725d 100644 --- a/apps/heft/src/operations/runners/TaskOperationRunner.ts +++ b/apps/heft/src/operations/runners/TaskOperationRunner.ts @@ -55,7 +55,7 @@ export class TaskOperationRunner implements IOperationRunner { public readonly silent: boolean = false; - public get name(): string { + public get operationName(): string { const { taskName, parentPhase } = this._options.task; return `Task ${JSON.stringify(taskName)} of phase ${JSON.stringify(parentPhase.phaseName)}`; } diff --git a/common/changes/@rushstack/heft/octogonz-heft-issue-4467_2024-10-01-08-08.json b/common/changes/@rushstack/heft/octogonz-heft-issue-4467_2024-10-01-08-08.json new file mode 100644 index 00000000000..62c3633861c --- /dev/null +++ b/common/changes/@rushstack/heft/octogonz-heft-issue-4467_2024-10-01-08-08.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@rushstack/heft", + "comment": "Require a logging name for every operation.", + "type": "patch" + } + ], + "packageName": "@rushstack/heft" +} \ No newline at end of file diff --git a/common/changes/@rushstack/operation-graph/octogonz-heft-issue-4467_2024-10-01-08-08.json b/common/changes/@rushstack/operation-graph/octogonz-heft-issue-4467_2024-10-01-08-08.json new file mode 100644 index 00000000000..ddaadb5ac07 --- /dev/null +++ b/common/changes/@rushstack/operation-graph/octogonz-heft-issue-4467_2024-10-01-08-08.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@rushstack/operation-graph", + "comment": "(BREAKING API CHANGE) Require an operationName for every operation to improve logging.", + "type": "minor" + } + ], + "packageName": "@rushstack/operation-graph" +} \ No newline at end of file diff --git a/common/reviews/api/operation-graph.api.md b/common/reviews/api/operation-graph.api.md index e85f4bad9ed..4e835a1adef 100644 --- a/common/reviews/api/operation-graph.api.md +++ b/common/reviews/api/operation-graph.api.md @@ -33,7 +33,7 @@ export interface IExecuteOperationContext extends Omit Promise, priority: number): Promise; - requestRun?: (requestor?: string) => void; + requestRun?: (requestor: string) => void; terminal: ITerminal; } @@ -50,7 +50,7 @@ export interface IOperationExecutionOptions { // (undocumented) parallelism: number; // (undocumented) - requestRun?: (requestor?: string) => void; + requestRun?: (requestor: string) => void; // (undocumented) terminal: ITerminal; } @@ -58,7 +58,7 @@ export interface IOperationExecutionOptions { // @beta export interface IOperationOptions { groupName?: string | undefined; - name?: string | undefined; + operationName: string; runner?: IOperationRunner | undefined; weight?: number | undefined; } @@ -66,7 +66,7 @@ export interface IOperationOptions { // @beta export interface IOperationRunner { executeAsync(context: IOperationRunnerContext): Promise; - readonly name: string; + readonly operationName: string; silent: boolean; } @@ -99,7 +99,7 @@ export interface IRequestRunEventMessage { // (undocumented) event: 'requestRun'; // (undocumented) - requestor?: string; + requestor: string; } // @beta @@ -127,7 +127,7 @@ export interface IWatchLoopOptions { executeAsync: (state: IWatchLoopState) => Promise; onAbort: () => void; onBeforeExecute: () => void; - onRequestRun: (requestor?: string) => void; + onRequestRun: (requestor: string) => void; } // @beta @@ -135,12 +135,12 @@ export interface IWatchLoopState { // (undocumented) get abortSignal(): AbortSignal; // (undocumented) - requestRun: (requestor?: string) => void; + requestRun: (requestor: string) => void; } // @beta export class Operation implements IOperationStates { - constructor(options?: IOperationOptions); + constructor(options: IOperationOptions); // (undocumented) addDependency(dependency: Operation): void; readonly consumers: Set; @@ -152,7 +152,7 @@ export class Operation implements IOperationStates { _executeAsync(context: IExecuteOperationContext): Promise; readonly groupName: string | undefined; lastState: IOperationState | undefined; - readonly name: string | undefined; + readonly operationName: string; // (undocumented) reset(): void; runner: IOperationRunner | undefined; @@ -231,7 +231,7 @@ export class Stopwatch { export class WatchLoop implements IWatchLoopState { constructor(options: IWatchLoopOptions); get abortSignal(): AbortSignal; - requestRun: (requestor?: string) => void; + requestRun: (requestor: string) => void; runIPCAsync(host?: IPCHost): Promise; runUntilAbortedAsync(abortSignal: AbortSignal, onWaiting: () => void): Promise; runUntilStableAsync(abortSignal: AbortSignal): Promise; diff --git a/libraries/operation-graph/src/IOperationRunner.ts b/libraries/operation-graph/src/IOperationRunner.ts index 543257273c1..2590a4dafa5 100644 --- a/libraries/operation-graph/src/IOperationRunner.ts +++ b/libraries/operation-graph/src/IOperationRunner.ts @@ -81,7 +81,7 @@ export interface IOperationRunner { /** * Name of the operation, for logging. */ - readonly name: string; + readonly operationName: string; /** * Indicates that this runner is architectural and should not be reported on. diff --git a/libraries/operation-graph/src/Operation.ts b/libraries/operation-graph/src/Operation.ts index 167c7abcafb..c1cd78638c1 100644 --- a/libraries/operation-graph/src/Operation.ts +++ b/libraries/operation-graph/src/Operation.ts @@ -22,7 +22,7 @@ export interface IOperationOptions { /** * The name of this operation, for logging. */ - name?: string | undefined; + operationName: string; /** * The group that this operation belongs to. Will be used for logging and duration tracking. @@ -68,7 +68,7 @@ export interface IExecuteOperationContext extends Omit void; + requestRun?: (requestor: string) => void; /** * Terminal to write output to. @@ -101,7 +101,7 @@ export class Operation implements IOperationStates { /** * The name of this operation, for logging. */ - public readonly name: string | undefined; + public readonly operationName: string; /** * When the scheduler is ready to process this `Operation`, the `runner` implements the actual work of @@ -174,11 +174,11 @@ export class Operation implements IOperationStates { */ private _runPending: boolean = true; - public constructor(options?: IOperationOptions) { + public constructor(options: IOperationOptions) { this.groupName = options?.groupName; this.runner = options?.runner; this.weight = options?.weight || 1; - this.name = options?.name; + this.operationName = options.operationName; } public addDependency(dependency: Operation): void { @@ -280,7 +280,9 @@ export class Operation implements IOperationStates { // The requestRun callback is assumed to remain constant // throughout the lifetime of the process, so it is safe // to capture here. - return requestRun(this.name); + return requestRun(this.operationName); + case undefined: + throw new InternalError(`The operation state is undefined`); default: // This line is here to enforce exhaustiveness const currentStatus: undefined = this.state?.status; diff --git a/libraries/operation-graph/src/OperationExecutionManager.ts b/libraries/operation-graph/src/OperationExecutionManager.ts index f780802cd61..5788f8561fe 100644 --- a/libraries/operation-graph/src/OperationExecutionManager.ts +++ b/libraries/operation-graph/src/OperationExecutionManager.ts @@ -21,7 +21,7 @@ export interface IOperationExecutionOptions { parallelism: number; terminal: ITerminal; - requestRun?: (requestor?: string) => void; + requestRun?: (requestor: string) => void; } /** @@ -77,8 +77,8 @@ export class OperationExecutionManager { for (const dependency of consumer.dependencies) { if (!operations.has(dependency)) { throw new Error( - `Operation ${JSON.stringify(consumer.name)} declares a dependency on operation ` + - `${JSON.stringify(dependency.name)} that is not in the set of operations to execute.` + `Operation ${JSON.stringify(consumer.operationName)} declares a dependency on operation ` + + `${JSON.stringify(dependency.operationName)} that is not in the set of operations to execute.` ); } } diff --git a/libraries/operation-graph/src/OperationGroupRecord.ts b/libraries/operation-graph/src/OperationGroupRecord.ts index bb99ec38eca..a22eb85ceb8 100644 --- a/libraries/operation-graph/src/OperationGroupRecord.ts +++ b/libraries/operation-graph/src/OperationGroupRecord.ts @@ -54,7 +54,7 @@ export class OperationGroupRecord { public setOperationAsComplete(operation: Operation, state: IOperationState): void { if (!this._remainingOperations.has(operation)) { - throw new InternalError(`Operation ${operation.name} is not in the group ${this.name}`); + throw new InternalError(`Operation ${operation.operationName} is not in the group ${this.name}`); } if (state.status === OperationStatus.Aborted) { diff --git a/libraries/operation-graph/src/WatchLoop.ts b/libraries/operation-graph/src/WatchLoop.ts index ca911c32bf7..fb59f16a188 100644 --- a/libraries/operation-graph/src/WatchLoop.ts +++ b/libraries/operation-graph/src/WatchLoop.ts @@ -31,7 +31,7 @@ export interface IWatchLoopOptions { /** * Logging callback when a run is requested (and hasn't already been). */ - onRequestRun: (requestor?: string) => void; + onRequestRun: (requestor: string) => void; /** * Logging callback when a run is aborted. */ @@ -45,7 +45,7 @@ export interface IWatchLoopOptions { */ export interface IWatchLoopState { get abortSignal(): AbortSignal; - requestRun: (requestor?: string) => void; + requestRun: (requestor: string) => void; } /** @@ -60,7 +60,7 @@ export class WatchLoop implements IWatchLoopState { private _isRunning: boolean; private _runRequested: boolean; private _requestRunPromise: Promise; - private _resolveRequestRun!: (requestor?: string) => void; + private _resolveRequestRun!: (requestor: string) => void; public constructor(options: IWatchLoopOptions) { this._options = options; @@ -147,7 +147,7 @@ export class WatchLoop implements IWatchLoopState { } } - function requestRunFromHost(requestor?: string): void { + function requestRunFromHost(requestor: string): void { if (runRequestedFromHost) { return; } @@ -192,9 +192,12 @@ export class WatchLoop implements IWatchLoopState { try { status = await this.runUntilStableAsync(abortController.signal); - // ESLINT: "Promises must be awaited, end with a call to .catch, end with a call to .then ..." - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this._requestRunPromise.finally(requestRunFromHost); + this._requestRunPromise + // the reject callback in the promise is discarded so we ignore errors + .catch(() => {}) + .finally(() => { + requestRunFromHost('runIPCAsync'); + }); } catch (err) { status = OperationStatus.Failure; return reject(err); @@ -225,7 +228,7 @@ export class WatchLoop implements IWatchLoopState { /** * Requests that a new run occur. */ - public requestRun: (requestor?: string) => void = (requestor?: string) => { + public requestRun: (requestor: string) => void = (requestor: string) => { if (!this._runRequested) { this._options.onRequestRun(requestor); this._runRequested = true; diff --git a/libraries/operation-graph/src/calculateCriticalPath.ts b/libraries/operation-graph/src/calculateCriticalPath.ts index d00ba113457..c81872346f5 100644 --- a/libraries/operation-graph/src/calculateCriticalPath.ts +++ b/libraries/operation-graph/src/calculateCriticalPath.ts @@ -2,7 +2,7 @@ // See LICENSE in the project root for license information. export interface ISortableOperation> { - name: string | undefined; + operationName: string | undefined; criticalPathLength?: number | undefined; weight: number; consumers: Set; @@ -54,7 +54,9 @@ export function calculateShortestPath>( } if (!finalParent) { - throw new Error(`Could not find a path from "${startOperation.name}" to "${endOperation.name}"`); + throw new Error( + `Could not find a path from "${startOperation.operationName}" to "${endOperation.operationName}"` + ); } // Walk back up the path from the end operation to the start operation @@ -81,7 +83,7 @@ export function calculateCriticalPathLength>( throw new Error( 'A cyclic dependency was encountered:\n ' + - shortestPath.map((visitedTask) => visitedTask.name).join('\n -> ') + shortestPath.map((visitedTask) => visitedTask.operationName).join('\n -> ') ); } diff --git a/libraries/operation-graph/src/protocol.types.ts b/libraries/operation-graph/src/protocol.types.ts index 1c8b6293cb6..60100afca4d 100644 --- a/libraries/operation-graph/src/protocol.types.ts +++ b/libraries/operation-graph/src/protocol.types.ts @@ -10,7 +10,7 @@ import type { OperationStatus } from './OperationStatus'; */ export interface IRequestRunEventMessage { event: 'requestRun'; - requestor?: string; + requestor: string; } /** diff --git a/libraries/operation-graph/src/test/OperationExecutionManager.test.ts b/libraries/operation-graph/src/test/OperationExecutionManager.test.ts index e9230100a12..700e99d8c11 100644 --- a/libraries/operation-graph/src/test/OperationExecutionManager.test.ts +++ b/libraries/operation-graph/src/test/OperationExecutionManager.test.ts @@ -23,10 +23,10 @@ describe(OperationExecutionManager.name, () => { it('throws if a dependency is not in the set', () => { const alpha: Operation = new Operation({ - name: 'alpha' + operationName: 'alpha' }); const beta: Operation = new Operation({ - name: 'beta' + operationName: 'beta' }); alpha.addDependency(beta); @@ -38,10 +38,10 @@ describe(OperationExecutionManager.name, () => { it('sets critical path lengths', () => { const alpha: Operation = new Operation({ - name: 'alpha' + operationName: 'alpha' }); const beta: Operation = new Operation({ - name: 'beta' + operationName: 'beta' }); alpha.addDependency(beta); @@ -73,7 +73,7 @@ describe(OperationExecutionManager.name, () => { it('handles trivial input', async () => { const operation: Operation = new Operation({ - name: 'alpha' + operationName: 'alpha' }); const manager: OperationExecutionManager = new OperationExecutionManager(new Set([operation])); @@ -97,17 +97,17 @@ describe(OperationExecutionManager.name, () => { const runBeta: ExecuteAsyncMock = jest.fn(); const alpha: Operation = new Operation({ - name: 'alpha', + operationName: 'alpha', runner: { - name: 'alpha', + operationName: 'alpha', executeAsync: runAlpha, silent: false } }); const beta: Operation = new Operation({ - name: 'beta', + operationName: 'beta', runner: { - name: 'beta', + operationName: 'beta', executeAsync: runBeta, silent: false } @@ -149,17 +149,17 @@ describe(OperationExecutionManager.name, () => { const runBeta: ExecuteAsyncMock = jest.fn(); const alpha: Operation = new Operation({ - name: 'alpha', + operationName: 'alpha', runner: { - name: 'alpha', + operationName: 'alpha', executeAsync: runAlpha, silent: false } }); const beta: Operation = new Operation({ - name: 'beta', + operationName: 'beta', runner: { - name: 'beta', + operationName: 'beta', executeAsync: runBeta, silent: false } @@ -197,9 +197,9 @@ describe(OperationExecutionManager.name, () => { it('does not track noops', async () => { const operation: Operation = new Operation({ - name: 'alpha', + operationName: 'alpha', runner: { - name: 'alpha', + operationName: 'alpha', executeAsync(): Promise { return Promise.resolve(OperationStatus.NoOp); }, @@ -226,17 +226,17 @@ describe(OperationExecutionManager.name, () => { const runBeta: ExecuteAsyncMock = jest.fn(); const alpha: Operation = new Operation({ - name: 'alpha', + operationName: 'alpha', runner: { - name: 'alpha', + operationName: 'alpha', executeAsync: runAlpha, silent: false } }); const beta: Operation = new Operation({ - name: 'beta', + operationName: 'beta', runner: { - name: 'beta', + operationName: 'beta', executeAsync: runBeta, silent: false } @@ -297,17 +297,17 @@ describe(OperationExecutionManager.name, () => { ); const alpha: Operation = new Operation({ - name: 'alpha', + operationName: 'alpha', runner: { - name: 'alpha', + operationName: 'alpha', executeAsync: run, silent: false } }); const beta: Operation = new Operation({ - name: 'beta', + operationName: 'beta', runner: { - name: 'beta', + operationName: 'beta', executeAsync: run, silent: false } @@ -343,17 +343,17 @@ describe(OperationExecutionManager.name, () => { const requestRun: jest.Mock = jest.fn(); const alpha: Operation = new Operation({ - name: 'alpha', + operationName: 'alpha', runner: { - name: 'alpha', + operationName: 'alpha', executeAsync: runAlpha, silent: false } }); const beta: Operation = new Operation({ - name: 'beta', + operationName: 'beta', runner: { - name: 'beta', + operationName: 'beta', executeAsync: runBeta, silent: false } @@ -402,7 +402,7 @@ describe(OperationExecutionManager.name, () => { betaRequestRun!(); expect(requestRun).toHaveBeenCalledTimes(1); - expect(requestRun).toHaveBeenLastCalledWith(beta.name); + expect(requestRun).toHaveBeenLastCalledWith(beta.operationName); const terminalProvider2: StringBufferTerminalProvider = new StringBufferTerminalProvider(false); const terminal2: ITerminal = new Terminal(terminalProvider2); diff --git a/libraries/operation-graph/src/test/calculateCriticalPath.test.ts b/libraries/operation-graph/src/test/calculateCriticalPath.test.ts index bc45ffb6cea..5212fc86501 100644 --- a/libraries/operation-graph/src/test/calculateCriticalPath.test.ts +++ b/libraries/operation-graph/src/test/calculateCriticalPath.test.ts @@ -20,7 +20,7 @@ function createGraph( if (weights) { for (const [name, weight] of weights) { nodes.set(name, { - name, + operationName: name, weight, consumers: new Set() }); @@ -31,7 +31,7 @@ function createGraph( let node: ITestOperation | undefined = nodes.get(name); if (!node) { node = { - name, + operationName: name, weight: 1, consumers: new Set() }; @@ -60,22 +60,22 @@ describe(calculateShortestPath.name, () => { ]); const result1: ITestOperation[] = calculateShortestPath(graph.get('a')!, graph.get('f')!); - expect(result1.map((x) => x.name)).toMatchSnapshot('long'); + expect(result1.map((x) => x.operationName)).toMatchSnapshot('long'); graph.get('c')!.consumers.add(graph.get('a')!); const result2: ITestOperation[] = calculateShortestPath(graph.get('a')!, graph.get('f')!); - expect(result2.map((x) => x.name)).toMatchSnapshot('with shortcut'); + expect(result2.map((x) => x.operationName)).toMatchSnapshot('with shortcut'); graph.get('f')!.consumers.add(graph.get('c')!); const result3: ITestOperation[] = calculateShortestPath(graph.get('a')!, graph.get('f')!); - expect(result3.map((x) => x.name)).toMatchSnapshot('with multiple shortcuts'); + expect(result3.map((x) => x.operationName)).toMatchSnapshot('with multiple shortcuts'); graph.get('a')!.consumers.add(graph.get('f')!); const result4: ITestOperation[] = calculateShortestPath(graph.get('a')!, graph.get('a')!); - expect(result4.map((x) => x.name)).toMatchSnapshot('with multiple shortcuts (circular)'); + expect(result4.map((x) => x.operationName)).toMatchSnapshot('with multiple shortcuts (circular)'); }); });