Skip to content

Do invalidation in Build instead of the asset graph #3984

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
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
5 changes: 2 additions & 3 deletions build_runner/bin/graph_inspector.dart
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,9 @@ class InspectNodeCommand extends Command<bool> {
final nodeState = node.generatedNodeState!;
final nodeConfiguration = node.generatedNodeConfiguration!;
description
..writeln(' state: ${nodeState.pendingBuildAction}')
..writeln(' wasOutput: ${nodeState.wasOutput}')
..writeln(' wasOutput: ${node.wasOutput}')
..writeln(' phase: ${nodeConfiguration.phaseNumber}')
..writeln(' isFailure: ${nodeState.isFailure}');
..writeln(' result: ${nodeState.result}');
}

void printAsset(AssetId asset) =>
Expand Down
2 changes: 1 addition & 1 deletion build_runner/lib/src/entrypoint/clean.dart
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ Future<void> _cleanUpSourceOutputs(
for (var id in assetGraph.outputs) {
if (id.package != packageGraph.root.name) continue;
var node = assetGraph.get(id)!;
if (node.generatedNodeState!.wasOutput) {
if (node.wasOutput) {
// Note that this does a file.exists check in the root package and
// only tries to delete the file if it exists. This way we only
// actually delete to_source outputs, without reading in the build
Expand Down
15 changes: 3 additions & 12 deletions build_runner/lib/src/server/asset_graph_handler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -151,19 +151,10 @@ class AssetGraphHandler {
node.type == NodeType.generated
? node.generatedNodeConfiguration!.isHidden
: null,
'state':
'wasOutput': node.type == NodeType.generated ? node.wasOutput : null,
'result':
node.type == NodeType.generated
? '${node.generatedNodeState!.pendingBuildAction}'
: node.type == NodeType.glob
? '${node.globNodeState!.pendingBuildAction}'
: null,
'wasOutput':
node.type == NodeType.generated
? node.generatedNodeState!.wasOutput
: null,
'isFailure':
node.type == NodeType.generated
? node.generatedNodeState!.isFailure
? node.generatedNodeState!.result
: null,
'phaseNumber':
node.type == NodeType.generated
Expand Down
8 changes: 2 additions & 6 deletions build_runner/test/generate/watch_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -367,9 +367,7 @@ void main() {
bCopyId,
phaseNumber: 0,
primaryInput: makeAssetId('a|web/b.txt'),
pendingBuildAction: PendingBuildAction.none,
wasOutput: true,
isFailure: false,
result: true,
lastKnownDigest: computeDigest(bCopyId, 'b2'),
inputs: [makeAssetId('a|web/b.txt')],
isHidden: false,
Expand All @@ -388,9 +386,7 @@ void main() {
cCopyId,
phaseNumber: 0,
primaryInput: cTxtId,
pendingBuildAction: PendingBuildAction.none,
wasOutput: true,
isFailure: false,
result: true,
lastKnownDigest: computeDigest(cCopyId, 'c'),
inputs: [makeAssetId('a|web/c.txt')],
isHidden: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ final copyTwice = TestBuilder(
final maybeReadCopy = TestBuilder(
buildExtensions: appendExtension('.other', from: '.txt'),
extraWork: (buildStep, _) async {
print('${buildStep.inputId} -> .other, checking for true');
if ((await buildStep.readAsString(buildStep.inputId)).contains('true')) {
print('${buildStep.inputId} -> .other, found true so reading copy');
await buildStep.readAsString(buildStep.inputId.addExtension('.copy'));
} else {
print('${buildStep.inputId} -> .other, no true so not reading copy');
}
},
);
Expand Down
6 changes: 3 additions & 3 deletions build_runner/test/server/asset_handler_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import 'package:build_runner_core/src/asset_graph/post_process_build_step_id.dar
import 'package:build_runner_core/src/generate/build_phases.dart';
import 'package:build_runner_core/src/generate/options.dart';
import 'package:build_runner_core/src/package_graph/target_graph.dart';
import 'package:crypto/crypto.dart';
import 'package:shelf/shelf.dart';
import 'package:test/test.dart';

Expand Down Expand Up @@ -137,10 +138,9 @@ void main() {
AssetNode.generated(
AssetId('a', 'web/main.ddc.js'),
phaseNumber: 0,
pendingBuildAction: PendingBuildAction.none,
isHidden: false,
wasOutput: true,
isFailure: true,
lastKnownDigest: Digest([]),
result: false,
primaryInput: AssetId('a', 'web/main.dart'),
),
);
Expand Down
6 changes: 3 additions & 3 deletions build_runner/test/server/serve_handler_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import 'package:build_runner_core/src/generate/build_phases.dart';
import 'package:build_runner_core/src/generate/options.dart';
import 'package:build_runner_core/src/generate/performance_tracker.dart';
import 'package:build_runner_core/src/package_graph/target_graph.dart';
import 'package:crypto/crypto.dart';
import 'package:logging/logging.dart';
import 'package:shelf/shelf.dart';
import 'package:stream_channel/stream_channel.dart';
Expand Down Expand Up @@ -210,10 +211,9 @@ void main() {
AssetNode.generated(
AssetId('a', 'web/main.ddc.js'),
phaseNumber: 0,
pendingBuildAction: PendingBuildAction.none,
isHidden: false,
wasOutput: true,
isFailure: true,
lastKnownDigest: Digest([]),
result: false,
primaryInput: AssetId('a', 'web/main.dart'),
),
);
Expand Down
4 changes: 2 additions & 2 deletions build_runner_core/lib/src/asset/finalized_reader.dart
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ class FinalizedReader {

if (node.type == NodeType.generated) {
final nodeState = node.generatedNodeState!;
if (nodeState.isFailure) return UnreadableReason.failed;
if (!nodeState.wasOutput) return UnreadableReason.notOutput;
if (nodeState.result == false) return UnreadableReason.failed;
if (!node.wasOutput) return UnreadableReason.notOutput;
// No need to explicitly check readability for generated files, their
// readability is recorded in the node state.
return null;
Expand Down
121 changes: 8 additions & 113 deletions build_runner_core/lib/src/asset_graph/graph.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';
import 'dart:collection';
import 'dart:convert';
import 'dart:io';

Expand All @@ -14,7 +13,6 @@ import 'package:build/src/internal.dart';
import 'package:built_collection/built_collection.dart';
import 'package:built_value/serializer.dart';
import 'package:crypto/crypto.dart';
import 'package:glob/glob.dart';
import 'package:meta/meta.dart';
import 'package:package_config/package_config.dart';
import 'package:watcher/watcher.dart';
Expand Down Expand Up @@ -381,9 +379,7 @@ class AssetGraph implements GeneratedAssetHider {
/// The outputs which were, or would have been, produced by failing actions.
Iterable<AssetNode> get failedOutputs => allNodes.where(
(n) =>
n.type == NodeType.generated &&
n.generatedNodeState!.isFailure &&
n.generatedNodeState!.pendingBuildAction == PendingBuildAction.none,
n.type == NodeType.generated && n.generatedNodeState!.result == false,
);

/// All the generated outputs for a particular phase.
Expand All @@ -400,9 +396,7 @@ class AssetGraph implements GeneratedAssetHider {

/// Updates graph structure, invalidating and deleting any outputs that were
/// affected.
///
/// Returns the list of [AssetId]s that were invalidated.
Future<Set<AssetId>> updateAndInvalidate(
Future<void> updateAndInvalidate(
BuildPhases buildPhases,
Map<AssetId, ChangeType> updates,
String rootPackage,
Expand Down Expand Up @@ -466,111 +460,19 @@ class AssetGraph implements GeneratedAssetHider {
var idsToDelete = Set<AssetId>.from(transitiveRemovedIds)
..removeAll(removeIds);

// TODO(davidmorgan): ensure deleted outputs are updated.
// We definitely need to update manually deleted outputs.
for (var deletedOutput in removeIds
/*for (var deletedOutput in removeIds
.map(get)
.nonNulls
.where((n) => n.type == NodeType.generated)) {
updateNode(deletedOutput.id, (nodeBuilder) {
nodeBuilder.generatedNodeState.pendingBuildAction =
PendingBuildAction.build;
});
}
}*/

var newGeneratedOutputs = _addOutputsForSources(
buildPhases,
newIds,
rootPackage,
);
var allNewAndDeletedIds = {...newGeneratedOutputs, ...transitiveRemovedIds};

// Transitively invalidates all assets. This needs to happen after the
// structure of the graph has been updated.
var invalidatedIds = <AssetId>{};
final computedOutputs = computeOutputs();

// TODO(davidmorgan): it should be possible to track what needs building in
// the `Build` class instead of this invalidation logic.
void invalidateNodeAndDeps(AssetId startNodeId) {
if (!invalidatedIds.add(startNodeId)) return;
var nodesToInvalidate = [startNodeId];

while (nodesToInvalidate.isNotEmpty) {
final nextNodes = nodesToInvalidate;
nodesToInvalidate = [];
for (final id in nextNodes) {
final invalidatedNode = updateNodeIfPresent(id, (nodeBuilder) {
if (nodeBuilder.type == NodeType.generated) {
final nodeState = nodeBuilder.generatedNodeState;
if (nodeState.pendingBuildAction == PendingBuildAction.none) {
nodeBuilder.generatedNodeState.pendingBuildAction =
PendingBuildAction.buildIfInputsChanged;
}
} else if (nodeBuilder.type == NodeType.glob) {
final nodeState = nodeBuilder.globNodeState;
if (nodeState.pendingBuildAction == PendingBuildAction.none) {
nodeBuilder.globNodeState.pendingBuildAction =
PendingBuildAction.buildIfInputsChanged;
}
}
});

if (invalidatedNode != null) {
for (final id
in (computedOutputs[invalidatedNode.id] ?? const <AssetId>{})) {
if (invalidatedIds.add(id)) {
nodesToInvalidate.add(id);
}
}
}
}
}
}

for (var changed in updates.keys.followedBy(newGeneratedOutputs)) {
invalidateNodeAndDeps(changed);
}

// Invalidate nodes that depend on nodes that will be rebuilt due to changed
// build options.
if (previousInBuildPhasesOptionsDigests != null) {
final invalidatedPhases = <int>{};
for (var i = 0; i != buildPhases.inBuildPhases.length; i++) {
if (previousInBuildPhasesOptionsDigests![i] !=
inBuildPhasesOptionsDigests[i]) {
invalidatedPhases.add(i);
}
}
for (final node in allNodes) {
if (node.type == NodeType.generated &&
invalidatedPhases.contains(
node.generatedNodeConfiguration!.phaseNumber,
)) {
invalidateNodeAndDeps(node.id);
}
}
}

// For all new or deleted assets, check if they match any glob nodes and
// invalidate those.
for (var id in allNewAndDeletedIds) {
var samePackageGlobNodes = packageNodes(id.package).where(
(n) =>
n.type == NodeType.glob &&
n.globNodeState!.pendingBuildAction == PendingBuildAction.none,
);
for (final node in samePackageGlobNodes) {
final nodeConfiguration = node.globNodeConfiguration!;
final glob = Glob(nodeConfiguration.glob);
if (glob.matches(id.path)) {
invalidateNodeAndDeps(node.id);
updateNode(node.id, (nodeBuilder) {
nodeBuilder.globNodeState.pendingBuildAction =
PendingBuildAction.buildIfInputsChanged;
});
}
}
}
_addOutputsForSources(buildPhases, newIds, rootPackage);

// Delete all the invalidated assets, then remove them from the graph. This
// order is important because some `AssetWriter`s throw if the id is not in
Expand All @@ -582,13 +484,11 @@ class AssetGraph implements GeneratedAssetHider {
for (final id in removeIds) {
final node = get(id);
if (node != null && node.type == NodeType.source) {
invalidateNodeAndDeps(id);
_removeRecursive(id);
}
}

_outputs = null;
return invalidatedIds;
}

/// Crawl up primary inputs to see if the original Source file matches the
Expand Down Expand Up @@ -622,7 +522,7 @@ class AssetGraph implements GeneratedAssetHider {
///
/// If [placeholders] is supplied they will be added to [newSources] to create
/// the full input set.
Set<AssetId> _addOutputsForSources(
void _addOutputsForSources(
BuildPhases buildPhases,
Set<AssetId> newSources,
String rootPackage, {
Expand All @@ -646,7 +546,6 @@ class AssetGraph implements GeneratedAssetHider {
);
}
_addPostBuildActionApplications(buildPhases.postBuildPhase, allInputs);
return allInputs;
}

/// Adds all [AssetNode.generated]s for [phase] given [allInputs].
Expand Down Expand Up @@ -752,9 +651,6 @@ class AssetGraph implements GeneratedAssetHider {
output,
phaseNumber: phaseNumber,
primaryInput: primaryInput,
pendingBuildAction: PendingBuildAction.build,
wasOutput: false,
isFailure: false,
isHidden: isHidden,
);
if (existing != null) {
Expand Down Expand Up @@ -820,8 +716,7 @@ class AssetGraph implements GeneratedAssetHider {
for (final id in outputs) {
var node = get(id)!;
final nodeConfiguration = node.generatedNodeConfiguration!;
final nodeState = node.generatedNodeState!;
if (nodeState.wasOutput && !nodeConfiguration.isHidden) {
if (node.wasOutput && !nodeConfiguration.isHidden) {
var idToDelete = id;
// If the package no longer exists, then the user must have
// renamed the root package.
Expand Down
Loading
Loading