Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

Move verbose to environment.verbose, pass to ninja --verbose if provided. #52619

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tools/engine_tool/lib/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ void main(List<String> args) async {
platform: const LocalPlatform(),
processRunner: ProcessRunner(),
logger: Logger(),
verbose: verbose,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: as a further cleanup, the verbosity could just be encoded in the log level of the Logger() on the line above. It's currently set in https://github.com/flutter/engine/blob/main/tools/engine_tool/lib/src/commands/command_runner.dart#L106, but could just as well be set here.

Copy link
Contributor Author

@matanlurey matanlurey May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense, but probably what I would do is something like Logger(verbose: true) and have Logger set itself up versus trying to overload Logger's log severity (where verbose isn't really a thing) with the app-wide verbosity flag.

Let me send a follow-up PR and we can chomp on that a bit.

);

// Use the Engine and BuildConfig collection to build the CommandRunner.
final ToolCommandRunner runner = ToolCommandRunner(
environment: environment,
configs: configs,
verbose: verbose,
help: help,
);

Expand Down
17 changes: 12 additions & 5 deletions tools/engine_tool/lib/src/build_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ 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));
(verbose || build.name.startsWith(env.platform.operatingSystem));
});
}

Expand Down Expand Up @@ -119,9 +119,12 @@ String demangleConfigName(Environment env, String name) {
}

/// Build the build target in the environment.
Future<int> runBuild(Environment environment, Build build,
{List<String> extraGnArgs = const <String>[],
List<String> targets = const <String>[]}) async {
Future<int> runBuild(
Environment environment,
Build build, {
List<String> extraGnArgs = const <String>[],
List<String> targets = const <String>[],
}) async {
// If RBE config files aren't in the tree, then disable RBE.
final String rbeConfigPath = p.join(
environment.engine.srcDir.path,
Expand All @@ -143,7 +146,11 @@ Future<int> runBuild(Environment environment, Build build,
build: build,
extraGnArgs: gnArgs,
runTests: false,
extraNinjaArgs: targets,
extraNinjaArgs: <String>[
...targets,
// If the environment is verbose, pass the verbose flag to ninja.
if (environment.verbose) '--verbose',
],
);

Spinner? spinner;
Expand Down
3 changes: 1 addition & 2 deletions tools/engine_tool/lib/src/commands/build_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,12 @@ final class BuildCommand extends CommandBase {
BuildCommand({
required super.environment,
required Map<String, BuilderConfig> configs,
super.verbose = false,
super.help = false,
super.usageLineLength,
}) {
// When printing the help/usage for this command, only list all builds
// when the --verbose flag is supplied.
final bool includeCiBuilds = verbose || !help;
final bool includeCiBuilds = environment.verbose || !help;
builds = runnableBuilds(environment, configs, includeCiBuilds);
debugCheckBuilds(builds);
addConfigOption(
Expand Down
4 changes: 0 additions & 4 deletions tools/engine_tool/lib/src/commands/command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,13 @@ abstract base class CommandBase extends Command<int> {
/// Constructs the base command.
CommandBase({
required this.environment,
this.verbose = false,
this.help = false,
int? usageLineLength,
}) : argParser = ArgParser(usageLineLength: usageLineLength);

/// The host system environment.
final Environment environment;

/// Whether verbose logging is enabled.
final bool verbose;

/// Whether the Command is being constructed only to print the usage/help
/// message.
final bool help;
Expand Down
10 changes: 1 addition & 9 deletions tools/engine_tool/lib/src/commands/command_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ final class ToolCommandRunner extends CommandRunner<int> {
ToolCommandRunner({
required this.environment,
required this.configs,
this.verbose = false,
this.help = false,
}) : super(toolName, toolDescription, usageLineLength: _usageLineLength) {
final List<Command<int>> commands = <Command<int>>[
Expand All @@ -40,21 +39,18 @@ final class ToolCommandRunner extends CommandRunner<int> {
QueryCommand(
environment: environment,
configs: configs,
verbose: verbose,
help: help,
usageLineLength: _usageLineLength,
),
BuildCommand(
environment: environment,
configs: configs,
verbose: verbose,
help: help,
usageLineLength: _usageLineLength,
),
RunCommand(
environment: environment,
configs: configs,
verbose: verbose,
usageLineLength: _usageLineLength,
),
LintCommand(
Expand All @@ -64,7 +60,6 @@ final class ToolCommandRunner extends CommandRunner<int> {
TestCommand(
environment: environment,
configs: configs,
verbose: verbose,
help: help,
usageLineLength: _usageLineLength,
),
Expand Down Expand Up @@ -94,15 +89,12 @@ final class ToolCommandRunner extends CommandRunner<int> {
/// Build configurations loaded from the engine from under ci/builders.
final Map<String, BuilderConfig> configs;

/// Whether et should emit verbose logs.
final bool verbose;

/// Whether the invocation is for a help command
final bool help;

@override
Future<int> run(Iterable<String> args) async {
if (verbose) {
if (environment.verbose) {
environment.logger.level = Logger.infoLevel;
}
try {
Expand Down
4 changes: 1 addition & 3 deletions tools/engine_tool/lib/src/commands/fetch_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import '../dependencies.dart';
import 'command.dart';
import 'flags.dart';

/// The root 'fetch' command.
final class FetchCommand extends CommandBase {
Expand All @@ -25,7 +24,6 @@ final class FetchCommand extends CommandBase {

@override
Future<int> run() {
final bool verbose = globalResults![verboseFlag] as bool;
return fetchDependencies(environment, verbose: verbose);
return fetchDependencies(environment);
}
}
11 changes: 3 additions & 8 deletions tools/engine_tool/lib/src/commands/query_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ final class QueryCommand extends CommandBase {
QueryCommand({
required super.environment,
required this.configs,
super.verbose = false,
super.help = false,
super.usageLineLength,
}) {
Expand Down Expand Up @@ -45,13 +44,11 @@ final class QueryCommand extends CommandBase {
addSubcommand(QueryBuildersCommand(
environment: environment,
configs: configs,
verbose: verbose,
help: help,
));
addSubcommand(QueryTargetsCommand(
environment: environment,
configs: configs,
verbose: verbose,
help: help,
));
}
Expand All @@ -73,7 +70,6 @@ final class QueryBuildersCommand extends CommandBase {
QueryBuildersCommand({
required super.environment,
required this.configs,
super.verbose = false,
super.help = false,
});

Expand All @@ -93,7 +89,7 @@ final class QueryBuildersCommand extends CommandBase {
// current platform.
final bool all = parent!.argResults![allFlag]! as bool;
final String? builderName = parent!.argResults![builderFlag] as String?;
if (!verbose) {
if (!environment.verbose) {
environment.logger.status(
'Add --verbose to see detailed information about each builder',
);
Expand All @@ -115,7 +111,7 @@ final class QueryBuildersCommand extends CommandBase {
continue;
}
environment.logger.status('"${build.name}" config', indent: 3);
if (!verbose) {
if (!environment.verbose) {
continue;
}
environment.logger.status('gn flags:', indent: 6);
Expand All @@ -140,12 +136,11 @@ final class QueryTargetsCommand extends CommandBase {
QueryTargetsCommand({
required super.environment,
required this.configs,
super.verbose = false,
super.help = false,
}) {
// When printing the help/usage for this command, only list all builds
// when the --verbose flag is supplied.
final bool includeCiBuilds = verbose || !help;
final bool includeCiBuilds = environment.verbose || !help;
builds = runnableBuilds(environment, configs, includeCiBuilds);
debugCheckBuilds(builds);
addConfigOption(
Expand Down
3 changes: 1 addition & 2 deletions tools/engine_tool/lib/src/commands/run_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,12 @@ final class RunCommand extends CommandBase {
RunCommand({
required super.environment,
required Map<String, BuilderConfig> configs,
super.verbose = false,
super.help = false,
super.usageLineLength,
}) {
// When printing the help/usage for this command, only list all builds
// when the --verbose flag is supplied.
final bool includeCiBuilds = verbose || !help;
final bool includeCiBuilds = environment.verbose || !help;
builds = runnableBuilds(environment, configs, includeCiBuilds);
debugCheckBuilds(builds);
// We default to nothing in order to automatically detect attached devices
Expand Down
3 changes: 1 addition & 2 deletions tools/engine_tool/lib/src/commands/test_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,12 @@ final class TestCommand extends CommandBase {
TestCommand({
required super.environment,
required Map<String, BuilderConfig> configs,
super.verbose = false,
super.help = false,
super.usageLineLength,
}) {
// When printing the help/usage for this command, only list all builds
// when the --verbose flag is supplied.
final bool includeCiBuilds = verbose || !help;
final bool includeCiBuilds = environment.verbose || !help;
builds = runnableBuilds(environment, configs, includeCiBuilds);
debugCheckBuilds(builds);
addConfigOption(
Expand Down
20 changes: 10 additions & 10 deletions tools/engine_tool/lib/src/dependencies.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,21 @@ import 'environment.dart';
import 'logger.dart';

/// Update Flutter engine dependencies. Returns an exit code.
Future<int> fetchDependencies(
Environment environment, {
bool verbose = false,
}) async {
Future<int> fetchDependencies(Environment environment) async {
if (!environment.processRunner.processManager.canRun('gclient')) {
environment.logger.error('Cannot find the gclient command in your path');
return 1;
}

environment.logger.status('Fetching dependencies... ', newline: verbose);
environment.logger.status(
'Fetching dependencies... ',
newline: environment.verbose,
);

Spinner? spinner;
ProcessRunnerResult result;
try {
if (!verbose) {
if (!environment.verbose) {
spinner = environment.logger.startSpinner();
}

Expand All @@ -35,9 +35,9 @@ Future<int> fetchDependencies(
'-D',
],
runInShell: true,
startMode: verbose
? io.ProcessStartMode.inheritStdio
: io.ProcessStartMode.normal,
startMode: environment.verbose
? io.ProcessStartMode.inheritStdio
: io.ProcessStartMode.normal,
);
} finally {
spinner?.finish();
Expand All @@ -48,7 +48,7 @@ Future<int> fetchDependencies(

// Verbose mode already logged output by making the child process inherit
// this process's stdio handles.
if (!verbose) {
if (!environment.verbose) {
environment.logger.error('Output:\n${result.output}');
}
}
Expand Down
4 changes: 4 additions & 0 deletions tools/engine_tool/lib/src/environment.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@ final class Environment {
required this.logger,
required this.platform,
required this.processRunner,
this.verbose = false,
});

/// Whether the tool should be considered running in "verbose" mode.
final bool verbose;

/// The host OS and architecture that the tool is running on.
final ffi.Abi abi;

Expand Down
2 changes: 1 addition & 1 deletion tools/engine_tool/test/build_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -398,12 +398,12 @@ void main() {
() async {
final TestEnvironment testEnv = TestEnvironment.withTestEngine(
cannedProcesses: cannedProcesses,
verbose: true,
);
try {
final ToolCommandRunner runner = ToolCommandRunner(
environment: testEnv.environment,
configs: configs,
verbose: true,
help: true,
);
final int result = await runner.run(<String>[
Expand Down
14 changes: 11 additions & 3 deletions tools/engine_tool/test/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class TestEnvironment {
Engine engine, {
Logger? logger,
ffi.Abi abi = ffi.Abi.macosArm64,
bool verbose = false,
this.cannedProcesses = const <CannedProcess>[],
}) {
logger ??= Logger.test();
Expand All @@ -78,13 +79,15 @@ class TestEnvironment {
throw UnimplementedError('onRun');
})),
logger: logger,
verbose: verbose,
);
}

factory TestEnvironment.withTestEngine({
bool withRbe = false,
ffi.Abi abi = ffi.Abi.linuxX64,
List<CannedProcess> cannedProcesses = const <CannedProcess>[],
bool verbose = false,
}) {
final io.Directory rootDir = io.Directory.systemTemp.createTempSync('et');
final TestEngine engine = TestEngine.createTemp(rootDir: rootDir);
Expand All @@ -107,8 +110,12 @@ class TestEnvironment {
}
return false;
});
final TestEnvironment testEnvironment = TestEnvironment(engine,
abi: abi, cannedProcesses: cannedProcesses + <CannedProcess>[cannedGn]);
final TestEnvironment testEnvironment = TestEnvironment(
engine,
abi: abi,
cannedProcesses: cannedProcesses + <CannedProcess>[cannedGn],
verbose: verbose,
);
return testEnvironment;
}

Expand Down Expand Up @@ -206,7 +213,8 @@ Matcher containsCommand(CommandMatcher commandMatcher) => (dynamic processes) {
/// command[5] == 'flutter/fml:fml_arc_unittests';
/// })
/// );
Matcher doesNotContainCommand(CommandMatcher commandMatcher) => (dynamic processes) {
Matcher doesNotContainCommand(CommandMatcher commandMatcher) =>
(dynamic processes) {
Expect.type<List<ExecutedProcess>>(processes);
final List<List<String>> commands = (processes as List<ExecutedProcess>)
.map((ExecutedProcess process) => process.command)
Expand Down