pub run build_runner build returns exit code 0 even when SEVERE errors reported

[chalin]

If I seed a template error and run test over one of the toh-0 example app like this:

pub run build_runner build --delete-conflicting-outputs --output=build; echo $?

I get:

---

[INFO] Entrypoint: Generating build script completed, took 334ms
[INFO] BuildDefinition: Building new asset graph completed, took 663ms
[INFO] BuildDefinition: Checking for unexpected pre-existing outputs. completed, took 1ms
[SEVERE] Instance of 'LibraryBuilder' on angular_tour_of_heroes|lib/app_component.dart: Error running TemplateGenerator for angular_tour_of_heroes|lib/app_component.dart.
Template parse errors:
... [some error]...

Please fix all errors before compiling (warnings are okay).
}
[WARNING] Instance of 'WebEntrypointBuilder' on angular_tour_of_heroes|test/app_test.dart: Unable to read angular_tour_of_heroes|test/app_test.ddc.js, check your console for compilation errors.
[WARNING] Instance of 'WebEntrypointBuilder' on angular_tour_of_heroes|test/app_test.dart.browser_test.dart: Unable to read angular_tour_of_heroes|test/app_test.ddc.js, check your console for compilation errors.
[INFO] Build: Running build completed, took 20.0s
[INFO] Build: Caching finalized dependency graph completed, took 100ms
[INFO] CreateOutputDir: Creating merged output dir build completed, took 1.4s
[INFO] Build: Succeeded after 21.6s with 2074 outputs
0

---

So, in summary:

• The template compiler reports SEVERE errors
• A message suggests "Please fix all errors before compiling", but yet
• We get "Build: Succeeded"
• An exit code of 0

I'm using the latest build_* packages:

toh-0 > grep build_ pubspec.yaml
  build_runner: ^0.7.8
  build_test: ^0.10.0
  build_web_compilers: ^0.2.1

cc @kwalrath

[jakemac53]

There is a flag to fail builds, I can't recall exactly what it is but --help should show it. This is a bit of a grey area because at dev time it isn't clear we want to fail the build, there is value in letting you see what it was able to complete

[alorenzen]

I'm not sure I agree that we don't want to fail the build at dev time. Internally, we fail the build if any of the builders report an error.

The normal dev cycle would be to use serve, not build. I'm fine with serve being a little more forgiving, but build should be stricter.

I've noticed issues like this on travis, where the build "fails", but returns exit code 0, and thus travis thinks everything is dandy.

[chalin]

... thus travis thinks everything is dandy.

Yes, that is the one of the main issues.

[matanlurey]

There is a --fail-on-severe.

The reason this is not the default is similar to

• Enforce build_extensions config from build.yaml to be accurate #590
• Add an optional flag --no-package-cycles (or similar) #907

... which would match the internal build system, but be frustrating to some users.

@alorenzen @chalin If that flag works for you, let's file a separate issue about defaulting it.

[chalin]

The option --fail-on-severe has no effect: exit code is still 0 and the last line reports:

[INFO] Build: Succeeded

[matanlurey]

That doesn't look good, the test cases look fine:

build/build_runner/test/generate/build_error_test.dart

Lines 34 to 65 in 2da9f97

	test('should not fail if a severe is logged without failOnSevere', () async {
	await testBuilders(
	[applyToRoot(new _LoggingBuilder(Level.SEVERE))],
	{
	'a|lib/a.dart': '',
	},
	packageGraph: buildPackageGraph({rootPackage('a'): []}),
	failOnSevere: false,
	checkBuildStatus: true,
	status: BuildStatus.success,
	outputs: {
	'a|lib/a.dart.empty': '',
	},
	);
	});
	test('should fail if a servere logged and failOnSevere is set', () async {
	await testBuilders(
	[applyToRoot(new _LoggingBuilder(Level.SEVERE))],
	{
	'a|lib/a.dart': '',
	},
	packageGraph: buildPackageGraph({rootPackage('a'): []}),
	failOnSevere: true,
	checkBuildStatus: true,
	status: BuildStatus.failure,
	outputs: {
	'a|lib/a.dart.empty': '',
	},
	);
	});
	}

/cc @natebosch @jakemac53

[jakemac53]

My guess here would be that there was a previous build ran without --fail-on-severe (where the actual failures happened), and then there was an incremental build with --fail-on-severe which didn't actually re-run the failing parts, so we never saw a severe log.

In general, if the build fails I believe that we wouldn't cache the asset graph so we would always rerun the build, but if you don't provide that flag when it initially fails then we get in this situation.

[chalin]

My guess here would be that there was a previous build ran without --fail-on-severe

I actually started afresh:

> rm -Rf build .dart* *.lock

And then ran pub (for get and then build_runner build), and it still returns an exit code of 0 and reports a build success

[jakemac53]

ah yep I can repro even on e2e_example

[jakemac53]

Looks like we parse it but we never pass the option to build

build/build_runner/lib/src/entrypoint/options.dart

Line 218 in d964854

var result = await build(builderApplications,

[chalin]

That would do it :)

[jakemac53]

With #922 fresh builds will have the correct behavior.

There are a few remaining issues so I will keep this open:

• The logs come through async so its technically possible (although extremely unlikely) to miss severe logs.
• The logging message still says it completed successfully even if it in fact failed.
• It doesn't remember if previous builds failed, so only clean builds have the right behavior.
• Add a --rerun-failing-actions flag so you can see logs for failing actions again if desired.

[matanlurey]

@jakemac53 Can you give an example of --rerun-failing-actions?

[natebosch]

Example use case for --rerun-failing-actions:

• Run a build. See multiple separate failures.
• Fix one of the failures, rerun the build, see that that part succeeded but the overall build is still failed, we didn't even try to rerun the other action that failed because it's inputs didn't change. I forgot what the failure even was.
• Run the build with --rerun-failing-actions, can see the error message for the other failure and know what I need to fix next.

[matanlurey]

Cool, makes sense!

[kevmoo]

Also just hit this – by default I'd argue that rebuilding something with errors should show all of the errors the next time around.

[natebosch]

by default I'd argue that rebuilding something with errors should show all of the errors the next time around.

Showing the errors is one aspect - rerunning is a different aspect. One thing I hope to avoid doing is rerunning by default because it encourages us to tolerate things that fail to build for flaky reasons. We could consider storing details when things fail though if our main focus is not losing them when the previous build scrolls away.