From dbab06b604c4e76800a97a7fedbfd065c3171b75 Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Fri, 10 Mar 2023 13:59:38 -0800 Subject: [PATCH] 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 | 5 +++++ build_daemon/lib/change_provider.dart | 20 +++++++++++++------ build_daemon/lib/constants.dart | 2 +- .../lib/src/fakes/fake_change_provider.dart | 2 +- build_daemon/lib/src/server.dart | 11 ++++++++-- build_daemon/pubspec.yaml | 6 +++++- build_runner/CHANGELOG.md | 1 + .../lib/src/daemon/change_providers.dart | 18 ++++++----------- .../lib/src/daemon/daemon_builder.dart | 4 ++-- build_runner/pubspec.yaml | 2 +- 10 files changed, 45 insertions(+), 26 deletions(-) diff --git a/build_daemon/CHANGELOG.md b/build_daemon/CHANGELOG.md index 451704908..b14bfdc03 100644 --- a/build_daemon/CHANGELOG.md +++ b/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. diff --git a/build_daemon/lib/change_provider.dart b/build_daemon/lib/change_provider.dart index d1be849fc..bd3c18066 100644 --- a/build_daemon/lib/change_provider.dart +++ b/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> 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> 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> collectChanges(); +} diff --git a/build_daemon/lib/constants.dart b/build_daemon/lib/constants.dart index 0e8e7e9fd..5ec60d828 100644 --- a/build_daemon/lib/constants.dart +++ b/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) { diff --git a/build_daemon/lib/src/fakes/fake_change_provider.dart b/build_daemon/lib/src/fakes/fake_change_provider.dart index d95f29491..9d5487648 100644 --- a/build_daemon/lib/src/fakes/fake_change_provider.dart +++ b/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>(); @override Stream> get changes => changeStreamController.stream; diff --git a/build_daemon/lib/src/server.dart b/build_daemon/lib/src/server.dart index cc01b1ae6..a341e3f66 100644 --- a/build_daemon/lib/src/server.dart +++ b/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() + : []; var targets = changes.isEmpty ? _buildTargetManager.targets : _buildTargetManager.targetsForChanges(changes); diff --git a/build_daemon/pubspec.yaml b/build_daemon/pubspec.yaml index a7287326c..017988f63 100644 --- a/build_daemon/pubspec.yaml +++ b/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 diff --git a/build_runner/CHANGELOG.md b/build_runner/CHANGELOG.md index 57691a275..7180fccb4 100644 --- a/build_runner/CHANGELOG.md +++ b/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 diff --git a/build_runner/lib/src/daemon/change_providers.dart b/build_runner/lib/src/daemon/change_providers.dart index 7b7819b73..246eee9d7 100644 --- a/build_runner/lib/src/daemon/change_providers.dart +++ b/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,25 +13,20 @@ 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> changes; - AutoChangeProvider(this.changes); - - @override - Future> 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> collectChanges() async { @@ -37,7 +34,4 @@ class ManualChangeProvider implements ChangeProvider { return List.of(updates.entries .map((entry) => WatchEvent(entry.value, '${entry.key}'))); } - - @override - Stream> get changes => Stream.empty(); } diff --git a/build_runner/lib/src/daemon/daemon_builder.dart b/build_runner/lib/src/daemon/daemon_builder.dart index 4af1c4610..ae30b5942 100644 --- a/build_runner/lib/src/daemon/daemon_builder.dart +++ b/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); diff --git a/build_runner/pubspec.yaml b/build_runner/pubspec.yaml index faf2b0c2c..509a29c62 100644 --- a/build_runner/pubspec.yaml +++ b/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