From 3800af160e3ae10a3e6b2d34bf1abb3608fc0866 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 30 Sep 2024 16:54:52 -0700 Subject: [PATCH 1/5] Refactor run_command and friends. --- .../lib/src/commands/run_command.dart | 242 +++++++++++++-- .../lib/src/flutter_tool_interop/device.dart | 65 +++++ .../flutter_tool_interop/flutter_tool.dart | 66 +++++ .../flutter_tool_interop/target_platform.dart | 154 ++++++++++ tools/engine_tool/lib/src/run_utils.dart | 162 ---------- .../external_tools/flutter_tools_test.dart | 208 +++++++++++++ .../test/{ => external_tools}/gn_test.dart | 2 +- tools/engine_tool/test/run_command_test.dart | 276 +++++++++--------- tools/engine_tool/test/run_target_test.dart | 197 +++++++++++++ tools/engine_tool/test/src/matchers.dart | 92 ++++++ 10 files changed, 1138 insertions(+), 326 deletions(-) create mode 100644 tools/engine_tool/lib/src/flutter_tool_interop/device.dart create mode 100644 tools/engine_tool/lib/src/flutter_tool_interop/flutter_tool.dart create mode 100644 tools/engine_tool/lib/src/flutter_tool_interop/target_platform.dart delete mode 100644 tools/engine_tool/lib/src/run_utils.dart create mode 100644 tools/engine_tool/test/external_tools/flutter_tools_test.dart rename tools/engine_tool/test/{ => external_tools}/gn_test.dart (99%) create mode 100644 tools/engine_tool/test/run_target_test.dart create mode 100644 tools/engine_tool/test/src/matchers.dart diff --git a/tools/engine_tool/lib/src/commands/run_command.dart b/tools/engine_tool/lib/src/commands/run_command.dart index dc015938d58d6..32de7ef18fed1 100644 --- a/tools/engine_tool/lib/src/commands/run_command.dart +++ b/tools/engine_tool/lib/src/commands/run_command.dart @@ -5,11 +5,15 @@ import 'dart:io' show ProcessStartMode; import 'package:engine_build_configs/engine_build_configs.dart'; +import 'package:meta/meta.dart'; import 'package:process_runner/process_runner.dart'; import '../build_utils.dart'; +import '../flutter_tool_interop/device.dart'; +import '../flutter_tool_interop/flutter_tool.dart'; +import '../flutter_tool_interop/target_platform.dart'; import '../label.dart'; -import '../run_utils.dart'; +import '../logger.dart'; import 'command.dart'; import 'flags.dart'; @@ -21,6 +25,7 @@ final class RunCommand extends CommandBase { required Map configs, super.help = false, super.usageLineLength, + @visibleForTesting FlutterTool? flutterTool, }) { // When printing the help/usage for this command, only list all builds // when the --verbose flag is supplied. @@ -41,8 +46,13 @@ final class RunCommand extends CommandBase { defaultsTo: environment.hasRbeConfigInTree(), help: 'RBE is enabled by default when available.', ); + + _flutterTool = flutterTool ?? FlutterTool.fromEnvironment(environment); } + /// Flutter tool. + late final FlutterTool _flutterTool; + /// List of compatible builds. late final List builds; @@ -58,7 +68,7 @@ Run a Flutter app with a local engine build. See `flutter run --help` for a listing '''; - Build? _lookup(String configName) { + Build? _findTargetBuild(String configName) { final String demangledName = demangleConfigName(environment, configName); return builds .where((Build build) => build.name == demangledName) @@ -78,11 +88,11 @@ See `flutter run --help` for a listing final String ci = mangledName.startsWith('ci') ? mangledName.substring(0, 3) : ''; if (mangledName.contains('_debug')) { - return _lookup('${ci}host_debug'); + return _findTargetBuild('${ci}host_debug'); } else if (mangledName.contains('_profile')) { - return _lookup('${ci}host_profile'); + return _findTargetBuild('${ci}host_profile'); } else if (mangledName.contains('_release')) { - return _lookup('${ci}host_release'); + return _findTargetBuild('${ci}host_release'); } return null; } @@ -114,43 +124,41 @@ See `flutter run --help` for a listing return mode; } - late final Future _runTarget = - detectAndSelectRunTarget(environment, _getDeviceId()); + late final Future _runTarget = (() async { + final devices = await _flutterTool.devices(); + return RunTarget.detectAndSelect(devices, idPrefix: _getDeviceId()); + })(); - Future _selectTargetConfig() async { - final String configName = argResults![configFlag] as String; + Future _selectTargetConfig() async { + final configName = argResults![configFlag] as String; if (configName.isNotEmpty) { return demangleConfigName(environment, configName); } - final RunTarget? target = await _runTarget; + final target = await _runTarget; if (target == null) { return demangleConfigName(environment, 'host_debug'); } - environment.logger.status( - 'Building to run on "${target.name}" running ${target.targetPlatform}'); - return target.buildConfigFor(_getMode()); + + final result = target.buildConfigFor(_getMode()); + environment.logger.status('Building to run on $result'); + return result; } @override Future run() async { if (!environment.processRunner.processManager.canRun('flutter')) { - environment.logger.error('Cannot find the flutter command in your path'); - return 1; + throw FatalError('Cannot find the "flutter" command in your PATH'); } - final String? configName = await _selectTargetConfig(); - if (configName == null) { - environment.logger.error('Could not find target config'); - return 1; - } - final Build? build = _lookup(configName); - final Build? hostBuild = _findHostBuild(build); - if (build == null) { - environment.logger.error('Could not find build $configName'); - return 1; + final configName = await _selectTargetConfig(); + + final targetBuild = _findTargetBuild(configName); + if (targetBuild == null) { + throw FatalError('Could not find build $configName'); } + + final hostBuild = _findHostBuild(targetBuild); if (hostBuild == null) { - environment.logger.error('Could not find host build for $configName'); - return 1; + throw FatalError('Could not find host build for $configName'); } final bool useRbe = argResults![rbeFlag] as bool; @@ -162,8 +170,7 @@ See `flutter run --help` for a listing if (!useRbe) '--no-rbe', ]; final RunTarget? target = await _runTarget; - final List