Skip to content

Commit d0b59cd

Browse files
iclantoniclanton
authored
Merge pull request microsoft#1314 from microsoft/ianc/rush-fail-on-warnings
[rush] Rush should return a nonzero exit code if warnings are encountered during a build
2 parents 04547ea + 30de8f4 commit d0b59cd

11 files changed

Lines changed: 243 additions & 67 deletions

File tree

‎apps/rush-lib/assets/rush-init/common/config/rush/command-line.json‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,29 @@
7878
* the custom command name. To disable this check, set "ignoreMissingScript" to true;
7979
* projects with a missing definition will be skipped.
8080
*/
81-
"ignoreMissingScript": false
81+
"ignoreMissingScript": false,
82+
83+
/**
84+
* When invoking shell scripts, Rush uses a heuristic to distinguish errors from warnings:
85+
* - If the shell script returns a nonzero process exit code, Rush interprets this as "one or more errors".
86+
* Error output is displayed in red, and it prevents Rush from attempting to process any downstream projects.
87+
* - If the shell script returns a zero process exit code but writes something to its stderr stream,
88+
* Rush interprets this as "one or more warnings". Warning output is printed in yellow, but does NOT prevent
89+
* Rush from processing downstream projects.
90+
*
91+
* Thus, warnings do not interfere with local development, but they will cause a CI job to fail, because
92+
* the Rush process itself returns a nonzero exit code if there are any warnings or errors. This is by design.
93+
* In an active monorepo, we've found that if you allow any warnings in your master branch, it inadvertently
94+
* teaches developers to ignore warnings, which quickly leads to a situation where so many "expected" warnings
95+
* have accumulated that warnings no longer serve any useful purpose.
96+
*
97+
* Sometimes a poorly behaved task will write output to stderr even though its operation was successful.
98+
* In that case, it's strongly recommended to fix the task. However, as a workaround you can set
99+
* allowWarningsInSuccessfulBuild=true, which causes Rush to return a nonzero exit code for errors only.
100+
*
101+
* Note: The default value is false. In Rush 5.7.x and earlier, the default value was true.
102+
*/
103+
"allowWarningsInSuccessfulBuild": false
82104
},
83105

84106
{

‎apps/rush-lib/src/api/CommandLineJson.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export interface IBulkCommandJson extends IBaseCommandJson {
2323
enableParallelism: boolean;
2424
ignoreDependencyOrder?: boolean;
2525
ignoreMissingScript?: boolean;
26+
allowWarningsInSuccessfulBuild?: boolean;
2627
}
2728

2829
/**

‎apps/rush-lib/src/cli/RushCommandLineParser.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,8 @@ export class RushCommandLineParser extends CommandLineParser {
193193

194194
enableParallelism: true,
195195
ignoreMissingScript: false,
196-
ignoreDependencyOrder: false
196+
ignoreDependencyOrder: false,
197+
allowWarningsInSuccessfulBuild: false
197198
}));
198199
}
199200

@@ -215,7 +216,8 @@ export class RushCommandLineParser extends CommandLineParser {
215216

216217
enableParallelism: true,
217218
ignoreMissingScript: false,
218-
ignoreDependencyOrder: false
219+
ignoreDependencyOrder: false,
220+
allowWarningsInSuccessfulBuild: false
219221
}));
220222
}
221223
}
@@ -247,7 +249,8 @@ export class RushCommandLineParser extends CommandLineParser {
247249

248250
enableParallelism: command.enableParallelism,
249251
ignoreMissingScript: command.ignoreMissingScript || false,
250-
ignoreDependencyOrder: command.ignoreDependencyOrder || false
252+
ignoreDependencyOrder: command.ignoreDependencyOrder || false,
253+
allowWarningsInSuccessfulBuild: !!command.allowWarningsInSuccessfulBuild
251254
}));
252255
break;
253256
case 'global':

‎apps/rush-lib/src/cli/scriptActions/BaseScriptAction.ts‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,9 @@ export abstract class BaseScriptAction extends BaseRushAction {
4040
}
4141

4242
// Find any parameters that are associated with this command
43-
for (const parameter of this._commandLineConfiguration.parameters) {
43+
for (const parameterJson of this._commandLineConfiguration.parameters) {
4444
let associated: boolean = false;
45-
for (const associatedCommand of parameter.associatedCommands) {
45+
for (const associatedCommand of parameterJson.associatedCommands) {
4646
if (associatedCommand === this.actionName) {
4747
associated = true;
4848
}
@@ -51,35 +51,36 @@ export abstract class BaseScriptAction extends BaseRushAction {
5151
if (associated) {
5252
let customParameter: CommandLineParameter | undefined;
5353

54-
switch (parameter.parameterKind) {
54+
switch (parameterJson.parameterKind) {
5555
case 'flag':
5656
customParameter = this.defineFlagParameter({
57-
parameterShortName: parameter.shortName,
58-
parameterLongName: parameter.longName,
59-
description: parameter.description
57+
parameterShortName: parameterJson.shortName,
58+
parameterLongName: parameterJson.longName,
59+
description: parameterJson.description
6060
});
6161
break;
6262
case 'choice':
6363
customParameter = this.defineChoiceParameter({
64-
parameterShortName: parameter.shortName,
65-
parameterLongName: parameter.longName,
66-
description: parameter.description,
67-
alternatives: parameter.alternatives.map(x => x.name),
68-
defaultValue: parameter.defaultValue
64+
parameterShortName: parameterJson.shortName,
65+
parameterLongName: parameterJson.longName,
66+
description: parameterJson.description,
67+
alternatives: parameterJson.alternatives.map(x => x.name),
68+
defaultValue: parameterJson.defaultValue
6969
});
7070
break;
7171
case 'string':
7272
customParameter = this.defineStringParameter({
73-
parameterLongName: parameter.longName,
74-
parameterShortName: parameter.shortName,
75-
description: parameter.description,
76-
argumentName: parameter.argumentName
73+
parameterLongName: parameterJson.longName,
74+
parameterShortName: parameterJson.shortName,
75+
description: parameterJson.description,
76+
argumentName: parameterJson.argumentName
7777
});
7878
break;
7979
default:
80-
throw new Error(`${RushConstants.commandLineFilename} defines a parameter "${parameter!.longName}"`
81-
+ ` using an unsupported parameter kind "${parameter!.parameterKind}"`);
80+
throw new Error(`${RushConstants.commandLineFilename} defines a parameter "${parameterJson!.longName}"`
81+
+ ` using an unsupported parameter kind "${parameterJson!.parameterKind}"`);
8282
}
83+
8384
if (customParameter) {
8485
this.customParameters.push(customParameter);
8586
}

‎apps/rush-lib/src/cli/scriptActions/BulkScriptAction.ts‎

Lines changed: 32 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ export interface IBulkScriptActionOptions extends IBaseScriptActionOptions {
2929
enableParallelism: boolean;
3030
ignoreMissingScript: boolean;
3131
ignoreDependencyOrder: boolean;
32+
allowWarningsInSuccessfulBuild: boolean;
3233

3334
/**
3435
* Optional command to run. Otherwise, use the `actionName` as the command to run.
@@ -57,15 +58,15 @@ export class BulkScriptAction extends BaseScriptAction {
5758
private _verboseParameter: CommandLineFlagParameter;
5859
private _parallelismParameter: CommandLineStringParameter | undefined;
5960
private _ignoreDependencyOrder: boolean;
61+
private _allowWarningsInSuccessfulBuild: boolean;
6062

61-
constructor(
62-
options: IBulkScriptActionOptions
63-
) {
63+
constructor(options: IBulkScriptActionOptions) {
6464
super(options);
6565
this._enableParallelism = options.enableParallelism;
6666
this._ignoreMissingScript = options.ignoreMissingScript;
6767
this._commandToRun = options.commandToRun || options.actionName;
6868
this._ignoreDependencyOrder = options.ignoreDependencyOrder;
69+
this._allowWarningsInSuccessfulBuild = options.allowWarningsInSuccessfulBuild;
6970
}
7071

7172
public run(): Promise<void> {
@@ -88,43 +89,42 @@ export class BulkScriptAction extends BaseScriptAction {
8889
// Collect all custom parameter values
8990
const customParameterValues: string[] = [];
9091

91-
for (const customParameter of this.customParameters) {
92-
customParameter.appendToArgList(customParameterValues);
93-
}
94-
9592
const changedProjectsOnly: boolean = this.actionName === 'build' && this._changedProjectsOnly.value;
9693

97-
const tasks: TaskSelector = new TaskSelector(
98-
{
99-
rushConfiguration: this.rushConfiguration,
100-
toFlags: this._mergeToProjects(),
101-
fromFlags: this._fromFlag.values,
102-
commandToRun: this._commandToRun,
103-
customParameterValues,
104-
isQuietMode,
105-
parallelism,
106-
isIncrementalBuildAllowed: this.actionName === 'build',
107-
changedProjectsOnly,
108-
ignoreMissingScript: this._ignoreMissingScript,
109-
ignoreDependencyOrder: this._ignoreDependencyOrder
110-
}
111-
);
94+
const tasks: TaskSelector = new TaskSelector({
95+
rushConfiguration: this.rushConfiguration,
96+
toFlags: this._mergeToProjects(),
97+
fromFlags: this._fromFlag.values,
98+
commandToRun: this._commandToRun,
99+
customParameterValues,
100+
isQuietMode,
101+
parallelism,
102+
isIncrementalBuildAllowed: this.actionName === 'build',
103+
changedProjectsOnly,
104+
ignoreMissingScript: this._ignoreMissingScript,
105+
ignoreDependencyOrder: this._ignoreDependencyOrder,
106+
allowWarningsInSuccessfulBuild: this._allowWarningsInSuccessfulBuild
107+
});
112108

113-
return tasks.execute().then(
114-
() => {
115-
stopwatch.stop();
109+
return tasks.execute().then(() => {
110+
stopwatch.stop();
111+
console.log(colors.green(`rush ${this.actionName} (${stopwatch.toString()})`));
112+
this._doAfterTask(stopwatch, true);
113+
}).catch((error: Error) => {
114+
stopwatch.stop();
115+
if (error instanceof AlreadyReportedError) {
116116
console.log(colors.green(`rush ${this.actionName} (${stopwatch.toString()})`));
117-
this._doAfterTask(stopwatch, true);
118-
})
119-
.catch((error: Error) => {
117+
} else {
120118
if (error && error.message) {
121119
console.log('Error: ' + error.message);
122120
}
123-
stopwatch.stop();
121+
124122
console.log(colors.red(`rush ${this.actionName} - Errors! (${stopwatch.toString()})`));
125-
this._doAfterTask(stopwatch, false);
126-
throw new AlreadyReportedError();
127-
});
123+
}
124+
125+
this._doAfterTask(stopwatch, false);
126+
throw new AlreadyReportedError();
127+
});
128128
}
129129

130130
protected onDefineParameters(): void {

‎apps/rush-lib/src/logic/TaskSelector.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ export interface ITaskSelectorConstructor {
2121
changedProjectsOnly: boolean;
2222
ignoreMissingScript: boolean;
2323
ignoreDependencyOrder: boolean;
24+
allowWarningsInSuccessfulBuild: boolean;
2425
}
2526

2627
/**
@@ -42,10 +43,12 @@ export class TaskSelector {
4243
this._options = options;
4344

4445
this._packageChangeAnalyzer = new PackageChangeAnalyzer(options.rushConfiguration);
45-
this._taskRunner = new TaskRunner(
46-
this._options.isQuietMode,
47-
this._options.parallelism,
48-
this._options.changedProjectsOnly);
46+
this._taskRunner = new TaskRunner({
47+
quietMode: this._options.isQuietMode,
48+
parallelism: this._options.parallelism,
49+
changedProjectsOnly: this._options.changedProjectsOnly,
50+
allowWarningsInSuccessfulBuild: this._options.allowWarningsInSuccessfulBuild
51+
});
4952

5053
try {
5154
this._rushLinkJson = JsonFile.load(this._options.rushConfiguration.rushLinkJsonFilename);

‎apps/rush-lib/src/logic/taskRunner/TaskRunner.ts‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,15 @@ import { Stopwatch } from '../../utilities/Stopwatch';
1414
import { ITask, ITaskDefinition } from './ITask';
1515
import { TaskStatus } from './TaskStatus';
1616
import { TaskError } from './TaskError';
17+
import { AlreadyReportedError } from '../../utilities/AlreadyReportedError';
18+
19+
export interface ITaskRunnerOptions {
20+
quietMode: boolean;
21+
parallelism: string | undefined;
22+
changedProjectsOnly: boolean;
23+
allowWarningsInSuccessfulBuild: boolean;
24+
terminal?: Terminal;
25+
}
1726

1827
/**
1928
* A class which manages the execution of a set of tasks with interdependencies.
@@ -25,27 +34,33 @@ import { TaskError } from './TaskError';
2534
export class TaskRunner {
2635
private _tasks: Map<string, ITask>;
2736
private _changedProjectsOnly: boolean;
37+
private _allowWarningsInSuccessfulBuild: boolean;
2838
private _buildQueue: ITask[];
2939
private _quietMode: boolean;
3040
private _hasAnyFailures: boolean;
41+
private _hasAnyWarnings: boolean;
3142
private _parallelism: number;
3243
private _currentActiveTasks: number;
3344
private _totalTasks: number;
3445
private _completedTasks: number;
3546
private _terminal: Terminal;
3647

37-
constructor(
38-
quietMode: boolean,
39-
parallelism: string | undefined,
40-
changedProjectsOnly: boolean,
41-
terminal?: Terminal
42-
) {
48+
constructor(options: ITaskRunnerOptions) {
49+
const {
50+
quietMode,
51+
parallelism,
52+
changedProjectsOnly,
53+
allowWarningsInSuccessfulBuild,
54+
terminal = new Terminal(new ConsoleTerminalProvider())
55+
} = options;
4356
this._tasks = new Map<string, ITask>();
4457
this._buildQueue = [];
4558
this._quietMode = quietMode;
4659
this._hasAnyFailures = false;
60+
this._hasAnyWarnings = false;
4761
this._changedProjectsOnly = changedProjectsOnly;
48-
this._terminal = terminal || new Terminal(new ConsoleTerminalProvider());
62+
this._allowWarningsInSuccessfulBuild = allowWarningsInSuccessfulBuild;
63+
this._terminal = terminal;
4964

5065
const numberOfCores: number = os.cpus().length;
5166

@@ -161,6 +176,9 @@ export class TaskRunner {
161176

162177
if (this._hasAnyFailures) {
163178
return Promise.reject(new Error('Project(s) failed to build'));
179+
} else if (this._hasAnyWarnings && !this._allowWarningsInSuccessfulBuild) {
180+
this._terminal.writeWarningLine('Project(s) succeeded with warnings');
181+
return Promise.reject(new AlreadyReportedError());
164182
} else {
165183
return Promise.resolve();
166184
}
@@ -217,6 +235,7 @@ export class TaskRunner {
217235
this._markTaskAsSuccess(task);
218236
break;
219237
case TaskStatus.SuccessWithWarning:
238+
this._hasAnyWarnings = true;
220239
this._markTaskAsSuccessWithWarning(task);
221240
break;
222241
case TaskStatus.Skipped:

0 commit comments

Comments
 (0)