From f5de6aadd4761810b24e128bbb2b4e02133f1286 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 4 Jun 2020 16:35:36 -0700 Subject: [PATCH] [flutter_tools] remove zone level overrides of verbose and daemon logging (#57448) Make it possible for all FlutterCommands to be global free, by moving instantiation to inside the Zone context. Additionally, provide VerboseLogger and NotifyLogger (daemon) at the top level and remove from command-specific overrides. This allows removing a work around where web devices needed to look up directly from the context in non-test code. Technically the output preferences are still zone injected, but these will be moved soon as they were not being used correctly by the top level command (the injection comes after ArgParser reads the overflow values, causing numerous wrap issues) --- .../flutter_tools/bin/fuchsia_attach.dart | 2 +- packages/flutter_tools/lib/executable.dart | 13 ++++- packages/flutter_tools/lib/runner.dart | 16 +++-- .../lib/src/commands/analyze.dart | 9 +-- .../lib/src/commands/attach.dart | 2 +- .../lib/src/commands/daemon.dart | 58 ++++++++++++------- .../flutter_tools/lib/src/commands/run.dart | 2 +- packages/flutter_tools/lib/src/device.dart | 1 + .../src/runner/flutter_command_runner.dart | 7 --- .../flutter_tools/lib/src/web/web_device.dart | 16 ++--- .../commands.shard/hermetic/daemon_test.dart | 38 +++++++++++- .../general.shard/runner/runner_test.dart | 4 +- .../test/general.shard/web/devices_test.dart | 32 ---------- .../integration.shard/daemon_mode_test.dart | 2 +- packages/flutter_tools/test/src/common.dart | 21 ++++++- 15 files changed, 131 insertions(+), 92 deletions(-) diff --git a/packages/flutter_tools/bin/fuchsia_attach.dart b/packages/flutter_tools/bin/fuchsia_attach.dart index 59e00fe3030..09582d60f4e 100644 --- a/packages/flutter_tools/bin/fuchsia_attach.dart +++ b/packages/flutter_tools/bin/fuchsia_attach.dart @@ -98,7 +98,7 @@ Future main(List args) async { Cache.disableLocking(); // ignore: invalid_use_of_visible_for_testing_member await runner.run( command, - [ + () => [ _FuchsiaAttachCommand(), _FuchsiaDoctorCommand(), // If attach fails the tool will attempt to run doctor. ], diff --git a/packages/flutter_tools/lib/executable.dart b/packages/flutter_tools/lib/executable.dart index 08905a7f1fc..bf994bea3bb 100644 --- a/packages/flutter_tools/lib/executable.dart +++ b/packages/flutter_tools/lib/executable.dart @@ -6,6 +6,7 @@ import 'dart:async'; import 'runner.dart' as runner; import 'src/base/context.dart'; +import 'src/base/logger.dart'; import 'src/base/template.dart'; // The build_runner code generation is provided here to make it easier to // avoid introducing the dependency into google3. Not all build* packages @@ -64,8 +65,9 @@ Future main(List args) async { (args.isNotEmpty && args.first == 'help') || (args.length == 1 && verbose); final bool muteCommandLogging = help || doctor; final bool verboseHelp = help && verbose; + final bool daemon = args.contains('daemon'); - await runner.run(args, [ + await runner.run(args, () => [ AnalyzeCommand( verboseHelp: verboseHelp, fileSystem: globals.fs, @@ -122,5 +124,14 @@ Future main(List args) async { WebRunnerFactory: () => DwdsWebRunnerFactory(), // The mustache dependency is different in google3 TemplateRenderer: () => const MustacheTemplateRenderer(), + if (daemon) + Logger: () => NotifyingLogger(verbose: verbose) + else if (verbose) + Logger: () => VerboseLogger(StdoutLogger( + timeoutConfiguration: timeoutConfiguration, + stdio: globals.stdio, + terminal: globals.terminal, + outputPreferences: globals.outputPreferences, + )) }); } diff --git a/packages/flutter_tools/lib/runner.dart b/packages/flutter_tools/lib/runner.dart index cb7f957edd7..d40eda237f6 100644 --- a/packages/flutter_tools/lib/runner.dart +++ b/packages/flutter_tools/lib/runner.dart @@ -23,9 +23,12 @@ import 'src/runner/flutter_command.dart'; import 'src/runner/flutter_command_runner.dart'; /// Runs the Flutter tool with support for the specified list of [commands]. +/// +/// [commands] must be either `List` or `List Function()`. +// TODO(jonahwilliams): update command type once g3 has rolled. Future run( List args, - List commands, { + dynamic commands, { bool muteCommandLogging = false, bool verbose = false, bool verboseHelp = false, @@ -39,12 +42,17 @@ Future run( args = List.of(args); args.removeWhere((String option) => option == '-v' || option == '--verbose'); } - - final FlutterCommandRunner runner = FlutterCommandRunner(verboseHelp: verboseHelp); - commands.forEach(runner.addCommand); + List Function() commandGenerator; + if (commands is List) { + commandGenerator = () => commands; + } else { + commandGenerator = commands as List Function(); + } return runInContext(() async { reportCrashes ??= !await globals.isRunningOnBot; + final FlutterCommandRunner runner = FlutterCommandRunner(verboseHelp: verboseHelp); + commandGenerator().forEach(runner.addCommand); // Initialize the system locale. final String systemLocale = await intl_standalone.findSystemLocale(); diff --git a/packages/flutter_tools/lib/src/commands/analyze.dart b/packages/flutter_tools/lib/src/commands/analyze.dart index 969a2c5fc82..36f314aea5a 100644 --- a/packages/flutter_tools/lib/src/commands/analyze.dart +++ b/packages/flutter_tools/lib/src/commands/analyze.dart @@ -11,7 +11,6 @@ import '../base/file_system.dart'; import '../base/logger.dart'; import '../base/platform.dart'; import '../base/terminal.dart'; -import '../globals.dart' as globals; import '../runner/flutter_command.dart'; import 'analyze_continuously.dart'; import 'analyze_once.dart'; @@ -113,9 +112,7 @@ class AnalyzeCommand extends FlutterCommand { runner.getRepoRoots(), runner.getRepoPackages(), fileSystem: _fileSystem, - // TODO(jonahwilliams): determine a better way to inject the logger, - // since it is constructed on-demand. - logger: _logger ?? globals.logger, + logger: _logger, platform: _platform, processManager: _processManager, terminal: _terminal, @@ -128,9 +125,7 @@ class AnalyzeCommand extends FlutterCommand { runner.getRepoPackages(), workingDirectory: workingDirectory, fileSystem: _fileSystem, - // TODO(jonahwilliams): determine a better way to inject the logger, - // since it is constructed on-demand. - logger: _logger ?? globals.logger, + logger: _logger, platform: _platform, processManager: _processManager, terminal: _terminal, diff --git a/packages/flutter_tools/lib/src/commands/attach.dart b/packages/flutter_tools/lib/src/commands/attach.dart index e6d201ef634..33eacafb871 100644 --- a/packages/flutter_tools/lib/src/commands/attach.dart +++ b/packages/flutter_tools/lib/src/commands/attach.dart @@ -200,7 +200,7 @@ class AttachCommand extends FlutterCommand { ? Daemon( stdinCommandStream, stdoutCommandResponse, - notifyingLogger: NotifyingLogger(), + notifyingLogger: NotifyingLogger(verbose: globals.logger.isVerbose), logToStdout: true, ) : null; diff --git a/packages/flutter_tools/lib/src/commands/daemon.dart b/packages/flutter_tools/lib/src/commands/daemon.dart index 6147d961d20..1650d87070e 100644 --- a/packages/flutter_tools/lib/src/commands/daemon.dart +++ b/packages/flutter_tools/lib/src/commands/daemon.dart @@ -52,27 +52,16 @@ class DaemonCommand extends FlutterCommand { Future runCommand() async { globals.printStatus('Starting device daemon...'); isRunningFromDaemon = true; - - final NotifyingLogger notifyingLogger = NotifyingLogger(); - Cache.releaseLockEarly(); - await context.run( - body: () async { - final Daemon daemon = Daemon( - stdinCommandStream, stdoutCommandResponse, - notifyingLogger: notifyingLogger, - ); - - final int code = await daemon.onExit; - if (code != 0) { - throwToolExit('Daemon exited with non-zero exit code: $code', exitCode: code); - } - }, - overrides: { - Logger: () => notifyingLogger, - }, + final Daemon daemon = Daemon( + stdinCommandStream, stdoutCommandResponse, + notifyingLogger: globals.logger as NotifyingLogger, ); + final int code = await daemon.onExit; + if (code != 0) { + throwToolExit('Daemon exited with non-zero exit code: $code', exitCode: code); + } return FlutterCommandResult.success(); } } @@ -927,7 +916,22 @@ dynamic _toJsonable(dynamic obj) { } class NotifyingLogger extends Logger { - final StreamController _messageController = StreamController.broadcast(); + NotifyingLogger({ @required this.verbose }) { + _messageController = StreamController.broadcast( + onListen: _onListen, + ); + } + + final bool verbose; + final List messageBuffer = []; + StreamController _messageController; + + void _onListen() { + if (messageBuffer.isNotEmpty) { + messageBuffer.forEach(_messageController.add); + messageBuffer.clear(); + } + } Stream get onMessage => _messageController.stream; @@ -941,7 +945,7 @@ class NotifyingLogger extends Logger { int hangingIndent, bool wrap, }) { - _messageController.add(LogMessage('error', message, stackTrace)); + _sendMessage(LogMessage('error', message, stackTrace)); } @override @@ -954,12 +958,15 @@ class NotifyingLogger extends Logger { int hangingIndent, bool wrap, }) { - _messageController.add(LogMessage('status', message)); + _sendMessage(LogMessage('status', message)); } @override void printTrace(String message) { - // This is a lot of traffic to send over the wire. + if (!verbose) { + return; + } + _sendMessage(LogMessage('trace', message)); } @override @@ -979,6 +986,13 @@ class NotifyingLogger extends Logger { ); } + void _sendMessage(LogMessage logMessage) { + if (_messageController.hasListener) { + return _messageController.add(logMessage); + } + messageBuffer.add(logMessage); + } + void dispose() { _messageController.close(); } diff --git a/packages/flutter_tools/lib/src/commands/run.dart b/packages/flutter_tools/lib/src/commands/run.dart index a5125a8f6f1..2458a73d626 100644 --- a/packages/flutter_tools/lib/src/commands/run.dart +++ b/packages/flutter_tools/lib/src/commands/run.dart @@ -407,7 +407,7 @@ class RunCommand extends RunCommandBase { final Daemon daemon = Daemon( stdinCommandStream, stdoutCommandResponse, - notifyingLogger: NotifyingLogger(), + notifyingLogger: NotifyingLogger(verbose: globals.logger.isVerbose), logToStdout: true, ); AppInstance app; diff --git a/packages/flutter_tools/lib/src/device.dart b/packages/flutter_tools/lib/src/device.dart index 59f5b25f9f4..8612b5bf55a 100644 --- a/packages/flutter_tools/lib/src/device.dart +++ b/packages/flutter_tools/lib/src/device.dart @@ -103,6 +103,7 @@ class DeviceManager { fileSystem: globals.fs, platform: globals.platform, processManager: globals.processManager, + logger: globals.logger, ), ]); diff --git a/packages/flutter_tools/lib/src/runner/flutter_command_runner.dart b/packages/flutter_tools/lib/src/runner/flutter_command_runner.dart index 6730f7accdc..21be933e103 100644 --- a/packages/flutter_tools/lib/src/runner/flutter_command_runner.dart +++ b/packages/flutter_tools/lib/src/runner/flutter_command_runner.dart @@ -15,7 +15,6 @@ import '../artifacts.dart'; import '../base/common.dart'; import '../base/context.dart'; import '../base/file_system.dart'; -import '../base/logger.dart'; import '../base/terminal.dart'; import '../base/user_messages.dart'; import '../base/utils.dart'; @@ -236,12 +235,6 @@ class FlutterCommandRunner extends CommandRunner { Future runCommand(ArgResults topLevelResults) async { final Map contextOverrides = {}; - // Check for verbose. - if (topLevelResults['verbose'] as bool) { - // Override the logger. - contextOverrides[Logger] = VerboseLogger(globals.logger); - } - // Don't set wrapColumns unless the user said to: if it's set, then all // wrapping will occur at this width explicitly, and won't adapt if the // terminal size changes during a run. diff --git a/packages/flutter_tools/lib/src/web/web_device.dart b/packages/flutter_tools/lib/src/web/web_device.dart index 1042ee6a19c..41df82c3fc0 100644 --- a/packages/flutter_tools/lib/src/web/web_device.dart +++ b/packages/flutter_tools/lib/src/web/web_device.dart @@ -14,7 +14,6 @@ import '../base/platform.dart'; import '../build_info.dart'; import '../device.dart'; import '../features.dart'; -import '../globals.dart' as globals; import '../project.dart'; import 'chrome.dart'; @@ -36,7 +35,7 @@ abstract class ChromiumDevice extends Device { @required String name, @required this.chromeLauncher, @required FileSystem fileSystem, - Logger logger, + @required Logger logger, }) : _fileSystem = fileSystem, _logger = logger, super( @@ -49,7 +48,7 @@ abstract class ChromiumDevice extends Device { final ChromiumLauncher chromeLauncher; final FileSystem _fileSystem; - Logger _logger; + final Logger _logger; /// The active chrome instance. Chromium _chrome; @@ -115,7 +114,6 @@ abstract class ChromiumDevice extends Device { bool prebuiltApplication = false, bool ipv6 = false, }) async { - _logger ??= globals.logger; // See [ResidentWebRunner.run] in flutter_tools/lib/src/resident_web_runner.dart // for the web initialization and server logic. final String url = platformArgs['uri'] as String; @@ -241,10 +239,7 @@ class MicrosoftEdgeDevice extends ChromiumDevice { class WebDevices extends PollingDeviceDiscovery { WebDevices({ @required FileSystem fileSystem, - // TODO(jonahwilliams): the logger is overriden by the daemon command - // to support IDE integration. Accessing the global logger too early - // will grab the old stdout logger. - Logger logger, + @required Logger logger, @required Platform platform, @required ProcessManager processManager, @required FeatureFlags featureFlags, @@ -307,7 +302,7 @@ String parseVersionForWindows(String input) { /// A special device type to allow serving for arbitrary browsers. class WebServerDevice extends Device { WebServerDevice({ - Logger logger, + @required Logger logger, }) : _logger = logger, super( 'web-server', @@ -316,7 +311,7 @@ class WebServerDevice extends Device { ephemeral: false, ); - Logger _logger; + final Logger _logger; @override void clearLogs() { } @@ -375,7 +370,6 @@ class WebServerDevice extends Device { bool prebuiltApplication = false, bool ipv6 = false, }) async { - _logger ??= globals.logger; final String url = platformArgs['uri'] as String; if (debuggingOptions.startPaused) { _logger.printStatus('Waiting for connection from Dart debug extension at $url', emphasis: true); diff --git a/packages/flutter_tools/test/commands.shard/hermetic/daemon_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/daemon_test.dart index 7b377978732..fcab172306e 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/daemon_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/daemon_test.dart @@ -24,7 +24,7 @@ void main() { group('daemon', () { setUp(() { - notifyingLogger = NotifyingLogger(); + notifyingLogger = NotifyingLogger(verbose: false); }); tearDown(() { @@ -298,6 +298,42 @@ void main() { }); }); + testUsingContext('notifyingLogger outputs trace messages in verbose mode', () async { + final NotifyingLogger logger = NotifyingLogger(verbose: true); + + final Future messageResult = logger.onMessage.first; + logger.printTrace('test'); + + final LogMessage message = await messageResult; + + expect(message.level, 'trace'); + expect(message.message, 'test'); + }); + + testUsingContext('notifyingLogger ignores trace messages in non-verbose mode', () async { + final NotifyingLogger logger = NotifyingLogger(verbose: false); + + final Future messageResult = logger.onMessage.first; + logger.printTrace('test'); + logger.printStatus('hello'); + + final LogMessage message = await messageResult; + + expect(message.level, 'status'); + expect(message.message, 'hello'); + }); + + testUsingContext('notifyingLogger buffers messages sent before a subscription', () async { + final NotifyingLogger logger = NotifyingLogger(verbose: false); + + logger.printStatus('hello'); + + final LogMessage message = await logger.onMessage.first; + + expect(message.level, 'status'); + expect(message.message, 'hello'); + }); + group('daemon serialization', () { test('OperationResult', () { expect( diff --git a/packages/flutter_tools/test/general.shard/runner/runner_test.dart b/packages/flutter_tools/test/general.shard/runner/runner_test.dart index cffab93fc63..6c0b536bb90 100644 --- a/packages/flutter_tools/test/general.shard/runner/runner_test.dart +++ b/packages/flutter_tools/test/general.shard/runner/runner_test.dart @@ -44,7 +44,7 @@ void main() { () { unawaited(runner.run( ['test'], - [ + () => [ CrashingFlutterCommand(), ], // This flutterVersion disables crash reporting. @@ -86,7 +86,7 @@ void main() { () { unawaited(runner.run( ['test'], - [ + () => [ CrashingFlutterCommand(), ], // This flutterVersion disables crash reporting. diff --git a/packages/flutter_tools/test/general.shard/web/devices_test.dart b/packages/flutter_tools/test/general.shard/web/devices_test.dart index 2de45584b68..ffbe076ac79 100644 --- a/packages/flutter_tools/test/general.shard/web/devices_test.dart +++ b/packages/flutter_tools/test/general.shard/web/devices_test.dart @@ -5,7 +5,6 @@ import 'package:file/memory.dart'; import 'package:flutter_tools/src/base/logger.dart'; import 'package:flutter_tools/src/base/platform.dart'; -import 'package:flutter_tools/src/build_info.dart'; import 'package:flutter_tools/src/device.dart'; import 'package:flutter_tools/src/web/chrome.dart'; import 'package:flutter_tools/src/web/web_device.dart'; @@ -16,37 +15,6 @@ import '../../src/context.dart'; import '../../src/testbed.dart'; void main() { - testUsingContext('Grabs context logger if no constructor logger is provided', () async { - final WebDevices webDevices = WebDevices( - featureFlags: TestFeatureFlags(isWebEnabled: true), - fileSystem: MemoryFileSystem.test(), - platform: FakePlatform( - operatingSystem: 'linux', - environment: {} - ), - processManager: FakeProcessManager.any(), - ); - - final List devices = await webDevices.pollingGetDevices(); - final WebServerDevice serverDevice = devices.firstWhere((Device device) => device is WebServerDevice) - as WebServerDevice; - - await serverDevice.startApp( - null, - debuggingOptions: DebuggingOptions.enabled( - BuildInfo.debug, - startPaused: true, - ), - platformArgs: { - 'uri': 'foo', - } - ); - - // Verify that the injected testLogger is used. - expect(testLogger.statusText, contains( - 'Waiting for connection from Dart debug extension at foo' - )); - }); testWithoutContext('No web devices listed if feature is disabled', () async { final WebDevices webDevices = WebDevices( featureFlags: TestFeatureFlags(isWebEnabled: false), diff --git a/packages/flutter_tools/test/integration.shard/daemon_mode_test.dart b/packages/flutter_tools/test/integration.shard/daemon_mode_test.dart index fbf41637ce2..686a4127e3d 100644 --- a/packages/flutter_tools/test/integration.shard/daemon_mode_test.dart +++ b/packages/flutter_tools/test/integration.shard/daemon_mode_test.dart @@ -47,7 +47,7 @@ void main() { 'id': 1, 'method': 'device.enable', })}]'); - response = await stream.first; + response = await stream.firstWhere((Map json) => json['id'] == 1); expect(response['id'], 1); expect(response['error'], isNull); diff --git a/packages/flutter_tools/test/src/common.dart b/packages/flutter_tools/test/src/common.dart index 570694c693e..65375d640df 100644 --- a/packages/flutter_tools/test/src/common.dart +++ b/packages/flutter_tools/test/src/common.dart @@ -4,7 +4,9 @@ import 'dart:async'; +import 'package:args/args.dart'; import 'package:args/command_runner.dart'; +import 'package:flutter_tools/src/base/logger.dart'; import 'package:flutter_tools/src/convert.dart'; import 'package:vm_service/vm_service.dart' as vm_service; @@ -78,7 +80,7 @@ String getFlutterRoot() { } CommandRunner createTestCommandRunner([ FlutterCommand command ]) { - final FlutterCommandRunner runner = FlutterCommandRunner(); + final FlutterCommandRunner runner = TestFlutterCommandRunner(); if (command != null) { runner.addCommand(command); } @@ -352,3 +354,20 @@ class FakeVmServiceStreamResponse implements VmServiceExpectation { @override bool get isRequest => false; } + +class TestFlutterCommandRunner extends FlutterCommandRunner { + @override + Future runCommand(ArgResults topLevelResults) async { + final Logger topLevelLogger = globals.logger; + final Map contextOverrides = { + if (topLevelResults['verbose'] as bool) + Logger: VerboseLogger(topLevelLogger), + }; + return context.run( + overrides: contextOverrides.map((Type type, dynamic value) { + return MapEntry(type, () => value); + }), + body: () => super.runCommand(topLevelResults), + ); + } +}