Add storage and re-reporting of previous errors #1581
Conversation
Towards #910 The main benefit of the proposed `--rerun-failing-actions` is to be able to see errors again from previous builds that have scrolled off screen. The downside to rerunning is that it is a pattern encouraging retrying non-hermetic actions. Instead we will store and re-print previous error messages which more directly addresses the need. This commit sketches out the interaction between the failure reporter and the build - storage of errors to individual files will happen in a followup.
|
|
||
| import '../asset_graph/node.dart'; | ||
|
|
||
| /// A tracker for the errors which have already been reported during the current |
There was a problem hiding this comment.
nit: 80 chars (same below)
| await tracker.track( | ||
| () => runBuilder(builder, [input], wrappedReader, wrappedWriter, | ||
| new PerformanceTrackingResolvers(_resolvers, tracker), | ||
| logger: logger, | ||
| resourceManager: _resourceManager).catchError((_) { | ||
| errorThrown = true; | ||
| // Errors tracked throug logger |
There was a problem hiding this comment.
Do we need to clean up the previous errors before running this builder again? See the _cleanupStaleOutputs call on line 413 above
There was a problem hiding this comment.
oh yeah, I had put in FailureReporter.clean but forgot to call it. Calling it from within _cleanUpStaleOutputs now.
- Change less in constants.dart - Clean by primary input instead of by output since for PostProcessBuilders there might not be an output
| return new Future.value(null); | ||
| })); | ||
| await FailureReporter.clean(nodes.first); |
There was a problem hiding this comment.
lets leave a comment about why we only call this for the first node
There was a problem hiding this comment.
ended up removing this approach and it's cleaning based on the primary input now.
| /// Log stored errors for any build steps which would output nodes in | ||
| /// [failingNodes] which haven't already been reported. | ||
| Future<void> reportErrors(Iterable<GeneratedAssetNode> failingNodes) async { | ||
| for (final failure in failingNodes) { |
There was a problem hiding this comment.
it is probably worth parallelizing this
There was a problem hiding this comment.
Done. Do you think I should add a file handle pool? This isn't really in the critical path for a build so it might not make much difference.
There was a problem hiding this comment.
hmm, you could just use the asset reader as well which would handle this? If somebody has a lot of cascading errors it could cause problems.
There was a problem hiding this comment.
I'd prefer to leave it using dart:io for now, for consistency.
Since we do directory level work we can't use asset reader/writer for everything.
Closes #910
The main benefit of the proposed
--rerun-failing-actionsis to be ableto see errors again from previous builds that have scrolled off screen.
The downside to rerunning is that it is a pattern encouraging retrying
non-hermetic actions. Instead we will store and re-print previous error
messages which more directly addresses the need.
This commit sketches out the interaction between the failure reporter
and the build - storage of errors to individual files will happen in a
followup.