From df1d08c364650757c7dc10cfb618ea004996fcac Mon Sep 17 00:00:00 2001 From: David Michon Date: Wed, 15 May 2024 23:08:27 +0000 Subject: [PATCH] [rush] Improve error reporting --- .../fix-completed-count_2024-05-15-21-45.json | 10 ++++ .../fix-completed-count_2024-05-15-22-44.json | 10 ++++ .../fix-completed-count_2024-05-15-22-44.json | 10 ++++ common/reviews/api/terminal.api.md | 2 +- .../operations/OperationExecutionManager.ts | 50 ++++++++++++------- .../operations/OperationExecutionRecord.ts | 5 +- .../logic/operations/ShellOperationRunner.ts | 10 ++-- libraries/terminal/src/StdioSummarizer.ts | 6 +-- 8 files changed, 77 insertions(+), 26 deletions(-) create mode 100644 common/changes/@microsoft/rush/fix-completed-count_2024-05-15-21-45.json create mode 100644 common/changes/@microsoft/rush/fix-completed-count_2024-05-15-22-44.json create mode 100644 common/changes/@rushstack/terminal/fix-completed-count_2024-05-15-22-44.json diff --git a/common/changes/@microsoft/rush/fix-completed-count_2024-05-15-21-45.json b/common/changes/@microsoft/rush/fix-completed-count_2024-05-15-21-45.json new file mode 100644 index 0000000000..51168342f5 --- /dev/null +++ b/common/changes/@microsoft/rush/fix-completed-count_2024-05-15-21-45.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@microsoft/rush", + "comment": "Fix count of completed operations when silent operations are blocked. Add explicit message for child processes terminated by signals. Ensure that errors show up in summarized view.", + "type": "none" + } + ], + "packageName": "@microsoft/rush" +} \ No newline at end of file diff --git a/common/changes/@microsoft/rush/fix-completed-count_2024-05-15-22-44.json b/common/changes/@microsoft/rush/fix-completed-count_2024-05-15-22-44.json new file mode 100644 index 0000000000..a7cd4492a8 --- /dev/null +++ b/common/changes/@microsoft/rush/fix-completed-count_2024-05-15-22-44.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@microsoft/rush", + "comment": "Ensure that errors thrown in afterExecuteOperation show up in the summary at the end of the build.", + "type": "none" + } + ], + "packageName": "@microsoft/rush" +} \ No newline at end of file diff --git a/common/changes/@rushstack/terminal/fix-completed-count_2024-05-15-22-44.json b/common/changes/@rushstack/terminal/fix-completed-count_2024-05-15-22-44.json new file mode 100644 index 0000000000..a5057e4d97 --- /dev/null +++ b/common/changes/@rushstack/terminal/fix-completed-count_2024-05-15-22-44.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@rushstack/terminal", + "comment": "Allow use of 'preventAutoclose' flag in StdioSummarizer.", + "type": "minor" + } + ], + "packageName": "@rushstack/terminal" +} \ No newline at end of file diff --git a/common/reviews/api/terminal.api.md b/common/reviews/api/terminal.api.md index 917bfd6491..906946ed61 100644 --- a/common/reviews/api/terminal.api.md +++ b/common/reviews/api/terminal.api.md @@ -158,7 +158,7 @@ export interface IStdioLineTransformOptions extends ITerminalTransformOptions { } // @beta -export interface IStdioSummarizerOptions { +export interface IStdioSummarizerOptions extends ITerminalWritableOptions { leadingLines?: number; trailingLines?: number; } diff --git a/libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts b/libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts index 652a56b58d..4e20937097 100644 --- a/libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts +++ b/libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts @@ -6,7 +6,8 @@ import { StdioWritable, TextRewriterTransform, Colorize, - ConsoleTerminalProvider + ConsoleTerminalProvider, + TerminalChunkKind } from '@rushstack/terminal'; import { StreamCollator, type CollatedTerminal, type CollatedWriter } from '@rushstack/stream-collator'; import { NewlineKind, Async, InternalError, AlreadyReportedError } from '@rushstack/node-core-library'; @@ -236,13 +237,9 @@ export class OperationExecutionManager { try { await this._afterExecuteOperation?.(record); } catch (e) { - // Failed operations get reported here - const message: string | undefined = record.error?.message; - if (message) { - this._terminal.writeStderrLine('Unhandled exception: '); - this._terminal.writeStderrLine(message); - } - throw e; + this._reportOperationErrorIfAny(record); + record.error = e; + record.status = OperationStatus.Failure; } this._onOperationComplete(record); }; @@ -298,6 +295,27 @@ export class OperationExecutionManager { }; } + private _reportOperationErrorIfAny(record: OperationExecutionRecord): void { + // Failed operations get reported, even if silent. + // Generally speaking, silent operations shouldn't be able to fail, so this is a safety measure. + let message: string | undefined = undefined; + if (record.error) { + if (!(record.error instanceof AlreadyReportedError)) { + message = record.error.message; + } + } + + if (message) { + // This creates the writer, so don't do this until needed + record.collatedWriter.terminal.writeStderrLine(message); + // Ensure that the error message, if present, shows up in the summary + record.stdioSummarizer.writeChunk({ + text: `${message}\n`, + kind: TerminalChunkKind.Stderr + }); + } + } + /** * Handles the result of the operation and propagates any relevant effects. */ @@ -313,18 +331,10 @@ export class OperationExecutionManager { case OperationStatus.Failure: { // Failed operations get reported, even if silent. // Generally speaking, silent operations shouldn't be able to fail, so this is a safety measure. - let message: string | undefined = undefined; - if (record.error) { - if (!(record.error instanceof AlreadyReportedError)) { - message = record.error.message; - } - } + this._reportOperationErrorIfAny(record); // This creates the writer, so don't do this globally const { terminal } = record.collatedWriter; - if (message) { - terminal.writeStderrLine(message); - } terminal.writeStderrLine(Colorize.red(`"${name}" failed to build.`)); const blockedQueue: Set = new Set(record.consumers); @@ -339,7 +349,11 @@ export class OperationExecutionManager { blockedRecord.status = OperationStatus.Blocked; this._executionQueue.complete(blockedRecord); - this._completedOperations++; + if (!blockedRecord.runner.silent) { + // Only increment the count if the operation is not silent to avoid confusing the user. + // The displayed total is the count of non-silent operations. + this._completedOperations++; + } for (const dependent of blockedRecord.consumers) { blockedQueue.add(dependent); diff --git a/libraries/rush-lib/src/logic/operations/OperationExecutionRecord.ts b/libraries/rush-lib/src/logic/operations/OperationExecutionRecord.ts index e9e172c63b..3d2ceb7ee0 100644 --- a/libraries/rush-lib/src/logic/operations/OperationExecutionRecord.ts +++ b/libraries/rush-lib/src/logic/operations/OperationExecutionRecord.ts @@ -93,7 +93,10 @@ export class OperationExecutionRecord implements IOperationRunnerContext { public readonly consumers: Set = new Set(); public readonly stopwatch: Stopwatch = new Stopwatch(); - public readonly stdioSummarizer: StdioSummarizer = new StdioSummarizer(); + public readonly stdioSummarizer: StdioSummarizer = new StdioSummarizer({ + // Allow writing to this object after transforms have been closed. We clean it up manually in a finally block. + preventAutoclose: true + }); public readonly runner: IOperationRunner; public readonly associatedPhase: IPhase | undefined; diff --git a/libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts b/libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts index 848bca33bf..fedafcc024 100644 --- a/libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts +++ b/libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts @@ -99,10 +99,13 @@ export class ShellOperationRunner implements IOperationRunner { const status: OperationStatus = await new Promise( (resolve: (status: OperationStatus) => void, reject: (error: OperationError) => void) => { - subProcess.on('close', (code: number) => { + subProcess.on('close', (code: number, signal?: string) => { try { - if (code !== 0) { - // Do NOT reject here immediately, give a chance for other logic to suppress the error + // Do NOT reject here immediately, give a chance for other logic to suppress the error + if (signal) { + context.error = new OperationError('error', `Terminated by signal: ${signal}`); + resolve(OperationStatus.Failure); + } else if (code !== 0) { context.error = new OperationError('error', `Returned error code: ${code}`); resolve(OperationStatus.Failure); } else if (hasWarningOrError) { @@ -111,6 +114,7 @@ export class ShellOperationRunner implements IOperationRunner { resolve(OperationStatus.Success); } } catch (error) { + context.error = error as OperationError; reject(error as OperationError); } }); diff --git a/libraries/terminal/src/StdioSummarizer.ts b/libraries/terminal/src/StdioSummarizer.ts index ec9380dc1e..12c3bf18c5 100644 --- a/libraries/terminal/src/StdioSummarizer.ts +++ b/libraries/terminal/src/StdioSummarizer.ts @@ -2,13 +2,13 @@ // See LICENSE in the project root for license information. import { type ITerminalChunk, TerminalChunkKind } from './ITerminalChunk'; -import { TerminalWritable } from './TerminalWritable'; +import { type ITerminalWritableOptions, TerminalWritable } from './TerminalWritable'; /** * Constructor options for {@link StdioSummarizer}. * @beta */ -export interface IStdioSummarizerOptions { +export interface IStdioSummarizerOptions extends ITerminalWritableOptions { /** * Specifies the maximum number of leading lines to include in the summary. * @defaultValue `10` @@ -62,7 +62,7 @@ export class StdioSummarizer extends TerminalWritable { private _abridgedStderr: boolean; public constructor(options?: IStdioSummarizerOptions) { - super(); + super(options); if (!options) { options = {};