Skip to content

Commit b921089

Browse files
[flutter_plugin_tools] Add a new base command for looping over packages (flutter#4067)
Most of our commands are generally of the form: ``` for (each plugin as defined by the tool flags) check some things for success or failure print a summary all of the failing things exit non-zero if anything failed ``` Currently all that logic not consistent, having been at various points copied and pasted around, modified, in some cases rewritten. There's unnecessary boilerplate in each new command, and there's unnecessary variation that makes it harder both to maintain the tool, and to consume the test output: - There's no standard format for separating each plugin's run to search within a log - There's no standard format for the summary at the end - In some cases commands have been written to ToolExit on failure, which means we don't actually get the rest of the runs This makes a new base class for commands that follow this structure to use, with shared code for all the common bits. This makes it harder to accidentally write new commands incorrectly, easier to maintain the code, and lets us standardize output so that searching within large logs will be easier. This ports two commands over as a proof of concept to demonstrate that it works; more will be converted in follow-ups. Related to flutter#83413
1 parent 13e5b5b commit b921089

9 files changed

+710
-96
lines changed

script/tool/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
- `xctest` now supports running macOS tests in addition to iOS
77
- **Breaking change**: it now requires an `--ios` and/or `--macos` flag.
88
- The tooling now runs in strong null-safe mode.
9+
- Modified the output format of `pubspec-check` and `xctest`
910

1011
## 0.2.0
1112

script/tool/lib/src/common/core.dart

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,17 +53,35 @@ bool isFlutterPackage(FileSystemEntity entity) {
5353
}
5454
}
5555

56+
/// Prints `successMessage` in green.
57+
void printSuccess(String successMessage) {
58+
print(Colorize(successMessage)..green());
59+
}
60+
5661
/// Prints `errorMessage` in red.
5762
void printError(String errorMessage) {
58-
final Colorize redError = Colorize(errorMessage)..red();
59-
print(redError);
63+
print(Colorize(errorMessage)..red());
6064
}
6165

6266
/// Error thrown when a command needs to exit with a non-zero exit code.
67+
///
68+
/// While there is no specific definition of the meaning of different non-zero
69+
/// exit codes for this tool, commands should follow the general convention:
70+
/// 1: The command ran correctly, but found errors.
71+
/// 2: The command failed to run because the arguments were invalid.
72+
/// >2: The command failed to run correctly for some other reason. Ideally,
73+
/// each such failure should have a unique exit code within the context of
74+
/// that command.
6375
class ToolExit extends Error {
6476
/// Creates a tool exit with the given [exitCode].
6577
ToolExit(this.exitCode);
6678

6779
/// The code that the process should exit with.
6880
final int exitCode;
6981
}
82+
83+
/// A exit code for [ToolExit] for a successful run that found errors.
84+
const int exitCommandFoundErrors = 1;
85+
86+
/// A exit code for [ToolExit] for a failure to run due to invalid arguments.
87+
const int exitInvalidArguments = 2;
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:colorize/colorize.dart';
6+
import 'package:file/file.dart';
7+
import 'package:git/git.dart';
8+
import 'package:path/path.dart' as p;
9+
10+
import 'core.dart';
11+
import 'plugin_command.dart';
12+
import 'process_runner.dart';
13+
14+
/// An abstract base class for a command that iterates over a set of packages
15+
/// controlled by a standard set of flags, running some actions on each package,
16+
/// and collecting and reporting the success/failure of those actions.
17+
abstract class PackageLoopingCommand extends PluginCommand {
18+
/// Creates a command to operate on [packagesDir] with the given environment.
19+
PackageLoopingCommand(
20+
Directory packagesDir, {
21+
ProcessRunner processRunner = const ProcessRunner(),
22+
GitDir? gitDir,
23+
}) : super(packagesDir, processRunner: processRunner, gitDir: gitDir);
24+
25+
/// Called during [run] before any calls to [runForPackage]. This provides an
26+
/// opportunity to fail early if the command can't be run (e.g., because the
27+
/// arguments are invalid), and to set up any run-level state.
28+
Future<void> initializeRun() async {}
29+
30+
/// Runs the command for [package], returning a list of errors.
31+
///
32+
/// Errors may either be an empty string if there is no context that should
33+
/// be included in the final error summary (e.g., a command that only has a
34+
/// single failure mode), or strings that should be listed for that package
35+
/// in the final summary. An empty list indicates success.
36+
Future<List<String>> runForPackage(Directory package);
37+
38+
/// Whether or not the output (if any) of [runForPackage] is long, or short.
39+
///
40+
/// This changes the logging that happens at the start of each package's
41+
/// run; long output gets a banner-style message to make it easier to find,
42+
/// while short output gets a single-line entry.
43+
///
44+
/// When this is false, runForPackage output should be indented if possible,
45+
/// to make the output structure easier to follow.
46+
bool get hasLongOutput => true;
47+
48+
/// Whether to loop over all packages (e.g., including example/), rather than
49+
/// only top-level packages.
50+
bool get includeSubpackages => false;
51+
52+
/// The text to output at the start when reporting one or more failures.
53+
/// This will be followed by a list of packages that reported errors, with
54+
/// the per-package details if any.
55+
///
56+
/// This only needs to be overridden if the summary should provide extra
57+
/// context.
58+
String get failureListHeader => 'The following packages had errors:';
59+
60+
/// The text to output at the end when reporting one or more failures. This
61+
/// will be printed immediately after the a list of packages that reported
62+
/// errors.
63+
///
64+
/// This only needs to be overridden if the summary should provide extra
65+
/// context.
66+
String get failureListFooter => 'See above for full details.';
67+
68+
// ----------------------------------------
69+
70+
/// A convenience constant for [runForPackage] success that's more
71+
/// self-documenting than the value.
72+
static const List<String> success = <String>[];
73+
74+
/// A convenience constant for [runForPackage] failure without additional
75+
/// context that's more self-documenting than the value.
76+
static const List<String> failure = <String>[''];
77+
78+
/// Prints a message using a standard format indicating that the package was
79+
/// skipped, with an explanation of why.
80+
void printSkip(String reason) {
81+
print(Colorize('SKIPPING: $reason')..darkGray());
82+
}
83+
84+
/// Returns the identifying name to use for [package].
85+
///
86+
/// Implementations should not expect a specific format for this string, since
87+
/// it uses heuristics to try to be precise without being overly verbose. If
88+
/// an exact format (e.g., published name, or basename) is required, that
89+
/// should be used instead.
90+
String getPackageDescription(Directory package) {
91+
String packageName = p.relative(package.path, from: packagesDir.path);
92+
final List<String> components = p.split(packageName);
93+
// For the common federated plugin pattern of `foo/foo_subpackage`, drop
94+
// the first part since it's not useful.
95+
if (components.length == 2 &&
96+
components[1].startsWith('${components[0]}_')) {
97+
packageName = components[1];
98+
}
99+
return packageName;
100+
}
101+
102+
// ----------------------------------------
103+
104+
@override
105+
Future<void> run() async {
106+
await initializeRun();
107+
108+
final List<Directory> packages = includeSubpackages
109+
? await getPackages().toList()
110+
: await getPlugins().toList();
111+
112+
final Map<Directory, List<String>> results = <Directory, List<String>>{};
113+
for (final Directory package in packages) {
114+
_printPackageHeading(package);
115+
results[package] = await runForPackage(package);
116+
}
117+
118+
// If there were any errors reported, summarize them and exit.
119+
if (results.values.any((List<String> failures) => failures.isNotEmpty)) {
120+
const String indentation = ' ';
121+
printError(failureListHeader);
122+
for (final Directory package in packages) {
123+
final List<String> errors = results[package]!;
124+
if (errors.isNotEmpty) {
125+
final String errorIndentation = indentation * 2;
126+
String errorDetails = errors.join('\n$errorIndentation');
127+
if (errorDetails.isNotEmpty) {
128+
errorDetails = ':\n$errorIndentation$errorDetails';
129+
}
130+
printError(
131+
'$indentation${getPackageDescription(package)}$errorDetails');
132+
}
133+
}
134+
printError(failureListFooter);
135+
throw ToolExit(exitCommandFoundErrors);
136+
}
137+
138+
printSuccess('\n\nNo issues found!');
139+
}
140+
141+
/// Prints the status message indicating that the command is being run for
142+
/// [package].
143+
///
144+
/// Something is always printed to make it easier to distinguish between
145+
/// a command running for a package and producing no output, and a command
146+
/// not having been run for a package.
147+
void _printPackageHeading(Directory package) {
148+
String heading = 'Running for ${getPackageDescription(package)}';
149+
if (hasLongOutput) {
150+
heading = '''
151+
152+
============================================================
153+
|| $heading
154+
============================================================
155+
''';
156+
} else {
157+
heading = '$heading...';
158+
}
159+
print(Colorize(heading)..cyan());
160+
}
161+
}

script/tool/lib/src/common/plugin_command.dart

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import 'git_version_finder.dart';
1414
import 'process_runner.dart';
1515

1616
/// Interface definition for all commands in this tool.
17+
// TODO(stuartmorgan): Move most of this logic to PackageLoopingCommand.
1718
abstract class PluginCommand extends Command<void> {
1819
/// Creates a command to operate on [packagesDir] with the given environment.
1920
PluginCommand(
@@ -136,6 +137,8 @@ abstract class PluginCommand extends Command<void> {
136137

137138
/// Returns the root Dart package folders of the plugins involved in this
138139
/// command execution.
140+
// TODO(stuartmorgan): Rename/restructure this, _getAllPlugins, and
141+
// getPackages, as the current naming is very confusing.
139142
Stream<Directory> getPlugins() async* {
140143
// To avoid assuming consistency of `Directory.list` across command
141144
// invocations, we collect and sort the plugin folders before sharding.

script/tool/lib/src/pubspec_check_command.dart

Lines changed: 14 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,17 @@
44

55
import 'package:file/file.dart';
66
import 'package:git/git.dart';
7-
import 'package:path/path.dart' as p;
87
import 'package:pubspec_parse/pubspec_parse.dart';
98

10-
import 'common/core.dart';
11-
import 'common/plugin_command.dart';
9+
import 'common/package_looping_command.dart';
1210
import 'common/process_runner.dart';
1311

1412
/// A command to enforce pubspec conventions across the repository.
1513
///
1614
/// This both ensures that repo best practices for which optional fields are
1715
/// used are followed, and that the structure is consistent to make edits
1816
/// across multiple pubspec files easier.
19-
class PubspecCheckCommand extends PluginCommand {
17+
class PubspecCheckCommand extends PackageLoopingCommand {
2018
/// Creates an instance of the version check command.
2119
PubspecCheckCommand(
2220
Directory packagesDir, {
@@ -52,29 +50,20 @@ class PubspecCheckCommand extends PluginCommand {
5250
'Checks that pubspecs follow repository conventions.';
5351

5452
@override
55-
Future<void> run() async {
56-
final List<String> failingPackages = <String>[];
57-
await for (final Directory package in getPackages()) {
58-
final String relativePackagePath =
59-
p.relative(package.path, from: packagesDir.path);
60-
print('Checking $relativePackagePath...');
61-
final File pubspec = package.childFile('pubspec.yaml');
62-
final bool passesCheck = !pubspec.existsSync() ||
63-
await _checkPubspec(pubspec, packageName: package.basename);
64-
if (!passesCheck) {
65-
failingPackages.add(relativePackagePath);
66-
}
67-
}
53+
bool get hasLongOutput => false;
6854

69-
if (failingPackages.isNotEmpty) {
70-
print('The following packages have pubspec issues:');
71-
for (final String package in failingPackages) {
72-
print(' $package');
73-
}
74-
throw ToolExit(1);
75-
}
55+
@override
56+
bool get includeSubpackages => true;
7657

77-
print('\nNo pubspec issues found!');
58+
@override
59+
Future<List<String>> runForPackage(Directory package) async {
60+
final File pubspec = package.childFile('pubspec.yaml');
61+
final bool passesCheck = !pubspec.existsSync() ||
62+
await _checkPubspec(pubspec, packageName: package.basename);
63+
if (!passesCheck) {
64+
return PackageLoopingCommand.failure;
65+
}
66+
return PackageLoopingCommand.success;
7867
}
7968

8069
Future<bool> _checkPubspec(

0 commit comments

Comments
 (0)