[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)

packages/flutter_tools/bin/fuchsia_attach.dart

@@ -98,7 +98,7 @@ Future<void> main(List<String> args) async {
   Cache.disableLocking(); // ignore: invalid_use_of_visible_for_testing_member
   await runner.run(
     command,
-    <FlutterCommand>[
+    () => <FlutterCommand>[
       _FuchsiaAttachCommand(),
       _FuchsiaDoctorCommand(), // If attach fails the tool will attempt to run doctor.
     ],

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<void> main(List<String> 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, <FlutterCommand>[
+  await runner.run(args, () => <FlutterCommand>[
     AnalyzeCommand(
       verboseHelp: verboseHelp,
       fileSystem: globals.fs,
@@ -122,5 +124,14 @@ Future<void> main(List<String> 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,
+        ))
      });
 }

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<FlutterCommand>` or `List<FlutterCommand> Function()`.
+// TODO(jonahwilliams): update command type once g3 has rolled.
 Future<int> run(
   List<String> args,
-  List<FlutterCommand> commands, {
+  dynamic commands, {
     bool muteCommandLogging = false,
     bool verbose = false,
     bool verboseHelp = false,
@@ -39,12 +42,17 @@ Future<int> run(
     args = List<String>.of(args);
     args.removeWhere((String option) => option == '-v' || option == '--verbose');
   }
-
-  final FlutterCommandRunner runner = FlutterCommandRunner(verboseHelp: verboseHelp);
-  commands.forEach(runner.addCommand);
+  List<FlutterCommand> Function() commandGenerator;
+  if (commands is List<FlutterCommand>) {
+    commandGenerator = () => commands;
+  } else {
+    commandGenerator = commands as List<FlutterCommand> Function();
+  }
 
   return runInContext<int>(() 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();

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,

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;

packages/flutter_tools/lib/src/commands/daemon.dart

@@ -52,27 +52,16 @@ class DaemonCommand extends FlutterCommand {
   Future<FlutterCommandResult> runCommand() async {
     globals.printStatus('Starting device daemon...');
     isRunningFromDaemon = true;
-
-    final NotifyingLogger notifyingLogger = NotifyingLogger();
-
     Cache.releaseLockEarly();
 
-    await context.run<void>(
-      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: <Type, Generator>{
-        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<LogMessage> _messageController = StreamController<LogMessage>.broadcast();
+  NotifyingLogger({ @required this.verbose }) {
+    _messageController = StreamController<LogMessage>.broadcast(
+      onListen: _onListen,
+    );
+  }
+
+  final bool verbose;
+  final List<LogMessage> messageBuffer = <LogMessage>[];
+  StreamController<LogMessage> _messageController;
+
+  void _onListen() {
+    if (messageBuffer.isNotEmpty) {
+      messageBuffer.forEach(_messageController.add);
+      messageBuffer.clear();
+    }
+  }
 
   Stream<LogMessage> 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();
   }

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;

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,
     ),
   ]);
 

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<void> {
   Future<void> runCommand(ArgResults topLevelResults) async {
     final Map<Type, dynamic> contextOverrides = <Type, dynamic>{};
 
-    // 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.

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);

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<LogMessage> 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<LogMessage> 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(

packages/flutter_tools/test/general.shard/runner/runner_test.dart

@@ -44,7 +44,7 @@ void main() {
         () {
           unawaited(runner.run(
             <String>['test'],
-            <FlutterCommand>[
+            () => <FlutterCommand>[
               CrashingFlutterCommand(),
             ],
             // This flutterVersion disables crash reporting.
@@ -86,7 +86,7 @@ void main() {
         () {
         unawaited(runner.run(
           <String>['test'],
-          <FlutterCommand>[
+          () => <FlutterCommand>[
             CrashingFlutterCommand(),
           ],
           // This flutterVersion disables crash reporting.

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: <String, String>{}
-      ),
-      processManager:  FakeProcessManager.any(),
-    );
-
-    final List<Device> 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: <String, String>{
-        '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),

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<String, Object> json) => json['id'] == 1);
     expect(response['id'], 1);
     expect(response['error'], isNull);
 

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<void> 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<void> runCommand(ArgResults topLevelResults) async {
+    final Logger topLevelLogger = globals.logger;
+    final Map<Type, dynamic> contextOverrides = <Type, dynamic>{
+      if (topLevelResults['verbose'] as bool)
+        Logger: VerboseLogger(topLevelLogger),
+    };
+    return context.run<void>(
+      overrides: contextOverrides.map<Type, Generator>((Type type, dynamic value) {
+        return MapEntry<Type, Generator>(type, () => value);
+      }),
+      body: () => super.runCommand(topLevelResults),
+    );
+  }
+}