Refactor BuildPlan, better document and explain --config. (#56324)

Closes flutter/flutter#156591.
Closes flutter/flutter#156355.

I also moved some functions only used as implementation details :)

Author: matanlurey

tools/engine_tool/BUILD.gn

@@ -8,6 +8,7 @@ group("tests") {
   testonly = true
   public_deps = [
     ":build_command_test",
+    ":build_plan_test",
     ":entry_point_test",
     ":fetch_command_test",
     ":flutter_tools_test",
@@ -40,6 +41,10 @@ dart_test("build_command_test") {
   main_dart = "test/commands/build_command_test.dart"
 }
 
+dart_test("build_plan_test") {
+  main_dart = "test/commands/build_plan_test.dart"
+}
+
 dart_test("entry_point_test") {
   main_dart = "test/entry_point_test.dart"
 }

tools/engine_tool/lib/src/build_plan.dart

@@ -6,6 +6,7 @@ import 'package:args/args.dart';
 import 'package:collection/collection.dart';
 import 'package:engine_build_configs/engine_build_configs.dart';
 import 'package:meta/meta.dart';
+import 'package:platform/platform.dart';
 
 import 'build_utils.dart';
 import 'environment.dart';
@@ -162,28 +163,40 @@ final class BuildPlan {
     required Map<String, BuilderConfig> configs,
   }) {
     // Add --config.
-    final builds = runnableBuilds(
-      environment,
-      configs,
-      environment.verbose || !help,
+    final builds = _extractBuilds(
+      environment.platform,
+      runnableConfigs: _runnableBuildConfigs(
+        environment.platform,
+        configsByName: configs,
+      ),
+      hideCiSpecificBuilds: help && !environment.verbose,
     );
     debugCheckBuilds(builds);
     parser.addOption(
       _flagConfig,
       abbr: 'c',
-      defaultsTo: () {
-        if (builds.any((b) => b.name == 'host_debug')) {
-          return 'host_debug';
-        }
-        return null;
-      }(),
+      help: ''
+          'Selects a build configuration for the current platform.\n'
+          '\n'
+          'If omitted, et attempts '
+          'to default to a suitable target platform. This is typically a '
+          '"host_debug" build when building on a supported desktop OS, or a '
+          'suitable build when targeting (via "et run") a flutter app.\n'
+          '\n'
+          '${environment.verbose ? ''
+              'Since verbose mode was selected, both local development '
+              'configurations and configurations that are typically only '
+              'used on CI will be visible, including possible duplicates.' : ''
+              'Configurations include (use --verbose for more details):'}',
       allowed: [
         for (final config in builds) mangleConfigName(environment, config.name),
-      ],
-      allowedHelp: {
-        for (final config in builds)
-          mangleConfigName(environment, config.name): config.description,
-      },
+      ]..sort(),
+      allowedHelp: environment.verbose
+          ? {
+              for (final config in builds)
+                mangleConfigName(environment, config.name): config.description,
+            }
+          : null,
     );
 
     // Add --lto.
@@ -361,3 +374,41 @@ enum BuildStrategy {
   const BuildStrategy(this._help);
   final String _help;
 }
+
+typedef _ConfigsByName = Iterable<MapEntry<String, BuilderConfig>>;
+
+/// Computes a list of build configs that can can execute on [environment].
+_ConfigsByName _runnableBuildConfigs(
+  Platform platform, {
+  required Map<String, BuilderConfig> configsByName,
+}) {
+  return configsByName.entries.where((entry) {
+    return entry.value.canRunOn(platform);
+  });
+}
+
+/// Extracts [Build]s from [runnableConfigs] that can execute on [platform].
+///
+/// If [hideCiSpecificBuilds], builds that are unlikely to be picked for local
+/// development (i.e. start with the prefix `ci/` by convention) are not
+/// returned in order to make command-line _help_ text shorter.
+List<Build> _extractBuilds(
+  Platform platform, {
+  required _ConfigsByName runnableConfigs,
+  required bool hideCiSpecificBuilds,
+}) {
+  return [
+    for (final buildConfig in runnableConfigs)
+      ...buildConfig.value.builds.where(
+        (build) {
+          if (!build.canRunOn(platform)) {
+            return false;
+          }
+          if (!hideCiSpecificBuilds) {
+            return true;
+          }
+          return build.name.startsWith(platform.operatingSystem);
+        },
+      ),
+  ];
+}

tools/engine_tool/lib/src/build_utils.dart

@@ -11,51 +11,6 @@ import 'environment.dart';
 import 'label.dart';
 import 'logger.dart';
 
-/// A function that returns true or false when given a [BuilderConfig] and its
-/// name.
-typedef ConfigFilter = bool Function(String name, BuilderConfig config);
-
-/// A function that returns true or false when given a [BuilderConfig] name
-/// and a [Build].
-typedef BuildFilter = bool Function(String configName, Build build);
-
-/// Returns a filtered copy of [input] filtering out configs where test
-/// returns false.
-Map<String, BuilderConfig> filterBuilderConfigs(
-    Map<String, BuilderConfig> input, ConfigFilter test) {
-  return <String, BuilderConfig>{
-    for (final MapEntry<String, BuilderConfig> entry in input.entries)
-      if (test(entry.key, entry.value)) entry.key: entry.value,
-  };
-}
-
-/// Returns a copy of [input] filtering out configs that are not runnable
-/// on the current platform.
-Map<String, BuilderConfig> runnableBuilderConfigs(
-    Environment env, Map<String, BuilderConfig> input) {
-  return filterBuilderConfigs(input, (String name, BuilderConfig config) {
-    return config.canRunOn(env.platform);
-  });
-}
-
-/// Returns a List of [Build] that match test.
-List<Build> filterBuilds(Map<String, BuilderConfig> input, BuildFilter test) {
-  return <Build>[
-    for (final MapEntry<String, BuilderConfig> entry in input.entries)
-      for (final Build build in entry.value.builds)
-        if (test(entry.key, build)) build,
-  ];
-}
-
-/// Returns a list of runnable builds.
-List<Build> runnableBuilds(
-    Environment env, Map<String, BuilderConfig> input, bool verbose) {
-  return filterBuilds(input, (String configName, Build build) {
-    return build.canRunOn(env.platform) &&
-        (verbose || build.name.startsWith(env.platform.operatingSystem));
-  });
-}
-
 /// Validates the list of builds.
 /// Calls assert.
 void debugCheckBuilds(List<Build> builds) {
@@ -99,8 +54,10 @@ String mangleConfigName(Environment env, String name) {
   if (_doNotMangle(env, name)) {
     return name;
   }
-  throw ArgumentError(
-    'name argument "$name" must start with a valid platform name or "ci"',
+  throw ArgumentError.value(
+    name,
+    'name',
+    'Expected to start with a valid platform name (i.e. $osPrefix) or "ci/"',
   );
 }
 

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

@@ -25,7 +25,14 @@ final class ToolCommandRunner extends CommandRunner<int> {
     required this.environment,
     required this.configs,
     this.help = false,
-  }) : super(toolName, toolDescription, usageLineLength: _usageLineLength) {
+  }) : super(
+          'et',
+          ''
+              'A command line tool for working on '
+              'the Flutter Engine.\n\nThis is a community supported project, '
+              'for more information see https://flutter.dev/to/et.',
+          usageLineLength: _usageLineLength,
+        ) {
     final List<Command<int>> commands = <Command<int>>[
       FetchCommand(
         environment: environment,
@@ -73,16 +80,6 @@ final class ToolCommandRunner extends CommandRunner<int> {
     );
   }
 
-  /// The name of the tool as reported in the tool's usage and help
-  /// messages.
-  static const String toolName = 'et';
-
-  /// The description of the tool reported in the tool's usage and help
-  /// messages.
-  static const String toolDescription = 'A command line tool for working on '
-      'the Flutter Engine.\n\nThis is a community supported project, file '
-      'a bug or feature request: https://flutter.dev/to/engine-tool-bug.';
-
   /// The host system environment.
   final Environment environment;
 

tools/engine_tool/test/build_plan_test.dart

@@ -547,6 +547,75 @@ void main() {
         contains('Additional arguments to provide to "gn"'),
       );
     });
+
+    /// Returns an [ArgParser] pre-primed with both MacOS and Linux builds.
+    ArgParser createMultiPlatformArgParser({
+      required bool verbose,
+    }) {
+      final testEnv = TestEnvironment.withTestEngine(
+        verbose: verbose,
+      );
+      addTearDown(testEnv.cleanup);
+
+      final linuxConfig = TestBuilderConfig();
+      linuxConfig.addBuild(
+        name: 'linux/host_debug',
+        dimension: TestDroneDimension.linux,
+        description: 'A development build of the Linux host.',
+      );
+      linuxConfig.addBuild(
+        name: 'ci/linux_host_debug',
+        dimension: TestDroneDimension.linux,
+        description: 'A CI-suitable development build of the Linux host.',
+      );
+
+      final macOSConfig = TestBuilderConfig();
+      macOSConfig.addBuild(
+        name: 'macos/host_debug',
+        dimension: TestDroneDimension.mac,
+        description: 'A build we do not expect to see due to filtering.',
+      );
+
+      final parser = ArgParser();
+      final _ = BuildPlan.configureArgParser(
+        parser,
+        testEnv.environment,
+        configs: {
+          'linux_test_config': linuxConfig.buildConfig(
+            path: 'ci/builders/linux_test_config.json',
+          ),
+          'macos_test_config': macOSConfig.buildConfig(
+            path: 'ci/builders/macos_test_config.json',
+          ),
+        },
+        help: true,
+      );
+
+      return parser;
+    }
+
+    test('shows only non-CI builds that can run locally', () {
+      final parser = createMultiPlatformArgParser(verbose: false);
+
+      expect(
+        parser.usage,
+        contains('[host_debug]'),
+      );
+    });
+
+    test('shows builds that can run locally, with details', () {
+      final parser = createMultiPlatformArgParser(verbose: true);
+
+      expect(
+        parser.usage,
+        stringContainsInOrder([
+          '[ci/linux_host_debug]',
+          'A CI-suitable development build of the Linux host.',
+          '[host_debug]',
+          'A development build of the Linux host.',
+        ]),
+      );
+    });
   });
 }
 

tools/engine_tool/test/commands/build_command_test.dart

@@ -16,39 +16,6 @@ import '../src/test_build_configs.dart';
 import '../src/utils.dart';
 
 void main() {
-  test('can find host runnable build', () async {
-    final testEnv = TestEnvironment.withTestEngine(
-      abi: Abi.macosArm64,
-    );
-    addTearDown(testEnv.cleanup);
-
-    final builder = TestBuilderConfig();
-    builder.addBuild(
-      name: 'macos/host_debug',
-      dimension: TestDroneDimension.mac,
-    );
-    builder.addBuild(
-      name: 'mac/host_profile',
-      dimension: TestDroneDimension.mac,
-    );
-    builder.addBuild(
-      name: 'linux/host_debug',
-      dimension: TestDroneDimension.linux,
-    );
-
-    final configs = {
-      'mac_test_config': builder.buildConfig(
-        path: 'ci/builders/mac_test_config.json',
-      ),
-    };
-
-    final result = runnableBuilds(testEnv.environment, configs, true);
-    expect(
-      result.map((r) => r.name),
-      unorderedEquals(['macos/host_debug', 'mac/host_profile']),
-    );
-  });
-
   test('build command invokes gn', () async {
     final testEnv = TestEnvironment.withTestEngine(
       abi: Abi.macosArm64,