Updates handling (#3933)

* Move handling of updates always to `Build`.

* Fix test.

Author: davidmorgan

build_runner_core/lib/src/generate/build_definition.dart

@@ -34,7 +34,14 @@ class BuildDefinition {
   final AssetGraph assetGraph;
   final BuildScriptUpdates? buildScriptUpdates;
 
-  BuildDefinition._(this.assetGraph, this.buildScriptUpdates);
+  /// When reusing serialized state from a previous build: the file updates
+  /// since that build.
+  ///
+  /// Or, `null` if there was no serialized state or it was discared due to
+  /// the current build having an incompatible change.
+  final Map<AssetId, ChangeType>? updates;
+
+  BuildDefinition._(this.assetGraph, this.buildScriptUpdates, this.updates);
 
   static Future<BuildDefinition> prepareWorkspace(
     BuildEnvironment environment,
@@ -70,11 +77,12 @@ class _Loader {
     var internalSources = await assetTracker.findInternalSources();
 
     BuildScriptUpdates? buildScriptUpdates;
+    Map<AssetId, ChangeType>? updates;
     if (assetGraph != null) {
-      var updates = await logTimedAsync(
+      updates = await logTimedAsync(
         _logger,
         'Checking for updates since last build',
-        () => _updateAssetGraph(
+        () => _computeUpdates(
           assetGraph!,
           assetTracker,
           _buildPhases,
@@ -92,7 +100,7 @@ class _Loader {
 
       var buildScriptUpdated =
           !_options.skipBuildScriptCheck &&
-          buildScriptUpdates.hasBeenUpdated(updates.keys.toSet());
+          buildScriptUpdates.hasBeenUpdated(updates!.keys.toSet());
       if (buildScriptUpdated) {
         _logger.warning('Invalidating asset graph due to build script update!');
 
@@ -110,6 +118,7 @@ class _Loader {
         inputSources.removeAll(deletedSourceOutputs);
         assetGraph = null;
         buildScriptUpdates = null;
+        updates = null;
       }
     }
 
@@ -167,7 +176,7 @@ class _Loader {
       );
     }
 
-    return BuildDefinition._(assetGraph!, buildScriptUpdates);
+    return BuildDefinition._(assetGraph!, buildScriptUpdates, updates);
   }
 
   /// Deletes the generated output directory.
@@ -178,11 +187,9 @@ class _Loader {
     }
   }
 
-  /// Updates [assetGraph] based on a the new view of the world.
-  ///
-  /// Once done, this returns a map of [AssetId] to [ChangeType] for all the
-  /// changes.
-  Future<Map<AssetId, ChangeType>> _updateAssetGraph(
+  /// Returns which sources and builder options changed, and the [ChangeType]
+  /// describing whether they where added, removed or modified.
+  Future<Map<AssetId, ChangeType>> _computeUpdates(
     AssetGraph assetGraph,
     AssetTracker assetTracker,
     BuildPhases buildPhases,
@@ -197,15 +204,6 @@ class _Loader {
       assetGraph,
     );
     updates.addAll(_computeBuilderOptionsUpdates(assetGraph, buildPhases));
-    await assetGraph.updateAndInvalidate(
-      _buildPhases,
-      updates,
-      _options.packageGraph.root.name,
-      (id) => _environment.writer
-          .copyWith(generatedAssetHider: assetGraph)
-          .delete(id),
-      _environment.reader.copyWith(generatedAssetHider: assetGraph),
-    );
     return updates;
   }
 

build_runner_core/lib/src/generate/build_series.dart

@@ -50,6 +50,16 @@ class BuildSeries {
   final RunnerAssetWriter deleteWriter;
   final ResourceManager resourceManager = ResourceManager();
 
+  /// For the first build only, updates from the previous serialized build
+  /// state.
+  ///
+  /// Null after the first build, or if there was no serialized build state, or
+  /// if the serialized build state was discarded.
+  Map<AssetId, ChangeType>? updatesFromLoad;
+
+  /// Whether the next build is the first build.
+  bool firstBuild = true;
+
   Future<void> beforeExit() => resourceManager.beforeExit();
 
   BuildSeries._(
@@ -59,6 +69,7 @@ class BuildSeries {
     this.options,
     this.buildPhases,
     this.finalizedReader,
+    this.updatesFromLoad,
   ) : deleteWriter = environment.writer.copyWith(
         generatedAssetHider: assetGraph,
       ),
@@ -72,16 +83,29 @@ class BuildSeries {
 
   /// Runs a single build.
   ///
-  /// For the first build, pass empty [updates].
+  /// For the first build, pass any changes since the `BuildSeries` was created
+  /// as [updates]. If the first build happens immediately then pass empty
+  /// `updates`.
   ///
-  /// For subsequent builds, pass filesystem changes in [updates].
-  ///
-  /// TODO(davidmorgan): unify these two codepaths.
+  /// For further builds, pass the changes since the previous builds as
+  /// [updates].
   Future<BuildResult> run(
     Map<AssetId, ChangeType> updates, {
     Set<BuildDirectory> buildDirs = const <BuildDirectory>{},
     Set<BuildFilter> buildFilters = const {},
   }) async {
+    if (firstBuild) {
+      if (updatesFromLoad != null) {
+        updates = updatesFromLoad!..addAll(updates);
+        updatesFromLoad = null;
+      }
+      firstBuild = false;
+    } else {
+      if (updatesFromLoad != null) {
+        throw StateError('Only first build can have updates from load.');
+      }
+    }
+
     finalizedReader.reset(BuildDirectory.buildPaths(buildDirs), buildFilters);
     final build = Build(
       environment: environment,
@@ -142,6 +166,7 @@ class BuildSeries {
       options,
       buildPhases,
       finalizedReader,
+      buildDefinition.updates,
     );
     return build;
   }

build_runner_core/test/generate/build_definition_test.dart

@@ -23,6 +23,7 @@ import 'package:package_config/package_config.dart';
 import 'package:path/path.dart' as p;
 import 'package:test/test.dart';
 import 'package:test_descriptor/test_descriptor.dart' as d;
+import 'package:watcher/watcher.dart';
 
 void main() {
   final languageVersion = LanguageVersion(2, 0);
@@ -138,7 +139,7 @@ targets:
       addTearDown(options.logListener.cancel);
     });
 
-    group('updates the asset graph', () {
+    group('reports updates', () {
       test('for deleted source and generated nodes', () async {
         await createFile(p.join('lib', 'a.txt'), 'a');
         await createFile(p.join('lib', 'b.txt'), 'b');
@@ -169,23 +170,15 @@ targets:
           options,
           buildPhases,
         );
-        var newAssetGraph = buildDefinition.assetGraph;
-
-        var generatedANode = newAssetGraph.get(generatedAId)!;
-        expect(generatedANode, isNotNull);
-        expect(
-          generatedANode.generatedNodeState!.pendingBuildAction,
-          PendingBuildAction.build,
-        );
 
-        expect(newAssetGraph.contains(makeAssetId('a|lib/b.txt')), isFalse);
+        expect(buildDefinition.updates![generatedAId], ChangeType.REMOVE);
         expect(
-          newAssetGraph.contains(makeAssetId('a|lib/b.txt.copy')),
-          isFalse,
+          buildDefinition.updates![makeAssetId('a|lib/b.txt')],
+          ChangeType.REMOVE,
         );
       });
 
-      test('for new sources and generated nodes', () async {
+      test('for new sources', () async {
         var buildPhases = BuildPhases([
           InBuildPhase(TestBuilder(), 'a', hideOutput: true),
         ]);
@@ -206,17 +199,10 @@ targets:
           options,
           buildPhases,
         );
-        var newAssetGraph = buildDefinition.assetGraph;
 
-        expect(newAssetGraph.contains(makeAssetId('a|lib/a.txt')), isTrue);
-
-        var generatedANode =
-            newAssetGraph.get(makeAssetId('a|lib/a.txt.copy'))!;
-        expect(generatedANode, isNotNull);
-        // New nodes definitely need an update.
         expect(
-          generatedANode.generatedNodeState!.pendingBuildAction,
-          PendingBuildAction.build,
+          buildDefinition.updates![makeAssetId('a|lib/a.txt')],
+          ChangeType.ADD,
         );
       });
 
@@ -253,14 +239,10 @@ targets:
           options,
           buildPhases,
         );
-        var newAssetGraph = buildDefinition.assetGraph;
 
-        var generatedANode =
-            newAssetGraph.get(makeAssetId('a|lib/a.txt.copy'))!;
-        expect(generatedANode, isNotNull);
         expect(
-          generatedANode.generatedNodeState!.pendingBuildAction,
-          PendingBuildAction.buildIfInputsChanged,
+          buildDefinition.updates![makeAssetId('a|lib/a.txt')],
+          ChangeType.MODIFY,
         );
       });
 
@@ -355,22 +337,27 @@ targets:
           options,
           newBuildPhases,
         );
+
         var newAssetGraph = buildDefinition.assetGraph;
 
         // The *.copy node should be invalidated, its builder options changed.
         var generatedACopyNode = newAssetGraph.get(generatedACopyId)!;
-        expect(generatedACopyNode, isNotNull);
         expect(
-          generatedACopyNode.generatedNodeState!.pendingBuildAction,
-          PendingBuildAction.buildIfInputsChanged,
+          buildDefinition.updates![generatedACopyNode
+              .generatedNodeConfiguration!
+              .builderOptionsId],
+          ChangeType.MODIFY,
         );
 
         // But the *.clone node should remain the same since its options didn't.
         var generatedACloneNode = newAssetGraph.get(generatedACloneId)!;
-        expect(generatedACloneNode, isNotNull);
         expect(
-          generatedACloneNode.generatedNodeState!.pendingBuildAction,
-          PendingBuildAction.none,
+          buildDefinition.updates!.keys,
+          isNot(
+            contains(
+              generatedACloneNode.generatedNodeConfiguration!.builderOptionsId,
+            ),
+          ),
         );
       });
     });