Merge pull request #1504 from devoncarew/flutter_logs

refactor flutter logs

packages/flutter_tools/lib/src/android/device_android.dart

@@ -294,6 +294,7 @@ class AndroidDevice extends Device {
     String mainPath,
     String route,
     bool checked: true,
+    bool clearLogs: false,
     Map<String, dynamic> platformArgs
   }) {
     return flx.buildInTempDir(
@@ -309,7 +310,7 @@ class AndroidDevice extends Device {
           checked: checked,
           traceStartup: platformArgs['trace-startup'],
           route: route,
-          clearLogs: platformArgs['clear-logs']
+          clearLogs: clearLogs
         )) {
           return true;
         } else {
@@ -334,26 +335,7 @@ class AndroidDevice extends Device {
     runSync(adbCommandForDevice(['logcat', '-c']));
   }
 
-  Future<int> logs({bool clear: false}) async {
-    if (!isConnected()) {
-      return 2;
-    }
-
-    if (clear) {
-      clearLogs();
-    }
-
-    return await runCommandAndStreamOutput(adbCommandForDevice([
-      'logcat',
-      '-v',
-      'tag', // Only log the tag and the message
-      '-s',
-      'flutter:V',
-      'ActivityManager:W',
-      'System.err:W',
-      '*:F',
-    ]), prefix: 'android: ');
-  }
+  DeviceLogReader createLogReader() => new _AdbLogReader(this);
 
   void startTracing(AndroidApk apk) {
     runCheckedSync(adbCommandForDevice([
@@ -529,3 +511,41 @@ String getAdbPath() {
     return _defaultAdbPath;
   }
 }
+
+/// A log reader that logs from `adb logcat`. This will have the same output as
+/// another copy of [_AdbLogReader], and the two instances will be equivalent.
+class _AdbLogReader extends DeviceLogReader {
+  _AdbLogReader(this.device);
+
+  final AndroidDevice device;
+
+  String get name => 'Android';
+
+  Future<int> logs({bool clear: false}) async {
+    if (!device.isConnected())
+      return 2;
+
+    if (clear)
+      device.clearLogs();
+
+    return await runCommandAndStreamOutput(device.adbCommandForDevice(<String>[
+      'logcat',
+      '-v',
+      'tag', // Only log the tag and the message
+      '-s',
+      'flutter:V',
+      'ActivityManager:W',
+      'System.err:W',
+      '*:F',
+    ]), prefix: '[Android] ');
+  }
+
+  // Intentionally constant; overridden because we've overridden the `operator ==` method below.
+  int get hashCode => name.hashCode;
+
+  bool operator ==(dynamic other) {
+    if (identical(this, other))
+      return true;
+    return other is _AdbLogReader;
+  }
+}

packages/flutter_tools/lib/src/base/process.dart

@@ -8,12 +8,15 @@ import 'dart:io';
 
 import 'context.dart';
 
+typedef String StringConverter(String string);
+
 /// This runs the command and streams stdout/stderr from the child process to
 /// this process' stdout/stderr.
 Future<int> runCommandAndStreamOutput(List<String> cmd, {
+  String workingDirectory,
   String prefix: '',
   RegExp filter,
-  String workingDirectory
+  StringConverter mapFunction
 }) async {
   printTrace(cmd.join(' '));
   Process process = await Process.start(
@@ -26,14 +29,20 @@ Future<int> runCommandAndStreamOutput(List<String> cmd, {
     .transform(const LineSplitter())
     .where((String line) => filter == null ? true : filter.hasMatch(line))
     .listen((String line) {
-      printStatus('$prefix$line');
+      if (mapFunction != null)
+        line = mapFunction(line);
+      if (line != null)
+        printStatus('$prefix$line');
     });
   process.stderr
     .transform(UTF8.decoder)
     .transform(const LineSplitter())
     .where((String line) => filter == null ? true : filter.hasMatch(line))
     .listen((String line) {
-      printError('$prefix$line');
+      if (mapFunction != null)
+        line = mapFunction(line);
+      if (line != null)
+        printError('$prefix$line');
     });
   return await process.exitCode;
 }
@@ -57,7 +66,7 @@ Future<Process> runDetached(List<String> cmd) {
 /// Run cmd and return stdout.
 /// Throws an error if cmd exits with a non-zero value.
 String runCheckedSync(List<String> cmd, { String workingDirectory }) {
-  return _runWithLoggingSync(cmd, workingDirectory: workingDirectory, checked: true);
+  return _runWithLoggingSync(cmd, workingDirectory: workingDirectory, checked: true, noisyErrors: true);
 }
 
 /// Run cmd and return stdout.
@@ -73,6 +82,7 @@ String sdkBinaryName(String name) {
 
 String _runWithLoggingSync(List<String> cmd, {
   bool checked: false,
+  bool noisyErrors: false,
   String workingDirectory
 }) {
   printTrace(cmd.join(' '));
@@ -82,8 +92,13 @@ String _runWithLoggingSync(List<String> cmd, {
     String errorDescription = 'Error code ${results.exitCode} '
         'returned when attempting to run command: ${cmd.join(' ')}';
     printTrace(errorDescription);
-    if (results.stderr.length > 0)
-      printTrace('Errors logged: ${results.stderr.trim()}');
+    if (results.stderr.length > 0) {
+      if (noisyErrors) {
+        printError(results.stderr.trim());
+      } else {
+        printTrace('Errors logged: ${results.stderr.trim()}');
+      }
+    }
     if (checked)
       throw errorDescription;
   }

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

@@ -17,8 +17,6 @@ class IOSCommand extends FlutterCommand {
   final String name = "ios";
   final String description = "Commands for creating and updating Flutter iOS projects.";
 
-  final bool requiresProjectRoot = true;
-
   IOSCommand() {
     argParser.addFlag('init', help: 'Initialize the Xcode project for building the iOS application');
   }

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

@@ -4,6 +4,7 @@
 
 import 'dart:async';
 
+import '../base/context.dart';
 import '../device.dart';
 import '../runner/flutter_command.dart';
 
@@ -13,25 +14,52 @@ class LogsCommand extends FlutterCommand {
 
   LogsCommand() {
     argParser.addFlag('clear',
-        negatable: false,
-        abbr: 'c',
-        help: 'Clear log history before reading from logs (Android only).');
+      negatable: false,
+      abbr: 'c',
+      help: 'Clear log history before reading from logs.'
+    );
   }
 
   bool get requiresProjectRoot => false;
 
-  @override
   Future<int> runInProject() async {
-    connectToDevices();
+    DeviceManager deviceManager = new DeviceManager();
+    List<Device> devices;
+
+    String deviceId = globalResults['device-id'];
+    if (deviceId != null) {
+      Device device = await deviceManager.getDeviceById(deviceId);
+      if (device == null) {
+        printError("No device found with id '$deviceId'.");
+        return 1;
+      }
+      devices = <Device>[device];
+    } else {
+      devices = await deviceManager.getDevices();
+    }
+
+    if (devices.isEmpty) {
+      printStatus('No connected devices.');
+      return 0;
+    }
 
     bool clear = argResults['clear'];
 
-    Iterable<Future<int>> results = devices.all.map(
-        (Device device) => device.logs(clear: clear));
+    Set<DeviceLogReader> readers = new Set<DeviceLogReader>();
+    for (Device device in devices) {
+      readers.add(device.createLogReader());
+    }
+
+    printStatus('Logging for ${readers.join(', ')}...');
 
-    for (Future<int> result in results)
-      await result;
+    List<int> results = await Future.wait(readers.map((DeviceLogReader reader) async {
+      int result = await reader.logs(clear: clear);
+      if (result != 0)
+        printError('Error listening to $reader logs.');
+      return result;
+    }));
 
-    return 0;
+    // If all readers failed, return an error.
+    return results.every((int result) => result != 0) ? 1 : 0;
   }
 }

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

@@ -134,8 +134,6 @@ Future<int> startApp(
 
     if (traceStartup != null)
       platformArgs['trace-startup'] = traceStartup;
-    if (clearLogs != null)
-      platformArgs['clear-logs'] = clearLogs;
 
     printStatus('Starting ${_getDisplayPath(mainPath)} on ${device.name}...');
 
@@ -145,6 +143,7 @@ Future<int> startApp(
       mainPath: mainPath,
       route: route,
       checked: checked,
+      clearLogs: clearLogs,
       platformArgs: platformArgs
     );
 

packages/flutter_tools/lib/src/device.dart

@@ -34,6 +34,19 @@ class DeviceManager {
 
   Completer _initedCompleter = new Completer();
 
+  /// Return the device with the matching ID; else, complete the Future with
+  /// `null`.
+  ///
+  /// This does a case insentitive compare with `deviceId`.
+  Future<Device> getDeviceById(String deviceId) async {
+    deviceId = deviceId.toLowerCase();
+    List<Device> devices = await getDevices();
+    return devices.firstWhere(
+      (Device device) => device.id.toLowerCase() == deviceId,
+      orElse: () => null
+    );
+  }
+
   Future<List<Device>> getDevices() async {
     await _initedCompleter.future;
 
@@ -78,7 +91,7 @@ abstract class Device {
 
   TargetPlatform get platform;
 
-  Future<int> logs({bool clear: false});
+  DeviceLogReader createLogReader();
 
   /// Start an app package on the current device.
   ///
@@ -90,6 +103,7 @@ abstract class Device {
     String mainPath,
     String route,
     bool checked: true,
+    bool clearLogs: false,
     Map<String, dynamic> platformArgs
   });
 
@@ -99,6 +113,21 @@ abstract class Device {
   String toString() => '$runtimeType $id';
 }
 
+/// Read the log for a particular device. Subclasses must implement `hashCode`
+/// and `operator ==` so that log readers that read from the same location can be
+/// de-duped. For example, two Android devices will both try and log using
+/// `adb logcat`; we don't want to display two identical log streams.
+abstract class DeviceLogReader {
+  String get name;
+
+  Future<int> logs({ bool clear: false });
+
+  int get hashCode;
+  bool operator ==(dynamic other);
+
+  String toString() => name;
+}
+
 // TODO(devoncarew): Unify this with [DeviceManager].
 class DeviceStore {
   final AndroidDevice android;

packages/flutter_tools/lib/src/ios/device_ios.dart

@@ -184,9 +184,10 @@ class IOSDevice extends Device {
     String mainPath,
     String route,
     bool checked: true,
+    bool clearLogs: false,
     Map<String, dynamic> platformArgs
   }) async {
-    // TODO: Use checked, mainPath, route
+    // TODO(chinmaygarde): Use checked, mainPath, route, clearLogs.
     printTrace('Building ${app.name} for $id');
 
     // Step 1: Install the precompiled application if necessary
@@ -231,8 +232,7 @@ class IOSDevice extends Device {
     return false;
   }
 
-  Future<bool> pushFile(
-      ApplicationPackage app, String localFile, String targetFile) async {
+  Future<bool> pushFile(ApplicationPackage app, String localFile, String targetFile) async {
     if (Platform.isMacOS) {
       runSync([
         pusherPath,
@@ -255,14 +255,7 @@ class IOSDevice extends Device {
   @override
   TargetPlatform get platform => TargetPlatform.iOS;
 
-  /// Note that clear is not supported on iOS at this time.
-  Future<int> logs({bool clear: false}) async {
-    if (!isConnected()) {
-      return 2;
-    }
-    return await runCommandAndStreamOutput([loggerPath],
-        prefix: 'iOS: ', filter: new RegExp(r'(FlutterRunner|flutter.runner.Runner)'));
-  }
+  DeviceLogReader createLogReader() => new _IOSDeviceLogReader(this);
 }
 
 class IOSSimulator extends Device {
@@ -335,10 +328,9 @@ class IOSSimulator extends Device {
 
   String _getSimulatorPath() {
     String deviceID = id == defaultDeviceID ? _getRunningSimulatorInfo()?.id : id;
-    String homeDirectory = path.absolute(Platform.environment['HOME']);
     if (deviceID == null)
       return null;
-    return path.join(homeDirectory, 'Library', 'Developer', 'CoreSimulator', 'Devices', deviceID);
+    return path.join(_homeDirectory, 'Library', 'Developer', 'CoreSimulator', 'Devices', deviceID);
   }
 
   String _getSimulatorAppHomeDirectory(ApplicationPackage app) {
@@ -438,11 +430,15 @@ class IOSSimulator extends Device {
     String mainPath,
     String route,
     bool checked: true,
+    bool clearLogs: false,
     Map<String, dynamic> platformArgs
   }) async {
-    // TODO: Use checked, mainPath, route
+    // TODO(chinmaygarde): Use checked, mainPath, route.
     printTrace('Building ${app.name} for $id');
 
+    if (clearLogs)
+      this.clearLogs();
+
     // Step 1: Build the Xcode project
     bool buildResult = await _buildIOSXcodeProject(app, false);
     if (!buildResult) {
@@ -506,34 +502,122 @@ class IOSSimulator extends Device {
     return false;
   }
 
+  String get logFilePath {
+    return path.join(_homeDirectory, 'Library', 'Logs', 'CoreSimulator', id, 'system.log');
+  }
+
   @override
   TargetPlatform get platform => TargetPlatform.iOSSimulator;
 
-  Future<int> logs({bool clear: false}) async {
-    if (!isConnected())
+  DeviceLogReader createLogReader() => new _IOSSimulatorLogReader(this);
+
+  void clearLogs() {
+    File logFile = new File(logFilePath);
+    if (logFile.existsSync())
+      logFile.delete();
+  }
+}
+
+class _IOSDeviceLogReader extends DeviceLogReader {
+  _IOSDeviceLogReader(this.device);
+
+  final IOSDevice device;
+
+  String get name => device.name;
+
+  // TODO(devoncarew): Support [clear].
+  Future<int> logs({ bool clear: false }) async {
+    if (!device.isConnected())
       return 2;
 
-    String homeDirectory = path.absolute(Platform.environment['HOME']);
-    String simulatorDeviceID = _getRunningSimulatorInfo().id;
-    String logFilePath = path.join(
-      homeDirectory, 'Library', 'Logs', 'CoreSimulator', simulatorDeviceID, 'system.log'
+    return await runCommandAndStreamOutput(
+      [device.loggerPath],
+      prefix: '[$name] ',
+      filter: new RegExp(r'(FlutterRunner|flutter.runner.Runner)')
     );
+  }
+
+  int get hashCode => name.hashCode;
+
+  bool operator ==(dynamic other) {
+    if (identical(this, other))
+      return true;
+    if (other is! _IOSDeviceLogReader)
+      return false;
+    return other.name == name;
+  }
+}
+
+class _IOSSimulatorLogReader extends DeviceLogReader {
+  _IOSSimulatorLogReader(this.device);
+
+  final IOSSimulator device;
+
+  String get name => device.name;
+
+  Future<int> logs({bool clear: false}) async {
+    if (!device.isConnected())
+      return 2;
 
     if (clear)
-      runSync(['rm', logFilePath]);
-
-    // TODO(devoncarew): The log message prefix could be shortened or removed.
-    // Jan 29 01:31:44 devoncarew-macbookpro3 SpringBoard[96648]:
-    // TODO(devoncarew): This truncates multi-line messages like:
-    // Jan 29 01:31:43 devoncarew-macbookpro3 CoreSimulatorBridge[96656]: Requesting... {
-    //     environment =     {
-    //     };
-    //   }
-    return await runCommandAndStreamOutput(
-      ['tail', '-f', logFilePath],
-      prefix: 'iOS: ',
-      filter: new RegExp(r'(FlutterRunner|flutter.runner.Runner)')
+      device.clearLogs();
+
+    // Match the log prefix (in order to shorten it):
+    //   'Jan 29 01:31:44 devoncarew-macbookpro3 SpringBoard[96648]: ...'
+    RegExp mapRegex = new RegExp(r'\S+ +\S+ +\S+ \S+ (.+)\[\d+\]\)?: (.*)$');
+    // Jan 31 19:23:28 --- last message repeated 1 time ---
+    RegExp lastMessageRegex = new RegExp(r'\S+ +\S+ +\S+ (--- .* ---)$');
+
+    // This filter matches many Flutter lines in the log:
+    // new RegExp(r'(FlutterRunner|flutter.runner.Runner|$id)'), but it misses
+    // a fair number, including ones that would be useful in diagnosing crashes.
+    // For now, we're not filtering the log file (but do clear it with each run).
+
+    Future<int> result = runCommandAndStreamOutput(
+      <String>['tail', '-n', '+0', '-F', device.logFilePath],
+      prefix: '[$name] ',
+      mapFunction: (String string) {
+        Match match = mapRegex.matchAsPrefix(string);
+        if (match != null) {
+          // Filter out some messages that clearly aren't related to Flutter.
+          if (string.contains(': could not find icon for representation -> com.apple.'))
+            return null;
+          String category = match.group(1);
+          String content = match.group(2);
+          if (category == 'Game Center' || category == 'itunesstored' || category == 'nanoregistrylaunchd')
+            return null;
+          return '$category: $content';
+        }
+        match = lastMessageRegex.matchAsPrefix(string);
+        if (match != null)
+          return match.group(1);
+        return string;
+      }
     );
+
+    // Track system.log crashes.
+    // ReportCrash[37965]: Saved crash report for FlutterRunner[37941]...
+    runCommandAndStreamOutput(
+      <String>['tail', '-F', '/private/var/log/system.log'],
+      prefix: '[$name] ',
+      filter: new RegExp(r' FlutterRunner\[\d+\] '),
+      mapFunction: (String string) {
+        Match match = mapRegex.matchAsPrefix(string);
+        return match == null ? string : '${match.group(1)}: ${match.group(2)}';
+      }
+    );
+
+    return result;
+  }
+
+  int get hashCode => device.logFilePath.hashCode;
+
+  bool operator ==(dynamic other) {
+    if (identical(this, other))
+      return true;
+    if (other is! _IOSSimulatorLogReader)
+      return false;
+    return other.device.logFilePath == device.logFilePath;
   }
 }
 
@@ -547,6 +631,8 @@ class _IOSSimulatorInfo {
 final RegExp _xcodeVersionRegExp = new RegExp(r'Xcode (\d+)\..*');
 final String _xcodeRequirement = 'Xcode 7.0 or greater is required to develop for iOS.';
 
+String get _homeDirectory => path.absolute(Platform.environment['HOME']);
+
 bool _checkXcodeVersion() {
   if (!Platform.isMacOS)
     return false;
@@ -573,15 +659,18 @@ Future<bool> _buildIOSXcodeProject(ApplicationPackage app, bool isDevice) async
   if (!_checkXcodeVersion())
     return false;
 
-  List<String> command = [
-    'xcrun', 'xcodebuild', '-target', 'Runner', '-configuration', 'Release'
+  List<String> commands = [
+    '/usr/bin/env', 'xcrun', 'xcodebuild', '-target', 'Runner', '-configuration', 'Release'
   ];
 
   if (!isDevice) {
-    command.addAll(['-sdk', 'iphonesimulator']);
+    commands.addAll(['-sdk', 'iphonesimulator']);
   }
 
-  ProcessResult result = Process.runSync('/usr/bin/env', command,
-      workingDirectory: app.localPath);
-  return result.exitCode == 0;
+  try {
+    runCheckedSync(commands, workingDirectory: app.localPath);
+    return true;
+  } catch (error) {
+    return false;
+  }
 }

packages/flutter_tools/lib/src/runner/flutter_command_runner.dart

@@ -142,11 +142,16 @@ class FlutterCommandRunner extends CommandRunner {
   String get _defaultFlutterRoot {
     if (Platform.environment.containsKey(kFlutterRootEnvironmentVariableName))
       return Platform.environment[kFlutterRootEnvironmentVariableName];
-    String script = Platform.script.toFilePath();
-    if (path.basename(script) == kSnapshotFileName)
-      return path.dirname(path.dirname(path.dirname(script)));
-    if (path.basename(script) == kFlutterToolsScriptFileName)
-      return path.dirname(path.dirname(path.dirname(path.dirname(script))));
+
+    try {
+      String script = Platform.script.toFilePath();
+      if (path.basename(script) == kSnapshotFileName)
+        return path.dirname(path.dirname(path.dirname(script)));
+      if (path.basename(script) == kFlutterToolsScriptFileName)
+        return path.dirname(path.dirname(path.dirname(path.dirname(script))));
+    } catch (error) {
+      printTrace('Unable to locate fluter root: $error');
+    }
     return '.';
   }
 

packages/flutter_tools/test/logs_test.dart

@@ -4,7 +4,7 @@
 
 import 'package:args/command_runner.dart';
 import 'package:flutter_tools/src/commands/logs.dart';
-import 'package:mockito/mockito.dart';
+import 'package:flutter_tools/src/runner/flutter_command_runner.dart';
 import 'package:test/test.dart';
 
 import 'src/mocks.dart';
@@ -13,18 +13,13 @@ main() => defineTests();
 
 defineTests() {
   group('logs', () {
-    test('returns 0 when no device is connected', () {
+    test('fail with a bad device id', () {
       LogsCommand command = new LogsCommand();
       applyMocksToCommand(command);
-      MockDeviceStore mockDevices = command.devices;
-
-      when(mockDevices.android.isConnected()).thenReturn(false);
-      when(mockDevices.iOS.isConnected()).thenReturn(false);
-      when(mockDevices.iOSSimulator.isConnected()).thenReturn(false);
-
-      CommandRunner runner = new CommandRunner('test_flutter', '')
-        ..addCommand(command);
-      runner.run(['logs']).then((int code) => expect(code, equals(0)));
+      CommandRunner runner = new FlutterCommandRunner()..addCommand(command);
+      runner.run(<String>['-d', 'abc123', 'logs']).then((int code) {
+        expect(code, equals(1));
+      });
     });
   });
 }