Add --bug-report flag (#8435)

This adds support for a `--bug-report` flag, which is a recording
that:
  - includes the arguments that were passed to the command runner
  - is zipped up for easy attachment in Guthub issues

packages/flutter_driver/pubspec.yaml

@@ -8,7 +8,7 @@ environment:
   sdk: '>=1.19.0 <2.0.0'
 
 dependencies:
-  file: 2.1.0
+  file: 2.3.0
   json_rpc_2: '^2.0.0'
   matcher: '>=0.12.0 <1.0.0'
   path: '^1.4.0'

packages/flutter_tools/lib/executable.dart

@@ -126,7 +126,7 @@ Future<Null> main(List<String> args) async {
 
     return Chain.capture<Future<Null>>(() async {
       await runner.run(args);
-      _exit(0);
+      await _exit(0);
     }, onError: (dynamic error, Chain chain) {
       if (error is UsageException) {
         stderr.writeln(error.message);
@@ -249,9 +249,14 @@ Future<Null> _exit(int code) async {
   // Run shutdown hooks before flushing logs
   await runShutdownHooks();
 
+  Completer<Null> completer = new Completer<Null>();
+
   // Give the task / timer queue one cycle through before we hard exit.
   Timer.run(() {
     printTrace('exiting with code $code');
     exit(code);
+    completer.complete();
   });
+
+  await completer.future;
 }

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

@@ -34,10 +34,14 @@ FileSystem get fs => context == null ? _kLocalFs : context[FileSystem];
 /// It is permissible for [location] to represent an existing non-empty
 /// directory as long as there is no collision with the `"file"` subdirectory.
 void enableRecordingFileSystem(String location) {
+  FileSystem originalFileSystem = fs;
   Directory dir = getRecordingSink(location, _kRecordingType);
   RecordingFileSystem fileSystem = new RecordingFileSystem(
       delegate: _kLocalFs, destination: dir);
-  addShutdownHook(() => fileSystem.recording.flush());
+  addShutdownHook(() async {
+    await fileSystem.recording.flush();
+    context.setVariable(FileSystem, originalFileSystem);
+  }, ShutdownStage.SERIALIZE_RECORDING);
   context.setVariable(FileSystem, fileSystem);
 }
 

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

@@ -37,6 +37,8 @@ abstract class OperatingSystemUtils {
   /// Return the File representing a new pipe.
   File makePipe(String path);
 
+  void zip(Directory data, File zipFile);
+
   void unzip(File file, Directory targetDirectory);
 }
 
@@ -59,6 +61,11 @@ class _PosixUtils extends OperatingSystemUtils {
     return fs.file(path);
   }
 
+  @override
+  void zip(Directory data, File zipFile) {
+    runSync(<String>['zip', '-r', '-q', zipFile.path, '.'], workingDirectory: data.path);
+  }
+
   // unzip -o -q zipfile -d dest
   @override
   void unzip(File file, Directory targetDirectory) {
@@ -89,6 +96,21 @@ class _WindowsUtils extends OperatingSystemUtils {
     return fs.file(result.stdout.trim().split('\n').first.trim());
   }
 
+  @override
+  void zip(Directory data, File zipFile) {
+    Archive archive = new Archive();
+    for (FileSystemEntity entity in data.listSync(recursive: true)) {
+      if (entity is! File) {
+        continue;
+      }
+      File file = entity;
+      String path = file.fileSystem.path.relative(file.path, from: data.path);
+      List<int> bytes = file.readAsBytesSync();
+      archive.addFile(new ArchiveFile(path, bytes.length, bytes));
+    }
+    zipFile.writeAsBytesSync(new ZipEncoder().encode(archive), flush: true);
+  }
+
   @override
   void unzip(File file, Directory targetDirectory) {
     Archive archive = new ZipDecoder().decodeBytes(file.readAsBytesSync());

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

@@ -11,26 +11,70 @@ import 'io.dart';
 import 'process_manager.dart';
 
 typedef String StringConverter(String string);
+
+/// A function that will be run before the VM exits.
 typedef Future<dynamic> ShutdownHook();
 
 // TODO(ianh): We have way too many ways to run subprocesses in this project.
 
-List<ShutdownHook> _shutdownHooks = <ShutdownHook>[];
+/// The stage in which a [ShutdownHook] will be run. All shutdown hooks within
+/// a given stage will be started in parallel and will be guaranteed to run to
+/// completion before shutdown hooks in the next stage are started.
+class ShutdownStage implements Comparable<ShutdownStage> {
+  const ShutdownStage._(this._priority);
+
+  /// The stage priority. Smaller values will be run before larger values.
+  final int _priority;
+
+  /// The stage during which the invocation recording (if one exists) will be
+  /// serialized to disk. Invocations performed after this stage will not be
+  /// recorded.
+  static const ShutdownStage SERIALIZE_RECORDING = const ShutdownStage._(1);
+
+  /// The stage during which a serialized recording will be refined (e.g.
+  /// cleansed for tests, zipped up for bug reporting purposes, etc.).
+  static const ShutdownStage POST_PROCESS_RECORDING = const ShutdownStage._(2);
+
+  /// The stage during which temporary files and directories will be deleted.
+  static const ShutdownStage CLEANUP = const ShutdownStage._(3);
+
+  @override
+  int compareTo(ShutdownStage other) => _priority.compareTo(other._priority);
+}
+
+Map<ShutdownStage, List<ShutdownHook>> _shutdownHooks = <ShutdownStage, List<ShutdownHook>>{};
 bool _shutdownHooksRunning = false;
-void addShutdownHook(ShutdownHook shutdownHook) {
+
+/// Registers a [ShutdownHook] to be executed before the VM exits.
+///
+/// If [stage] is specified, the shutdown hook will be run during the specified
+/// stage. By default, the shutdown hook will be run during the
+/// [ShutdownStage.CLEANUP] stage.
+void addShutdownHook(
+  ShutdownHook shutdownHook, [
+  ShutdownStage stage = ShutdownStage.CLEANUP,
+]) {
   assert(!_shutdownHooksRunning);
-  _shutdownHooks.add(shutdownHook);
+  _shutdownHooks.putIfAbsent(stage, () => <ShutdownHook>[]).add(shutdownHook);
 }
 
+/// Runs all registered shutdown hooks and returns a future that completes when
+/// all such hooks have finished.
+///
+/// Shutdown hooks will be run in groups by their [ShutdownStage]. All shutdown
+/// hooks within a given stage will be started in parallel and will be
+/// guaranteed to run to completion before shutdown hooks in the next stage are
+/// started.
 Future<Null> runShutdownHooks() async {
-  List<ShutdownHook> hooks = new List<ShutdownHook>.from(_shutdownHooks);
-  _shutdownHooks.clear();
   _shutdownHooksRunning = true;
   try {
-    List<Future<dynamic>> futures = <Future<dynamic>>[];
-    for (ShutdownHook shutdownHook in hooks)
-      futures.add(shutdownHook());
-    await Future.wait<dynamic>(futures);
+    for (ShutdownStage stage in _shutdownHooks.keys.toList()..sort()) {
+      List<ShutdownHook> hooks = _shutdownHooks.remove(stage);
+      List<Future<dynamic>> futures = <Future<dynamic>>[];
+      for (ShutdownHook shutdownHook in hooks)
+        futures.add(shutdownHook());
+      await Future.wait<dynamic>(futures);
+    }
   } finally {
     _shutdownHooksRunning = false;
   }

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

@@ -28,10 +28,14 @@ ProcessManager get processManager => context[ProcessManager];
 /// directory as long as there is no collision with the `"process"`
 /// subdirectory.
 void enableRecordingProcessManager(String location) {
+  ProcessManager originalProcessManager = processManager;
   Directory dir = getRecordingSink(location, _kRecordingType);
   ProcessManager delegate = const LocalProcessManager();
   RecordingProcessManager manager = new RecordingProcessManager(delegate, dir);
-  addShutdownHook(() => manager.flush(finishRunningProcesses: true));
+  addShutdownHook(() async {
+    await manager.flush(finishRunningProcesses: true);
+    context.setVariable(ProcessManager, originalProcessManager);
+  }, ShutdownStage.SERIALIZE_RECORDING);
   context.setVariable(ProcessManager, manager);
 }
 

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

@@ -13,9 +13,11 @@ import '../base/common.dart';
 import '../base/context.dart';
 import '../base/file_system.dart';
 import '../base/logger.dart';
+import '../base/os.dart';
 import '../base/platform.dart';
 import '../base/process.dart';
 import '../base/process_manager.dart';
+import '../base/utils.dart';
 import '../cache.dart';
 import '../dart/package_map.dart';
 import '../device.dart';
@@ -66,6 +68,9 @@ class FlutterCommandRunner extends CommandRunner<Null> {
         negatable: false,
         hide: !verboseHelp,
         help: 'Suppress analytics reporting when this command runs.');
+    argParser.addFlag('bug-report',
+        negatable: false,
+        help: 'Captures a bug report file to submit to the Flutter team.');
 
     String packagesHelp;
     if (fs.isFileSync(kPackagesFileName))
@@ -97,14 +102,14 @@ class FlutterCommandRunner extends CommandRunner<Null> {
             'Use this to select a specific version of the engine if you have built multiple engine targets.\n'
             'This path is relative to --local-engine-src-path/out.');
     argParser.addOption('record-to',
-        hide: !verboseHelp,
+        hide: true,
         help:
             'Enables recording of process invocations (including stdout and stderr of all such invocations),\n'
             'and file system access (reads and writes).\n'
             'Serializes that recording to a directory with the path specified in this flag. If the\n'
             'directory does not already exist, it will be created.');
     argParser.addOption('replay-from',
-        hide: !verboseHelp,
+        hide: true,
         help:
             'Enables mocking of process invocations by replaying their stdout, stderr, and exit code from\n'
             'the specified recording (obtained via --record-to). The path specified in this flag must refer\n'
@@ -159,12 +164,39 @@ class FlutterCommandRunner extends CommandRunner<Null> {
       context.setVariable(Logger, new VerboseLogger());
     }
 
-    if (globalResults['record-to'] != null &&
-        globalResults['replay-from'] != null)
-      throwToolExit('--record-to and --replay-from cannot be used together.');
+    String recordTo = globalResults['record-to'];
+    String replayFrom = globalResults['replay-from'];
+
+    if (globalResults['bug-report']) {
+      // --bug-report implies --record-to=<tmp_path>
+      Directory tmp = await const LocalFileSystem()
+          .systemTempDirectory
+          .createTemp('flutter_tools_');
+      recordTo = tmp.path;
+
+      // Record the arguments that were used to invoke this runner.
+      File manifest = tmp.childFile('MANIFEST.txt');
+      StringBuffer buffer = new StringBuffer()
+        ..writeln('# arguments')
+        ..writeln(globalResults.arguments)
+        ..writeln()
+        ..writeln('# rest')
+        ..writeln(globalResults.rest);
+      await manifest.writeAsString(buffer.toString(), flush: true);
+
+      // ZIP the recording up once the recording has been serialized.
+      addShutdownHook(() async {
+        File zipFile = getUniqueFile(fs.currentDirectory, 'bugreport', 'zip');
+        os.zip(tmp, zipFile);
+        printStatus('Bug report written to ${zipFile.basename}');
+      }, ShutdownStage.POST_PROCESS_RECORDING);
+      addShutdownHook(() => tmp.delete(recursive: true), ShutdownStage.CLEANUP);
+    }
+
+    assert(recordTo == null || replayFrom == null);
 
-    if (globalResults['record-to'] != null) {
-      String recordTo = globalResults['record-to'].trim();
+    if (recordTo != null) {
+      recordTo = recordTo.trim();
       if (recordTo.isEmpty)
         throwToolExit('record-to location not specified');
       enableRecordingProcessManager(recordTo);
@@ -173,8 +205,8 @@ class FlutterCommandRunner extends CommandRunner<Null> {
       VMService.enableRecordingConnection(recordTo);
     }
 
-    if (globalResults['replay-from'] != null) {
-      String replayFrom = globalResults['replay-from'].trim();
+    if (replayFrom != null) {
+      replayFrom = replayFrom.trim();
       if (replayFrom.isEmpty)
         throwToolExit('replay-from location not specified');
       await enableReplayProcessManager(replayFrom);

packages/flutter_tools/lib/src/vmservice_record_replay.dart

@@ -32,24 +32,13 @@ class RecordingVMServiceChannel extends DelegatingStreamChannel<String> {
     addShutdownHook(() async {
       // Sort the messages such that they are ordered
       // `[request1, response1, request2, response2, ...]`. This serves no
-      // other purpose than to make the serialized format more human-readable.
-      _messages.sort((_Message message1, _Message message2) {
-        int id1 = message1.id;
-        int id2 = message2.id;
-        int result = id1.compareTo(id2);
-        if (result != 0) {
-          return result;
-        } else if (message1.type == _kRequest) {
-          return -1;
-        } else {
-          return 1;
-        }
-      });
+      // purpose other than to make the serialized format more human-readable.
+      _messages.sort();
 
       File file = _getManifest(location);
       String json = new JsonEncoder.withIndent('  ').convert(_messages);
       await file.writeAsString(json, flush: true);
-    });
+    }, ShutdownStage.SERIALIZE_RECORDING);
   }
 
   @override
@@ -70,7 +59,7 @@ class RecordingVMServiceChannel extends DelegatingStreamChannel<String> {
 }
 
 /// Base class for request and response JSON-rpc messages.
-abstract class _Message {
+abstract class _Message implements Comparable<_Message> {
   final String type;
   final Map<String, dynamic> data;
 
@@ -91,6 +80,18 @@ abstract class _Message {
       _kData: data,
     };
   }
+
+  @override
+  int compareTo(_Message other) {
+    int result = id.compareTo(other.id);
+    if (result != 0) {
+      return result;
+    } else if (type == _kRequest) {
+      return -1;
+    } else {
+      return 1;
+    }
+  }
 }
 
 /// A VM service JSON-rpc request (sent to the VM).

packages/flutter_tools/pubspec.yaml

@@ -12,7 +12,7 @@ dependencies:
   args: ^0.13.4
   coverage: ^0.8.0
   crypto: '>=1.1.1 <3.0.0'
-  file: 2.1.0
+  file: 2.3.0
   http: ^0.11.3
   intl: '>=0.14.0 <0.15.0'
   json_rpc_2: ^2.0.0

packages/flutter_tools/test/all.dart

@@ -19,6 +19,7 @@ import 'application_package_test.dart' as application_package_test;
 import 'artifacts_test.dart' as artifacts_test;
 import 'asset_bundle_test.dart' as asset_bundle_test;
 import 'base_utils_test.dart' as base_utils_test;
+import 'bug_report_test.dart' as bug_report_test;
 import 'channel_test.dart' as channel_test;
 import 'config_test.dart' as config_test;
 import 'context_test.dart' as context_test;
@@ -45,6 +46,7 @@ import 'test_test.dart' as test_test;
 import 'trace_test.dart' as trace_test;
 import 'upgrade_test.dart' as upgrade_test;
 import 'utils_test.dart' as utils_test;
+import 'src/base/process_test.dart' as process_test;
 
 void main() {
   Cache.disableLocking();
@@ -58,6 +60,7 @@ void main() {
   artifacts_test.main();
   asset_bundle_test.main();
   base_utils_test.main();
+  bug_report_test.main();
   channel_test.main();
   config_test.main();
   context_test.main();
@@ -77,6 +80,7 @@ void main() {
   logs_test.main();
   os_utils_test.main();
   packages_test.main();
+  process_test.main();
   protocol_discovery_test.main();
   run_test.main();
   stop_test.main();

packages/flutter_tools/test/bug_report_test.dart

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:file/testing.dart';
+import 'package:flutter_tools/executable.dart' as tools;
+import 'package:flutter_tools/src/cache.dart';
+import 'package:flutter_tools/src/base/io.dart';
+import 'package:flutter_tools/src/base/os.dart';
+import 'package:mockito/mockito.dart';
+import 'package:test/test.dart';
+
+import 'src/context.dart';
+
+void main() {
+  Cache.disableLocking();
+
+  int exitCode;
+  setExitFunctionForTests((int code) {
+    exitCode = code;
+  });
+
+  group('--bug-report', () {
+    testUsingContext('generates valid zip file', () async {
+      await tools.main(<String>['devices', '--bug-report']);
+      expect(exitCode, 0);
+      verify(os.zip(any, argThat(hasPath(matches(r'bugreport_01\.zip')))));
+    });
+  });
+}

packages/flutter_tools/test/src/base/process_test.dart

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:flutter_tools/src/base/process.dart';
+import 'package:test/test.dart';
+
+void main() {
+  group('shutdownHooks', () {
+    test('runInExpectedOrder', () async {
+      int i = 1;
+      int serializeRecording1;
+      int serializeRecording2;
+      int postProcessRecording;
+      int cleanup;
+
+      addShutdownHook(() async {
+        serializeRecording1 = i++;
+      }, ShutdownStage.SERIALIZE_RECORDING);
+
+      addShutdownHook(() async {
+        cleanup = i++;
+      }, ShutdownStage.CLEANUP);
+
+      addShutdownHook(() async {
+        postProcessRecording = i++;
+      }, ShutdownStage.POST_PROCESS_RECORDING);
+
+      addShutdownHook(() async {
+        serializeRecording2 = i++;
+      }, ShutdownStage.SERIALIZE_RECORDING);
+
+      await runShutdownHooks();
+
+      expect(serializeRecording1, lessThanOrEqualTo(2));
+      expect(serializeRecording2, lessThanOrEqualTo(2));
+      expect(postProcessRecording, 3);
+      expect(cleanup, 4);
+    });
+  });
+}