Build daemon ChangeProvider cleanup (#3470)

Removes all methods from ChangeProvider, and extract only the relevant ones into separate subclasses which are now public (AutoChangeProvider vs ManualChangeProvider).

Previously these modes were conflated and the different impls provided essentially no-op implementations of the non-applicable apis.

This should fix the failures we are seeing on the bots, because the daemon was shutting down early due to an empty stream given by the manual change provider. This was broken in #3411, but we only recently published that change, and weren't using dependency overrides for the tests that were actually broken, so we only saw it after the publish.

build_daemon/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 4.0.0-dev
+
+- **Breaking**: Remove methods from ChangeProvider, and extract them into
+  explicit AutoChangeProvider and ManualChangeProvider types.
+
 ## 3.1.1
 
 - Report file watching errors and stop the daemon.

build_daemon/lib/change_provider.dart

@@ -6,13 +6,12 @@ import 'dart:async';
 
 import 'package:watcher/watcher.dart' show WatchEvent;
 
-abstract class ChangeProvider {
-  /// Returns a list of file changes.
-  ///
-  /// Called immediately before a manual build. If the list is empty a no-op
-  /// build of all tracked targets will be attempted.
-  Future<List<WatchEvent>> collectChanges();
+/// Marker class for automatic and manual change providers, essentially acts
+/// as a union type.
+abstract class ChangeProvider {}
 
+/// A change provider which provides a stream of changes.
+abstract class AutoChangeProvider implements ChangeProvider {
   /// A stream of file changes.
   ///
   /// A build is triggered upon each stream event.
@@ -21,3 +20,12 @@ abstract class ChangeProvider {
   /// event. Otherwise, at least two builds will be triggered.
   Stream<List<WatchEvent>> get changes;
 }
+
+/// A change provider which returns a list of changes on demand.
+abstract class ManualChangeProvider implements ChangeProvider {
+  /// Returns a list of file changes.
+  ///
+  /// Called immediately before a manual build. If the list is empty a no-op
+  /// build of all tracked targets will be attempted.
+  Future<List<WatchEvent>> collectChanges();
+}

build_daemon/lib/constants.dart

@@ -36,7 +36,7 @@ const logStartMarker = 'BUILD DAEMON LOG START';
 const logEndMarker = 'BUILD DAEMON LOG END';
 
 // TODO(grouma) - use pubspec version when this is open sourced.
-const currentVersion = '8';
+const currentVersion = '9';
 
 var _username = Platform.environment['USER'] ?? '';
 String daemonWorkspace(String workingDirectory) {

build_daemon/lib/src/fakes/fake_change_provider.dart

@@ -6,7 +6,7 @@ import 'dart:async';
 import 'package:build_daemon/change_provider.dart';
 import 'package:watcher/watcher.dart' show WatchEvent;
 
-class FakeChangeProvider implements ChangeProvider {
+class FakeChangeProvider implements AutoChangeProvider, ManualChangeProvider {
   final changeStreamController = StreamController<List<WatchEvent>>();
   @override
   Stream<List<WatchEvent>> get changes => changeStreamController.stream;

build_daemon/lib/src/server.dart

@@ -56,7 +56,9 @@ class Server {
             BuildTargetManager(shouldBuildOverride: shouldBuild) {
     _logs = _outputStreamController.stream;
     _forwardData();
-    _handleChanges(changeProvider.changes);
+    if (changeProvider is AutoChangeProvider) {
+      _handleChanges(changeProvider.changes);
+    }
     // Stop the server if nobody connects.
     _timeout = Timer(timeout, () async {
       if (_buildTargetManager.isEmpty) {
@@ -82,7 +84,12 @@ class Server {
         if (request is BuildTargetRequest) {
           _buildTargetManager.addBuildTarget(request.target, channel);
         } else if (request is BuildRequest) {
-          var changes = await _changeProvider.collectChanges();
+          // We can only get explicit build requests if we have a manual change
+          // provider.
+          var changeProvider = _changeProvider;
+          var changes = changeProvider is ManualChangeProvider
+              ? await changeProvider.collectChanges()
+              : <WatchEvent>[];
           var targets = changes.isEmpty
               ? _buildTargetManager.targets
               : _buildTargetManager.targetsForChanges(changes);

build_daemon/pubspec.yaml

@@ -1,5 +1,5 @@
 name: build_daemon
-version:  3.1.1
+version:  4.0.0-dev
 description: A daemon for running Dart builds.
 repository: https://github.com/dart-lang/build/tree/master/build_daemon
 
@@ -29,3 +29,7 @@ dev_dependencies:
   test: ^1.16.0
   test_descriptor: ^2.0.0
   uuid: ^3.0.0
+
+dependency_overrides:
+  build_runner:
+    path: ../build_runner

build_runner/CHANGELOG.md

@@ -1,6 +1,7 @@
 ## 2.4.1-dev
 
 - Mention --build-filter option in the README.
+- Update to build_daemon 4.0.
 
 ## 2.4.0
 

build_runner/lib/src/daemon/change_providers.dart

@@ -2,6 +2,8 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'dart:async';
+
 import 'package:build_daemon/change_provider.dart';
 // ignore: implementation_imports
 import 'package:build_runner_core/src/asset_graph/graph.dart';
@@ -11,33 +13,25 @@ import 'package:watcher/watcher.dart' show WatchEvent;
 
 /// Continually updates the [changes] stream as watch events are seen on the
 /// input stream.
-///
-/// The [collectChanges] method is a no-op for this implementation.
-class AutoChangeProvider implements ChangeProvider {
+class AutoChangeProviderImpl implements AutoChangeProvider {
   @override
   final Stream<List<WatchEvent>> changes;
 
-  AutoChangeProvider(this.changes);
-
-  @override
-  Future<List<WatchEvent>> collectChanges() async => [];
+  AutoChangeProviderImpl(this.changes);
 }
 
 /// Computes changes with a file scan when requested by a call to
 /// [collectChanges].
-class ManualChangeProvider implements ChangeProvider {
+class ManualChangeProviderImpl implements ManualChangeProvider {
   final AssetGraph _assetGraph;
   final AssetTracker _assetTracker;
 
-  ManualChangeProvider(this._assetTracker, this._assetGraph);
+  ManualChangeProviderImpl(this._assetTracker, this._assetGraph);
 
   @override
   Future<List<WatchEvent>> collectChanges() async {
     var updates = await _assetTracker.collectChanges(_assetGraph);
     return List.of(updates.entries
         .map((entry) => WatchEvent(entry.value, '${entry.key}')));
   }
-
-  @override
-  Stream<List<WatchEvent>> get changes => Stream.empty();
 }

build_runner/lib/src/daemon/daemon_builder.dart

@@ -249,8 +249,8 @@ class BuildRunnerDaemonBuilder implements DaemonBuilder {
         .debounceBuffer(buildOptions.debounceDelay);
 
     var changeProvider = daemonOptions.buildMode == BuildMode.Auto
-        ? AutoChangeProvider(graphEvents())
-        : ManualChangeProvider(
+        ? AutoChangeProviderImpl(graphEvents())
+        : ManualChangeProviderImpl(
             AssetTracker(daemonEnvironment.reader, buildOptions.targetGraph),
             builder.assetGraph);
 

build_runner/pubspec.yaml

@@ -17,7 +17,7 @@ dependencies:
   analyzer: '>=4.4.0 <6.0.0'
   build: ">=2.1.0 <2.4.0"
   build_config: ">=1.1.0 <1.2.0"
-  build_daemon: ^3.1.0
+  build_daemon: ^4.0.0
   build_resolvers: ^2.0.0
   build_runner_core: ^7.2.0
   code_builder: ^4.2.0