Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lots of bug fixes relating to failed builds and recovery from them #550

Merged
merged 7 commits into from
Oct 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions build/lib/src/builder/logging.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,27 @@ const Symbol logKey = #buildLog;
/// Will be `null` when not running within a build.
Logger get log => Zone.current[logKey] as Logger;

T scopeLog<T>(T fn(), Logger log) => runZoned(fn, zoneSpecification:
new ZoneSpecification(print: (self, parent, zone, message) {
log.info(message);
}), zoneValues: {logKey: log});
/// Runs [fn] in an error handling [Zone].
///
/// Any calls to [print] will be logged with `log.info`, and any errors will be
/// logged with `log.severe`.
///
/// Completes with the first error or result of `fn`, whichever comes first.
Future<T> scopeLogAsync<T>(Future<T> fn(), Logger log) {
var done = new Completer<T>();
runZoned(fn,
zoneSpecification:
new ZoneSpecification(print: (self, parent, zone, message) {
log.info(message);
}),
zoneValues: {logKey: log},
onError: (Object e, StackTrace s) {
log.severe('', e);
if (done.isCompleted) return;
done.completeError(e, s);
}).then((result) {
if (done.isCompleted) return;
done.complete(result);
});
return done.future;
}
2 changes: 1 addition & 1 deletion build/lib/src/generate/run_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ Future<Null> runBuilder(Builder builder, Iterable<AssetId> inputs,
}
}

await scopeLog(() => Future.wait(inputs.map(buildForInput)), logger);
await scopeLogAsync(() => Future.wait(inputs.map(buildForInput)), logger);

if (shouldDisposeResourceManager) {
await resourceManager.disposeAll();
Expand Down
2 changes: 1 addition & 1 deletion build_compilers/lib/src/dev_compiler_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ class DevCompilerBuilder implements Builder {
try {
await createDevCompilerModule(module, buildStep);
} on DartDevcCompilationException catch (e) {
log.warning('Error compiling ${module.jsId}:\n$e');
await buildStep.writeAsString(
buildStep.inputId.changeExtension(jsModuleErrorsExtension), '$e');
log.severe('', e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty string? Weird?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just logging the error - it feels a bit weird for sure but there is already additional context provided by the loggers name (it is named based on the current builder and primary input).

The logger handles consistently formatting the error already as well - so that ends up being nicer than passing the error as the message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...why no stack? Not useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya - we don't have a true stack trace here. This error is originating from a worker, and contains the entire message that the worker sent back - which may or may not contain a useful stack trace already.

}
}
}
Expand Down
11 changes: 8 additions & 3 deletions build_compilers/lib/src/modules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,14 @@ class Module extends Object with _$ModuleSerializerMixin {
var next = modulesToCrawl.last;
modulesToCrawl.remove(next);
if (transitiveDeps.containsKey(next)) continue;
var module = new Module.fromJson(JSON.decode(
await reader.readAsString(next.changeExtension(moduleExtension)))
as Map<String, dynamic>);
var nextModuleId = next.changeExtension(moduleExtension);
if (!await reader.canRead(nextModuleId)) {
log.warning('Missing module $nextModuleId');
continue;
}
var module = new Module.fromJson(
JSON.decode(await reader.readAsString(nextModuleId))
as Map<String, dynamic>);
transitiveDeps[next] = module;
modulesToCrawl.addAll(module.directDependencies);
}
Expand Down
10 changes: 6 additions & 4 deletions build_runner/lib/src/asset/reader.dart
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,20 @@ class SinglePhaseReader implements AssetReader {
Iterable<Glob> get globsRan => _globsRan;

bool _isReadable(AssetId id) {
_assetsRead.add(id);
var node = _assetGraph.get(id);
if (node == null) return false;
if (node == null) {
_assetGraph.add(new SyntheticAssetNode(id));
return false;
}
if (node is SyntheticAssetNode) return false;
if (node is! GeneratedAssetNode) return true;
return (node as GeneratedAssetNode).phaseNumber < _phaseNumber;
}

@override
Future<bool> canRead(AssetId id) async {
if (!_isReadable(id)) return new Future.value(false);
_assetsRead.add(id);
await _ensureAssetIsBuilt(id);
var node = _assetGraph.get(id);
if (node is GeneratedAssetNode) {
Expand All @@ -59,15 +63,13 @@ class SinglePhaseReader implements AssetReader {
@override
Future<List<int>> readAsBytes(AssetId id) async {
if (!_isReadable(id)) throw new AssetNotFoundException(id);
_assetsRead.add(id);
await _ensureAssetIsBuilt(id);
return _delegate.readAsBytes(id);
}

@override
Future<String> readAsString(AssetId id, {Encoding encoding: UTF8}) async {
if (!_isReadable(id)) throw new AssetNotFoundException(id);
_assetsRead.add(id);
await _ensureAssetIsBuilt(id);
return _delegate.readAsString(id, encoding: encoding);
}
Expand Down
39 changes: 24 additions & 15 deletions build_runner/lib/src/asset_graph/graph.dart
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,17 @@ class AssetGraph {
///
/// Throws a [StateError] if it already exists in the graph.
void _add(AssetNode node) {
if (contains(node.id)) {
throw new StateError(
'Tried to add node ${node.id} to the asset graph but it already '
'exists.');
var existing = get(node.id);
if (existing != null) {
if (existing is SyntheticAssetNode) {
_remove(existing.id);
node.outputs.addAll(existing.outputs);
node.primaryOutputs.addAll(existing.primaryOutputs);
} else {
throw new StateError(
'Tried to add node ${node.id} to the asset graph but it already '
'exists.');
}
}
_nodesByPackage.putIfAbsent(node.id.package, () => {})[node.id.path] = node;
}
Expand All @@ -84,8 +91,9 @@ class AssetGraph {
allNodes.where((n) => n is GeneratedAssetNode).map((n) => n.id);

/// All the source files in the graph.
Iterable<AssetId> get sources =>
allNodes.where((n) => n is! GeneratedAssetNode).map((n) => n.id);
Iterable<AssetId> get sources => allNodes
.where((n) => n is! GeneratedAssetNode && n is! SyntheticAssetNode)
.map((n) => n.id);

/// Updates graph structure, invalidating and deleting any outputs that were
/// affected.
Expand All @@ -105,31 +113,32 @@ class AssetGraph {
// Builds up `idsToDelete` and `idsToRemove` by recursively invalidating
// the outputs of `id`.
void clearNodeAndDeps(AssetId id, ChangeType rootChangeType,
{AssetId parent, bool rootIsSource}) {
{bool rootIsSource}) {
var node = this.get(id);
if (node == null) return;
if (!invalidatedIds.add(id)) return;
if (parent == null) rootIsSource = node is! GeneratedAssetNode;

// Update all outputs of this asset as well.
for (var output in node.outputs) {
clearNodeAndDeps(output, rootChangeType,
parent: node.id, rootIsSource: rootIsSource);
}
rootIsSource ??= node is! GeneratedAssetNode;

if (node is GeneratedAssetNode) {
idsToDelete.add(id);
if (rootIsSource &&
rootChangeType == ChangeType.REMOVE &&
node.primaryInput == parent) {
idsToRemove.contains(node.primaryInput)) {
idsToRemove.add(id);
} else {
node.needsUpdate = true;
node.wasOutput = false;
node.globs = new Set();
}
} else {
// This is a source
if (rootChangeType == ChangeType.REMOVE) idsToRemove.add(id);
}

// Update all outputs of this asset as well.
for (var output in node.outputs) {
clearNodeAndDeps(output, rootChangeType, rootIsSource: rootIsSource);
}
}

updates.forEach(clearNodeAndDeps);
Expand Down
12 changes: 12 additions & 0 deletions build_runner/lib/src/asset_graph/node.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,18 @@ class AssetNode {
String toString() => 'AssetNode: $id';
}

/// A node which is not a generated or source asset.
///
/// Typically these are created as a result of `canRead` calls for assets that
/// don't exist in the graph. We still need to set up proper dependencies so
/// that if that asset gets added later the outputs are properly invalidated.
class SyntheticAssetNode extends AssetNode {
SyntheticAssetNode(AssetId id) : super(id);

@override
String toString() => 'SyntheticAssetNode: $id';
}

/// A generated node in the asset graph.
class GeneratedAssetNode extends AssetNode {
/// The phase which generated this asset.
Expand Down
43 changes: 25 additions & 18 deletions build_runner/lib/src/asset_graph/serialization.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ part of 'graph.dart';
///
/// This should be incremented any time the serialize/deserialize formats
/// change.
const _version = 7;
const _version = 8;

/// Deserializes an [AssetGraph] from a [Map].
class _AssetGraphDeserializer {
Expand Down Expand Up @@ -43,16 +43,21 @@ class _AssetGraphDeserializer {

AssetNode _deserializeAssetNode(List serializedNode) {
AssetNode node;
if (serializedNode.length == 3) {
node = new AssetNode(_idToAssetId[serializedNode[0]]);
} else if (serializedNode.length == 8) {
var id = _idToAssetId[serializedNode[0]];
if (serializedNode.length == 4) {
if (_deserializeBool(serializedNode[3] as int)) {
node = new SyntheticAssetNode(id);
} else {
node = new AssetNode(id);
}
} else if (serializedNode.length == 9) {
node = new GeneratedAssetNode(
serializedNode[5] as int,
_idToAssetId[serializedNode[3]],
_deserializeBool(serializedNode[7] as int),
_deserializeBool(serializedNode[4] as int),
_idToAssetId[serializedNode[0]],
globs: (serializedNode[6] as Iterable<String>)
serializedNode[6] as int,
_idToAssetId[serializedNode[4]],
_deserializeBool(serializedNode[8] as int),
_deserializeBool(serializedNode[5] as int),
id,
globs: (serializedNode[7] as Iterable<String>)
.map((pattern) => new Glob(pattern))
.toSet(),
);
Expand Down Expand Up @@ -125,7 +130,7 @@ class _WrappedAssetNode extends Object with ListMixin implements List {
_WrappedAssetNode(this.node, this.serializer);

@override
int get length => 3;
int get length => 4;
@override
set length(_) => throw new UnsupportedError(
'length setter not unsupported for WrappedAssetNode');
Expand All @@ -141,6 +146,8 @@ class _WrappedAssetNode extends Object with ListMixin implements List {
return node.primaryOutputs
.map((id) => serializer._assetIdToId[id])
.toList();
case 3:
return _serializeBool(node is SyntheticAssetNode);
default:
throw new RangeError.index(index, this);
}
Expand All @@ -167,22 +174,22 @@ class _WrappedGeneratedAssetNode extends _WrappedAssetNode {
Object operator [](int index) {
if (index < super.length) return super[index];
switch (index) {
case 3:
case 4:
return generatedNode.primaryInput != null
? serializer._assetIdToId[generatedNode.primaryInput]
: null;
case 4:
return _serializeBool(generatedNode.wasOutput);
case 5:
return generatedNode.phaseNumber;
return _serializeBool(generatedNode.wasOutput);
case 6:
return generatedNode.globs.map((glob) => glob.pattern).toList();
return generatedNode.phaseNumber;
case 7:
return generatedNode.globs.map((glob) => glob.pattern).toList();
case 8:
return _serializeBool(generatedNode.needsUpdate);
default:
throw new RangeError.index(index, this);
}
}

static int _serializeBool(bool value) => value ? 1 : 0;
}

int _serializeBool(bool value) => value ? 1 : 0;
13 changes: 9 additions & 4 deletions build_runner/lib/src/generate/build_definition.dart
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,17 @@ class _Loader {
}
}

var newSources = inputSources
.difference(assetGraph.allNodes.map((node) => node.id).toSet());
var newSources = inputSources.difference(assetGraph.allNodes
.where((node) => node is! SyntheticAssetNode)
.map((node) => node.id)
.toSet());
addUpdates(newSources, ChangeType.ADD);
var removedAssets = assetGraph.allNodes
.where((n) =>
n is! GeneratedAssetNode || (n as GeneratedAssetNode).wasOutput)
.where((n) {
if (n is SyntheticAssetNode) return false;
if (n is GeneratedAssetNode) return n.wasOutput;
return true;
})
.map((n) => n.id)
.where((id) => !allSources.contains((id)));

Expand Down
12 changes: 9 additions & 3 deletions build_runner/lib/src/generate/build_impl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ class BuildImpl {
var ids = new Set<AssetId>();
await Future
.wait(_assetGraph.packageNodes(inputSet.package).map((node) async {
if (node is SyntheticAssetNode) return;
if (!inputSet.matches(node.id)) return;
if (node is GeneratedAssetNode) {
if (node.phaseNumber >= phaseNumber) return;
Expand Down Expand Up @@ -310,8 +311,9 @@ class BuildImpl {
input.package,
(phase, input) => _runLazyPhaseForInput(phase, input, resourceManager));
var wrappedWriter = new AssetWriterSpy(_writer);
var logger = new Logger('$builder on $input');
await runBuilder(builder, [input], wrappedReader, wrappedWriter, _resolvers,
resourceManager: resourceManager);
logger: logger, resourceManager: resourceManager);

// Reset the state for all the `builderOutputs` nodes based on what was
// read and written.
Expand Down Expand Up @@ -339,11 +341,15 @@ class BuildImpl {
(phase, input) => _runLazyPhaseForInput(phase, input, resourceManager));
var wrappedWriter = new AssetWriterSpy(_writer);

var logger = new Logger('runBuilder');
var logger = new Logger('$builder on $package');
var buildStep = new BuildStepImpl(null, builderOutputs, wrappedReader,
wrappedWriter, _packageGraph.root.name, _resolvers, resourceManager);
try {
await scopeLog(() => builder.build(buildStep), logger);
// Wrapping in `new Future.value` to work around
// https://github.com/dart-lang/sdk/issues/31237, users might return
// synchronously and not have any analysis errors today.
await scopeLogAsync(
() => new Future.value(builder.build(buildStep)), logger);
} finally {
await buildStep.complete();
}
Expand Down
Loading