Small improvements to et lint command (#51372)

- Default to dumping out lint logs (can be disabled with `--quiet`
flag).
- Add Logger.fatal which logs an error and throws a FatalError which is
caught in main.
- Simplify `findDartBinDirectory` implementation.
- Make JSON serialized process artifacts more human readable.

Author: johnmccutchan

tools/engine_tool/lib/main.dart

@@ -65,6 +65,13 @@ void main(List<String> args) async {
     configs: configs,
   );
 
-  io.exitCode = await runner.run(args);
+  try {
+    io.exitCode = await runner.run(args);
+  } on FatalError catch (e, st) {
+    environment.logger.error('FatalError caught in main. Please file a bug\n'
+        'error: $e\n'
+        'stack: $st');
+    io.exitCode = 1;
+  }
   return;
 }

tools/engine_tool/lib/src/commands/lint_command.dart

@@ -63,6 +63,11 @@ final class LintCommand extends CommandBase {
         Linter.python,
         environment.engine.flutterDir,
         <String>[p.join(engineFlutterPath, 'ci', 'pylint.sh')]);
+    argParser.addFlag(
+      quietFlag,
+      abbr: 'q',
+      help: 'Prints minimal output',
+    );
   }
 
   final Map<Linter, _LinterDescription> _linters =
@@ -79,7 +84,7 @@ final class LintCommand extends CommandBase {
     // TODO(loic-sharma): Relax this restriction.
     if (environment.platform.isWindows) {
       environment.logger
-          .error('lint command is not supported on Windows (for now).');
+          .fatal('lint command is not supported on Windows (for now).');
       return 1;
     }
     final WorkerPool wp =
@@ -92,16 +97,17 @@ final class LintCommand extends CommandBase {
     }
     final bool r = await wp.run(tasks);
 
-    final bool verbose = globalResults![verboseFlag] as bool;
-    if (verbose) {
+    final bool quiet = argResults![quietFlag] as bool;
+    if (!quiet) {
       environment.logger.status('\nDumping failure logs\n');
       for (final ProcessTask pt in tasks) {
         final ProcessArtifacts pa = pt.processArtifacts;
         if (pa.exitCode == 0) {
           continue;
         }
-        environment.logger.status(pa.stdout);
-        environment.logger.status(pa.stderr);
+        environment.logger.status('Linter ${pt.name} found issues:');
+        environment.logger.status('${pa.stdout}\n');
+        environment.logger.status('${pa.stderr}\n');
       }
     }
     return r ? 0 : 1;

tools/engine_tool/lib/src/dart_utils.dart

@@ -2,52 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'dart:ffi' as ffi show Abi;
-
 import 'package:path/path.dart' as p;
 import 'environment.dart';
 
-String _getAbiSubdirectory(ffi.Abi abi) {
-  switch (abi) {
-    case ffi.Abi.macosArm64:
-      return 'macos-arm64';
-    case ffi.Abi.macosX64:
-      return 'macos-x64';
-    case ffi.Abi.windowsArm64:
-      return 'windows-arm64';
-    case ffi.Abi.windowsX64:
-      return 'windows-x64';
-    case ffi.Abi.linuxArm:
-      return 'linux-arm';
-    case ffi.Abi.linuxArm64:
-      return 'linux-arm64';
-    case ffi.Abi.linuxIA32:
-      return 'linux-x86';
-    case ffi.Abi.linuxX64:
-      return 'linux-x64';
-    case ffi.Abi.androidArm:
-    case ffi.Abi.androidArm64:
-    case ffi.Abi.androidIA32:
-    case ffi.Abi.androidX64:
-    case ffi.Abi.androidRiscv64:
-    case ffi.Abi.fuchsiaArm64:
-    case ffi.Abi.fuchsiaX64:
-    case ffi.Abi.fuchsiaRiscv64:
-    case ffi.Abi.iosArm:
-    case ffi.Abi.iosArm64:
-    case ffi.Abi.iosX64:
-    case ffi.Abi.linuxRiscv32:
-    case ffi.Abi.linuxRiscv64:
-    case ffi.Abi.windowsIA32:
-    default:
-      throw UnsupportedError('Unsupported Abi $abi');
-  }
-}
-
 /// Returns a dart-sdk/bin directory path that is compatible with the host.
 String findDartBinDirectory(Environment env) {
-  return p.join(env.engine.flutterDir.path, 'prebuilts',
-      _getAbiSubdirectory(env.abi), 'dart-sdk', 'bin');
+  return p.dirname(env.platform.resolvedExecutable);
 }
 
 /// Returns a dart-sdk/bin/dart file pthat that is executable on the host.

tools/engine_tool/lib/src/json_utils.dart

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:convert';
+
 void _appendTypeError(
   Map<String, Object?> map,
   String field,
@@ -77,3 +79,8 @@ int? intOfJson(
   }
   return map[field]! as int;
 }
+
+const JsonEncoder _jsonEncoder = JsonEncoder.withIndent('    ');
+
+/// Same as [jsonEncode] but is formatted to be human readable.
+String jsonEncodePretty(Object? object) => _jsonEncoder.convert(object);

tools/engine_tool/lib/src/logger.dart

@@ -3,10 +3,7 @@
 // found in the LICENSE file.
 
 import 'dart:async' show Timer, runZoned;
-import 'dart:io' as io show
-  IOSink,
-  stderr,
-  stdout;
+import 'dart:io' as io show IOSink, stderr, stdout;
 
 import 'package:logging/logging.dart' as log;
 import 'package:meta/meta.dart';
@@ -29,7 +26,9 @@ import 'package:meta/meta.dart';
 /// which can be inspected by unit tetss.
 class Logger {
   /// Constructs a logger for use in the tool.
-  Logger() : _logger = log.Logger.detached('et'), _test = false {
+  Logger()
+      : _logger = log.Logger.detached('et'),
+        _test = false {
     _logger.level = statusLevel;
     _logger.onRecord.listen(_handler);
     _setupIoSink(io.stderr);
@@ -38,7 +37,9 @@ class Logger {
 
   /// A logger for tests.
   @visibleForTesting
-  Logger.test() : _logger = log.Logger.detached('et'), _test = true {
+  Logger.test()
+      : _logger = log.Logger.detached('et'),
+        _test = true {
     _logger.level = statusLevel;
     _logger.onRecord.listen((log.LogRecord r) => _testLogs.add(r));
   }
@@ -57,9 +58,8 @@ class Logger {
 
   static void _handler(log.LogRecord r) {
     final io.IOSink sink = r.level >= warningLevel ? io.stderr : io.stdout;
-    final String prefix = r.level >= warningLevel
-      ? '[${r.time}] ${r.level}: '
-      : '';
+    final String prefix =
+        r.level >= warningLevel ? '[${r.time}] ${r.level}: ' : '';
     _ioSinkWrite(sink, '$prefix${r.message}');
   }
 
@@ -77,7 +77,7 @@ class Logger {
     runZoned<void>(() {
       try {
         sink.write(message);
-      } catch (_) { // ignore: avoid_catches_without_on_clauses
+      } catch (_) {
         _stdioDone = true;
       }
     }, onError: (Object e, StackTrace s) {
@@ -87,8 +87,12 @@ class Logger {
 
   static void _setupIoSink(io.IOSink sink) {
     sink.done.then(
-      (void _) { _stdioDone = true; },
-      onError: (Object err, StackTrace st) { _stdioDone = true; },
+      (void _) {
+        _stdioDone = true;
+      },
+      onError: (Object err, StackTrace st) {
+        _stdioDone = true;
+      },
     );
   }
 
@@ -106,6 +110,19 @@ class Logger {
     _logger.level = l;
   }
 
+  /// Record a log message level [Logger.error] and throw a FatalError.
+  /// This should only be called when the program has entered an impossible
+  /// to recover from state or when something isn't implemented yet.
+  void fatal(
+    Object? message, {
+    int indent = 0,
+    bool newline = true,
+    bool fit = false,
+  }) {
+    _emitLog(errorLevel, message, indent, newline, fit);
+    throw FatalError(_formatMessage(message, indent, newline, fit));
+  }
+
   /// Record a log message at level [Logger.error].
   void error(
     Object? message, {
@@ -165,9 +182,10 @@ class Logger {
       onFinish?.call();
       _status = null;
     }
+
     _status = io.stdout.hasTerminal && !_test
-      ? FlutterSpinner(onFinish: finishCallback)
-      : Spinner(onFinish: finishCallback);
+        ? FlutterSpinner(onFinish: finishCallback)
+        : Spinner(onFinish: finishCallback);
     _status!.start();
     return _status!;
   }
@@ -184,17 +202,22 @@ class Logger {
     _ioSinkWrite(io.stdout, '$backspaces$spaces$backspaces');
   }
 
+  String _formatMessage(Object? message, int indent, bool newline, bool fit) {
+    String m = '${' ' * indent}$message${newline ? '\n' : ''}';
+    if (fit && io.stdout.hasTerminal) {
+      m = fitToWidth(m, io.stdout.terminalColumns);
+    }
+    return m;
+  }
+
   void _emitLog(
     log.Level level,
     Object? message,
     int indent,
     bool newline,
     bool fit,
   ) {
-    String m = '${' ' * indent}$message${newline ? '\n' : ''}';
-    if (fit && io.stdout.hasTerminal) {
-      m = fitToWidth(m, io.stdout.terminalColumns);
-    }
+    final String m = _formatMessage(message, indent, newline, fit);
     _status?.pause();
     _logger.log(level, m);
     _status?.resume();
@@ -244,7 +267,6 @@ class Logger {
   List<log.LogRecord> get testLogs => _testLogs;
 }
 
-
 /// A base class for progress spinners, and a no-op implementation that prints
 /// nothing.
 class Spinner {
@@ -280,12 +302,10 @@ class FlutterSpinner extends Spinner {
     super.onFinish,
   });
 
-  @visibleForTesting
   /// The frames of the animation.
   static const String frames = '⢸⡯⠭⠅⢸⣇⣀⡀⢸⣇⣸⡇⠈⢹⡏⠁⠈⢹⡏⠁⢸⣯⣭⡅⢸⡯⢕⡂⠀⠀';
 
-  static final List<String> _flutterAnimation = frames
-      .runes
+  static final List<String> _flutterAnimation = frames.runes
       .map<String>((int scalar) => String.fromCharCode(scalar))
       .toList();
 
@@ -338,3 +358,14 @@ class FlutterSpinner extends Spinner {
     }
   }
 }
+
+/// FatalErrors are thrown when a fatal error has occurred.
+class FatalError extends Error {
+  /// Constructs a FatalError with a message.
+  FatalError(this._message);
+
+  final String _message;
+
+  @override
+  String toString() => _message;
+}

tools/engine_tool/lib/src/proc_utils.dart

@@ -50,7 +50,7 @@ final class ProcessArtifacts {
     data['stderr'] = stderr;
     data['cwd'] = cwd.absolute.path;
     data['commandLine'] = commandLine;
-    file.writeAsStringSync(jsonEncode(data));
+    file.writeAsStringSync(jsonEncodePretty(data));
   }
 
   /// Creates a temporary file and saves the artifacts into it.

tools/engine_tool/test/build_command_test.dart

@@ -60,7 +60,9 @@ void main() {
       Environment(
         abi: ffi.Abi.linuxX64,
         engine: engine,
-        platform: FakePlatform(operatingSystem: Platform.linux),
+        platform: FakePlatform(
+            operatingSystem: Platform.linux,
+            resolvedExecutable: io.Platform.resolvedExecutable),
         processRunner: ProcessRunner(
           processManager: FakeProcessManager(onStart: (List<String> command) {
             runHistory.add(command);

tools/engine_tool/test/fetch_command_test.dart

@@ -31,7 +31,9 @@ void main() {
       Environment(
         abi: ffi.Abi.linuxX64,
         engine: engine,
-        platform: FakePlatform(operatingSystem: Platform.linux),
+        platform: FakePlatform(
+            operatingSystem: Platform.linux,
+            resolvedExecutable: io.Platform.resolvedExecutable),
         processRunner: ProcessRunner(
           processManager: FakeProcessManager(onStart: (List<String> command) {
             runHistory.add(command);
@@ -54,8 +56,7 @@ void main() {
       environment: env,
       configs: configs,
     );
-    final int result =
-        await runner.run(<String>['fetch']);
+    final int result = await runner.run(<String>['fetch']);
     expect(result, equals(0));
     expect(runHistory.length, greaterThanOrEqualTo(1));
     expect(
@@ -71,8 +72,7 @@ void main() {
       environment: env,
       configs: configs,
     );
-    final int result =
-        await runner.run(<String>['sync']);
+    final int result = await runner.run(<String>['sync']);
     expect(result, equals(0));
     expect(runHistory.length, greaterThanOrEqualTo(1));
     expect(

tools/engine_tool/test/lint_command_test.dart

@@ -58,7 +58,9 @@ void main() {
       Environment(
         abi: ffi.Abi.macosArm64,
         engine: engine,
-        platform: FakePlatform(operatingSystem: Platform.macOS),
+        platform: FakePlatform(
+            operatingSystem: Platform.macOS,
+            resolvedExecutable: io.Platform.resolvedExecutable),
         processRunner: ProcessRunner(
             processManager: FakeProcessManager(onStart: (List<String> command) {
           runHistory.add(command);

tools/engine_tool/test/logger_test.dart

@@ -79,6 +79,19 @@ void main() {
     expect(stringsFromLogs(logger.testLogs), equals(<String>['info']));
   });
 
+  test('fatal throws exception', () {
+    final Logger logger = Logger.test();
+    logger.level = Logger.infoLevel;
+    bool caught = false;
+    try {
+      logger.fatal('test', newline: false);
+    } on FatalError catch (_) {
+      caught = true;
+    }
+    expect(caught, equals(true));
+    expect(stringsFromLogs(logger.testLogs), equals(<String>['test']));
+  });
+
   test('fitToWidth', () {
     expect(Logger.fitToWidth('hello', 0), equals(''));
     expect(Logger.fitToWidth('hello', 1), equals('.'));
@@ -117,7 +130,9 @@ void main() {
     final Logger logger = Logger.test();
     bool called = false;
     final Spinner spinner = logger.startSpinner(
-      onFinish: () { called = true; },
+      onFinish: () {
+        called = true;
+      },
     );
     spinner.finish();
     expect(called, isTrue);