Refactor `BuildPlan`, better document and explain `--config`. (#56324)
Closes https://github.com/flutter/flutter/issues/156591.
Closes https://github.com/flutter/flutter/issues/156355.
I also moved some functions only used as implementation details :)
diff --git a/tools/engine_tool/BUILD.gn b/tools/engine_tool/BUILD.gn
index 0171d08..398eb01 100644
--- a/tools/engine_tool/BUILD.gn
+++ b/tools/engine_tool/BUILD.gn
@@ -8,6 +8,7 @@
testonly = true
public_deps = [
":build_command_test",
+ ":build_plan_test",
":entry_point_test",
":fetch_command_test",
":flutter_tools_test",
@@ -40,6 +41,10 @@
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"
}
diff --git a/tools/engine_tool/lib/src/build_plan.dart b/tools/engine_tool/lib/src/build_plan.dart
index 035e519..3aab6ec 100644
--- a/tools/engine_tool/lib/src/build_plan.dart
+++ b/tools/engine_tool/lib/src/build_plan.dart
@@ -6,6 +6,7 @@
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 @@
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 @@
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);
+ },
+ ),
+ ];
+}
diff --git a/tools/engine_tool/lib/src/build_utils.dart b/tools/engine_tool/lib/src/build_utils.dart
index 3a25b56..5a99a0f 100644
--- a/tools/engine_tool/lib/src/build_utils.dart
+++ b/tools/engine_tool/lib/src/build_utils.dart
@@ -11,51 +11,6 @@
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 @@
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/"',
);
}
diff --git a/tools/engine_tool/lib/src/commands/command_runner.dart b/tools/engine_tool/lib/src/commands/command_runner.dart
index 454cfa2..d1afdd8 100644
--- a/tools/engine_tool/lib/src/commands/command_runner.dart
+++ b/tools/engine_tool/lib/src/commands/command_runner.dart
@@ -25,7 +25,14 @@
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 @@
);
}
- /// 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;
diff --git a/tools/engine_tool/test/build_plan_test.dart b/tools/engine_tool/test/build_plan_test.dart
index 691ddbb..20d8a36 100644
--- a/tools/engine_tool/test/build_plan_test.dart
+++ b/tools/engine_tool/test/build_plan_test.dart
@@ -547,6 +547,75 @@
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.',
+ ]),
+ );
+ });
});
}
diff --git a/tools/engine_tool/test/commands/build_command_test.dart b/tools/engine_tool/test/commands/build_command_test.dart
index 3267ba8..243e75a 100644
--- a/tools/engine_tool/test/commands/build_command_test.dart
+++ b/tools/engine_tool/test/commands/build_command_test.dart
@@ -16,39 +16,6 @@
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,
