From 76b827076fa98488d13618dc458e403a13762477 Mon Sep 17 00:00:00 2001 From: Zach Anderson Date: Thu, 15 Feb 2024 09:05:42 -0800 Subject: [PATCH 1/2] [et] Adds a logger --- tools/engine_tool/lib/main.dart | 6 +- .../lib/src/commands/command_runner.dart | 4 +- .../lib/src/commands/query_command.dart | 22 ++- tools/engine_tool/lib/src/environment.dart | 16 +-- tools/engine_tool/lib/src/logger.dart | 135 ++++++++++++++++++ tools/engine_tool/pubspec.yaml | 3 + tools/engine_tool/test/logger_test.dart | 56 ++++++++ .../engine_tool/test/query_command_test.dart | 38 ++--- 8 files changed, 233 insertions(+), 47 deletions(-) create mode 100644 tools/engine_tool/lib/src/logger.dart create mode 100644 tools/engine_tool/test/logger_test.dart diff --git a/tools/engine_tool/lib/main.dart b/tools/engine_tool/lib/main.dart index cd42035b63d8d..8b8f705e9732c 100644 --- a/tools/engine_tool/lib/main.dart +++ b/tools/engine_tool/lib/main.dart @@ -3,7 +3,7 @@ // found in the LICENSE file. import 'dart:ffi' as ffi show Abi; -import 'dart:io' as io show Directory, exitCode, stderr, stdout; +import 'dart:io' as io show Directory, exitCode, stderr; import 'package:engine_build_configs/engine_build_configs.dart'; import 'package:engine_repo_tools/engine_repo_tools.dart'; @@ -13,6 +13,7 @@ import 'package:process_runner/process_runner.dart'; import 'src/commands/command_runner.dart'; import 'src/environment.dart'; +import 'src/logger.dart'; void main(List args) async { // Find the engine repo. @@ -55,8 +56,7 @@ void main(List args) async { engine: engine, platform: const LocalPlatform(), processRunner: ProcessRunner(), - stderr: io.stderr, - stdout: io.stdout, + logger: Logger(), ), configs: configs, ); diff --git a/tools/engine_tool/lib/src/commands/command_runner.dart b/tools/engine_tool/lib/src/commands/command_runner.dart index 57aea41e02807..360e649aef775 100644 --- a/tools/engine_tool/lib/src/commands/command_runner.dart +++ b/tools/engine_tool/lib/src/commands/command_runner.dart @@ -43,10 +43,10 @@ final class ToolCommandRunner extends CommandRunner { try{ return await runCommand(parse(args)) ?? 1; } on FormatException catch (e) { - environment.stderr.writeln(e); + environment.logger.error(e); return 1; } on UsageException catch (e) { - environment.stderr.writeln(e); + environment.logger.error(e); return 1; } } diff --git a/tools/engine_tool/lib/src/commands/query_command.dart b/tools/engine_tool/lib/src/commands/query_command.dart index 20c87e6540d96..a9de5a8a0c727 100644 --- a/tools/engine_tool/lib/src/commands/query_command.dart +++ b/tools/engine_tool/lib/src/commands/query_command.dart @@ -63,12 +63,6 @@ final class QueryCommand extends CommandBase { @override String get description => 'Provides information about build configurations ' 'and tests.'; - - @override - Future run() async { - environment.stdout.write(usage); - return 0; - } } /// The 'query builds' command. @@ -97,10 +91,10 @@ final class QueryBuildsCommand extends CommandBase { final String? builderName = parent!.argResults![_builderFlag] as String?; final bool verbose = parent!.argResults![_verboseFlag] as bool; if (!verbose) { - environment.stdout.writeln( + environment.logger.status( 'Add --verbose to see detailed information about each builder', ); - environment.stdout.writeln(); + environment.logger.status(''); } for (final String key in configs.keys) { if (builderName != null && key != builderName) { @@ -112,23 +106,23 @@ final class QueryBuildsCommand extends CommandBase { continue; } - environment.stdout.writeln('"$key" builder:'); + environment.logger.status('"$key" builder:'); for (final GlobalBuild build in config.builds) { if (!build.canRunOn(environment.platform) && !all) { continue; } - environment.stdout.writeln(' "${build.name}" config'); + environment.logger.status('"${build.name}" config', indent: 3); if (!verbose) { continue; } - environment.stdout.writeln(' gn flags:'); + environment.logger.status('gn flags:', indent: 6); for (final String flag in build.gn) { - environment.stdout.writeln(' $flag'); + environment.logger.status(flag, indent: 9); } if (build.ninja.targets.isNotEmpty) { - environment.stdout.writeln(' ninja targets:'); + environment.logger.status('ninja targets:', indent: 6); for (final String target in build.ninja.targets) { - environment.stdout.writeln(' $target'); + environment.logger.status(target, indent: 9); } } } diff --git a/tools/engine_tool/lib/src/environment.dart b/tools/engine_tool/lib/src/environment.dart index f210977b02445..dfe40e501c4ac 100644 --- a/tools/engine_tool/lib/src/environment.dart +++ b/tools/engine_tool/lib/src/environment.dart @@ -8,6 +8,8 @@ import 'package:engine_repo_tools/engine_repo_tools.dart'; import 'package:platform/platform.dart'; import 'package:process_runner/process_runner.dart'; +import 'logger.dart'; + /// This class encapsulates information about the host system. /// /// Rather than being written directly against `dart:io`, implementations in the @@ -19,10 +21,9 @@ final class Environment { Environment({ required this.abi, required this.engine, + required this.logger, required this.platform, required this.processRunner, - required this.stderr, - required this.stdout, }); /// The host OS and architecture that the tool is running on. @@ -31,17 +32,12 @@ final class Environment { /// Information about paths in the engine repo. final Engine engine; + /// Where log messages are written. + final Logger logger; + /// More detailed information about the host platform. final Platform platform; /// Facility for commands to run subprocesses. final ProcessRunner processRunner; - - // TODO(zanderso): Replace stderr and stdout with a real logger. - - /// A sink for error messages from commands. - final StringSink stderr; - - /// A sink for non-error messages from commands. - final StringSink stdout; } diff --git a/tools/engine_tool/lib/src/logger.dart b/tools/engine_tool/lib/src/logger.dart new file mode 100644 index 0000000000000..08cf62657894a --- /dev/null +++ b/tools/engine_tool/lib/src/logger.dart @@ -0,0 +1,135 @@ +// Copyright 2013 The Flutter 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 'dart:async' show runZoned; +import 'dart:io' as io show + IOSink, + stderr, + stdout; + +import 'package:logging/logging.dart' as log; +import 'package:meta/meta.dart'; + +// This is where a flutter_tool style progress spinner, color output, +// ascii art, terminal control for clearing lines or the whole screen, etc. +// can go. We can just add more methods to Logger using the flutter_tool's +// Logger as a guide: +// +// https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/base/logger.dart#L422 + +/// A simplified wrapper around the [Logger] from package:logging. +/// +/// The default log level is [Logger.status]. A --quiet flag might change it to +/// [Logger.warning] or [Logger.error]. A --verbose flag might change it to +/// [Logger.info]. +/// +/// Log messages at [Logger.warning] and higher will be written to stderr, and +/// to stdout otherwise. [Logger.test] records all log messages to a buffer, +/// which can be inspected by unit tetss. +class Logger { + /// Constructs a logger for use in the tool. + Logger() : _logger = log.Logger.detached('et') { + _logger.level = statusLevel; + _logger.onRecord.listen(_handler); + _setupIoSink(io.stderr); + _setupIoSink(io.stdout); + } + + /// A logger for tests. + @visibleForTesting + Logger.test() : _logger = log.Logger.detached('et') { + _logger.level = statusLevel; + _logger.onRecord.listen((log.LogRecord r) => _testLogs.add(r)); + } + + /// The logging level for error messages. These go to stderr. + static const log.Level errorLevel = log.Level('ERROR', 100); + + /// The logging level for warning messages. These go to stderr. + static const log.Level warningLevel = log.Level('WARNING', 75); + + /// The logging level for normal status messages. These go to stdout. + static const log.Level statusLevel = log.Level('STATUS', 25); + + /// The logging level for verbose informational messages. These go to stdout. + static const log.Level infoLevel = log.Level('INFO', 10); + + 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}: ' + : ''; + _ioSinkWrite(sink, '$prefix${r.message}'); + } + + // Status of the global io.stderr and io.stdout is shared across all + // Logger instances. + static bool _stdioDone = false; + + // stdout and stderr might already be closed, and when not already closed, + // writing can still fail by throwing either a sync or async exception. + // This function handles all three cases. + static void _ioSinkWrite(io.IOSink sink, String message) { + if (_stdioDone) { + return; + } + runZoned(() { + try { + sink.writeln(message); + } catch (_) { // ignore: avoid_catches_without_on_clauses + _stdioDone = true; + } + }, onError: (Object e, StackTrace s) { + _stdioDone = true; + }); + } + + static void _setupIoSink(io.IOSink sink) { + sink.done.then( + (void _) { _stdioDone = true; }, + onError: (Object err, StackTrace st) { _stdioDone = true; }, + ); + } + + final log.Logger _logger; + final List _testLogs = []; + + /// Get the current logging level. + log.Level get level => _logger.level; + + /// Set the current logging level. + set level(log.Level l) { + _logger.level = l; + } + + /// Record a log message at level [Logger.error]. + void error(Object? message, {int indent = 0}) { + _emitLog(errorLevel, message, indent); + } + + /// Record a log message at level [Logger.warning]. + void warning(Object? message, {int indent = 0}) { + _emitLog(warningLevel, message, indent); + } + + /// Record a log message at level [Logger.warning]. + void status(Object? message, {int indent = 0}) { + _emitLog(statusLevel, message, indent); + } + + /// Record a log message at level [Logger.info]. + void info(Object? message, {int indent = 0}) { + _emitLog(infoLevel, message, indent); + } + + void _emitLog(log.Level level, Object? message, int indent) { + final String m = '${' ' * indent}$message'; + _logger.log(level, m); + } + + /// In a [Logger] constructed by [Logger.test], this list will contain all of + /// the [LogRecord]s emitted by the test. + @visibleForTesting + List get testLogs => _testLogs; +} diff --git a/tools/engine_tool/pubspec.yaml b/tools/engine_tool/pubspec.yaml index b4e2713e7b156..3142458e89137 100644 --- a/tools/engine_tool/pubspec.yaml +++ b/tools/engine_tool/pubspec.yaml @@ -23,6 +23,7 @@ dependencies: engine_repo_tools: path: ../pkg/engine_repo_tools file: any + logging: any meta: any path: any platform: any @@ -51,6 +52,8 @@ dependency_overrides: path: ../../../third_party/dart/third_party/pkg/file/packages/file litetest: path: ../../testing/litetest + logging: + path: ../../../third_party/dart/third_party/pkg/logging meta: path: ../../../third_party/dart/pkg/meta path: diff --git a/tools/engine_tool/test/logger_test.dart b/tools/engine_tool/test/logger_test.dart new file mode 100644 index 0000000000000..48eb78706012f --- /dev/null +++ b/tools/engine_tool/test/logger_test.dart @@ -0,0 +1,56 @@ +// Copyright 2013 The Flutter 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:engine_tool/src/logger.dart'; +import 'package:litetest/litetest.dart'; +import 'package:logging/logging.dart' as log; + +void main() { + List stringsFromLogs(List logs) { + return logs.map((log.LogRecord r) => r.message).toList(); + } + + test('Setting the level works', () { + final Logger logger = Logger.test(); + logger.level = Logger.infoLevel; + expect(logger.level, equals(Logger.infoLevel)); + }); + + test('error messages are recorded at the default log level', () { + final Logger logger = Logger.test(); + logger.error('Error'); + expect(stringsFromLogs(logger.testLogs), equals(['Error'])); + }); + + test('warning messages are recorded at the default log level', () { + final Logger logger = Logger.test(); + logger.warning('Warning'); + expect(stringsFromLogs(logger.testLogs), equals(['Warning'])); + }); + + test('status messages are recorded at the default log level', () { + final Logger logger = Logger.test(); + logger.status('Status'); + expect(stringsFromLogs(logger.testLogs), equals(['Status'])); + }); + + test('info messages are not recorded at the default log level', () { + final Logger logger = Logger.test(); + logger.info('info'); + expect(stringsFromLogs(logger.testLogs), equals([])); + }); + + test('info messages are recorded at the infoLevel log level', () { + final Logger logger = Logger.test(); + logger.level = Logger.infoLevel; + logger.info('info'); + expect(stringsFromLogs(logger.testLogs), equals(['info'])); + }); + + test('indent indents the message', () { + final Logger logger = Logger.test(); + logger.status('Status', indent: 1); + expect(stringsFromLogs(logger.testLogs), equals([' Status'])); + }); +} diff --git a/tools/engine_tool/test/query_command_test.dart b/tools/engine_tool/test/query_command_test.dart index d2f678f1b273e..9661be4e9303f 100644 --- a/tools/engine_tool/test/query_command_test.dart +++ b/tools/engine_tool/test/query_command_test.dart @@ -10,7 +10,9 @@ import 'package:engine_build_configs/engine_build_configs.dart'; import 'package:engine_repo_tools/engine_repo_tools.dart'; import 'package:engine_tool/src/commands/command_runner.dart'; import 'package:engine_tool/src/environment.dart'; +import 'package:engine_tool/src/logger.dart'; import 'package:litetest/litetest.dart'; +import 'package:logging/logging.dart' as log; import 'package:platform/platform.dart'; import 'package:process_fakes/process_fakes.dart'; import 'package:process_runner/process_runner.dart'; @@ -49,7 +51,7 @@ void main() { 'win_test_config': winTestConfig, }; - Environment linuxEnv(StringBuffer stderr, StringBuffer stdout) { + Environment linuxEnv(Logger logger) { return Environment( abi: ffi.Abi.linuxX64, engine: engine, @@ -57,15 +59,17 @@ void main() { processRunner: ProcessRunner( processManager: FakeProcessManager(), ), - stderr: stderr, - stdout: stdout, + logger: logger, ); } + List stringsFromLogs(List logs) { + return logs.map((log.LogRecord r) => r.message).toList(); + } + test('query command returns builds for the host platform.', () async { - final StringBuffer stderr = StringBuffer(); - final StringBuffer stdout = StringBuffer(); - final Environment env = linuxEnv(stderr, stdout); + final Logger logger = Logger.test(); + final Environment env = linuxEnv(logger); final ToolCommandRunner runner = ToolCommandRunner( environment: env, configs: configs, @@ -75,22 +79,21 @@ void main() { ]); expect(result, equals(0)); expect( - stdout.toString().trim().split('\n'), + stringsFromLogs(logger.testLogs), equals([ 'Add --verbose to see detailed information about each builder', '', '"linux_test_config" builder:', - ' "build_name" config', + ' "build_name" config', '"linux_test_config2" builder:', - ' "build_name" config', + ' "build_name" config', ]), ); }); test('query command with --builder returns only from the named builder.', () async { - final StringBuffer stderr = StringBuffer(); - final StringBuffer stdout = StringBuffer(); - final Environment env = linuxEnv(stderr, stdout); + final Logger logger = Logger.test(); + final Environment env = linuxEnv(logger); final ToolCommandRunner runner = ToolCommandRunner( environment: env, configs: configs, @@ -100,20 +103,19 @@ void main() { ]); expect(result, equals(0)); expect( - stdout.toString().trim().split('\n'), + stringsFromLogs(logger.testLogs), equals([ 'Add --verbose to see detailed information about each builder', '', '"linux_test_config" builder:', - ' "build_name" config', + ' "build_name" config', ]), ); }); test('query command with --all returns all builds.', () async { - final StringBuffer stderr = StringBuffer(); - final StringBuffer stdout = StringBuffer(); - final Environment env = linuxEnv(stderr, stdout); + final Logger logger = Logger.test(); + final Environment env = linuxEnv(logger); final ToolCommandRunner runner = ToolCommandRunner( environment: env, configs: configs, @@ -123,7 +125,7 @@ void main() { ]); expect(result, equals(0)); expect( - stdout.toString().trim().split('\n').length, + logger.testLogs.length, equals(10), ); }); From 620bb13645f28863d40d4f5cbd1afecb593c34ac Mon Sep 17 00:00:00 2001 From: Zachary Anderson Date: Thu, 15 Feb 2024 13:03:04 -0800 Subject: [PATCH 2/2] Update tools/engine_tool/lib/src/logger.dart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> --- tools/engine_tool/lib/src/logger.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/engine_tool/lib/src/logger.dart b/tools/engine_tool/lib/src/logger.dart index 08cf62657894a..97b9478b06f9f 100644 --- a/tools/engine_tool/lib/src/logger.dart +++ b/tools/engine_tool/lib/src/logger.dart @@ -16,7 +16,7 @@ import 'package:meta/meta.dart'; // can go. We can just add more methods to Logger using the flutter_tool's // Logger as a guide: // -// https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/base/logger.dart#L422 +// https://github.com/flutter/flutter/blob/c530276f7806c77da2541c518a0e103c9bb44f10/packages/flutter_tools/lib/src/base/logger.dart#L422 /// A simplified wrapper around the [Logger] from package:logging. ///