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

Closes flutter/flutter#147894.

While doing this PR I realized we were basically passing `(bool verbose, Envrionment)` as a tuple around, so I just moved the concept _into_ `Environment` directly, and made the necessary code changes across the tool and tests.

To clarify, this does _not_ mimic the output of `ninja --verbose` _today_, because we also don't stream the output directly, and instead do terminal magic. Combined with a hypothetical fix for flutter/flutter#147903, this would work exactly the same as before.

/cc @loic-sharma @johnmccutchan

Author: matanlurey

tools/engine_tool/lib/main.dart

@@ -66,13 +66,13 @@ void main(List<String> args) async {
     platform: const LocalPlatform(),
     processRunner: ProcessRunner(),
     logger: Logger(),
+    verbose: verbose,
   );
 
   // Use the Engine and BuildConfig collection to build the CommandRunner.
   final ToolCommandRunner runner = ToolCommandRunner(
     environment: environment,
     configs: configs,
-    verbose: verbose,
     help: help,
   );
 

tools/engine_tool/lib/src/build_utils.dart

@@ -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));
   });
 }
 
@@ -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,
@@ -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;

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

@@ -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(

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

@@ -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;

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

@@ -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>>[
@@ -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(
@@ -64,7 +60,6 @@ final class ToolCommandRunner extends CommandRunner<int> {
       TestCommand(
         environment: environment,
         configs: configs,
-        verbose: verbose,
         help: help,
         usageLineLength: _usageLineLength,
       ),
@@ -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 {

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

@@ -4,7 +4,6 @@
 
 import '../dependencies.dart';
 import 'command.dart';
-import 'flags.dart';
 
 /// The root 'fetch' command.
 final class FetchCommand extends CommandBase {
@@ -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);
   }
 }

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

@@ -15,7 +15,6 @@ final class QueryCommand extends CommandBase {
   QueryCommand({
     required super.environment,
     required this.configs,
-    super.verbose = false,
     super.help = false,
     super.usageLineLength,
   }) {
@@ -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,
     ));
   }
@@ -73,7 +70,6 @@ final class QueryBuildersCommand extends CommandBase {
   QueryBuildersCommand({
     required super.environment,
     required this.configs,
-    super.verbose = false,
     super.help = false,
   });
 
@@ -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',
       );
@@ -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);
@@ -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(

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

@@ -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

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

@@ -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(

tools/engine_tool/lib/src/dependencies.dart

@@ -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();
     }
 
@@ -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();
@@ -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}');
     }
   }

tools/engine_tool/lib/src/environment.dart

@@ -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;
 

tools/engine_tool/test/build_command_test.dart

@@ -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>[

tools/engine_tool/test/utils.dart

@@ -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();
@@ -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);
@@ -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;
   }
 
@@ -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)