Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Small improvements to et lint command #51372

Merged
merged 1 commit into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion tools/engine_tool/lib/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
16 changes: 11 additions & 5 deletions tools/engine_tool/lib/src/commands/lint_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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 =
Expand All @@ -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;
Expand Down
42 changes: 1 addition & 41 deletions tools/engine_tool/lib/src/dart_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions tools/engine_tool/lib/src/json_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
75 changes: 53 additions & 22 deletions tools/engine_tool/lib/src/logger.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);
Expand All @@ -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));
}
Expand All @@ -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}');
}

Expand All @@ -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) {
Expand All @@ -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;
},
);
}

Expand All @@ -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, {
Expand Down Expand Up @@ -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!;
}
Expand All @@ -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();
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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;
}
2 changes: 1 addition & 1 deletion tools/engine_tool/lib/src/proc_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion tools/engine_tool/test/build_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please put the ), on a new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be manually formatting code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree in principle, we should follow our project's style guide. For now, this means manual formatting. In the future we might be able to use the new Dart formatter. This file was already using Flutter's formatting, I'm not a huge fan of being inconsistent within a single file.

processRunner: ProcessRunner(
processManager: FakeProcessManager(onStart: (List<String> command) {
runHistory.add(command);
Expand Down
10 changes: 5 additions & 5 deletions tools/engine_tool/test/fetch_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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(
Expand All @@ -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(
Expand Down
4 changes: 3 additions & 1 deletion tools/engine_tool/test/lint_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 16 additions & 1 deletion tools/engine_tool/test/logger_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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('.'));
Expand Down Expand Up @@ -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);
Expand Down
Loading