Add and use mergeGnArgs with --gn-args from et. (flutter/engine#56228)

Closes https://github.com/flutter/flutter/issues/156909.

This PR adds (and implements) the `--gn-args` (extra command-line GN args) functionality by generalizing on the concept of "merged" GN args that @zanderso had special-cased for `--lto` and `--rbe`, and further testing it.

There is also a logical place for us to expand support of merged arguments at a future point in time.

engine/src/flutter/tools/engine_tool/lib/src/build_plan.dart

@@ -16,6 +16,7 @@ const _flagConcurrency = 'concurrency';
 const _flagStrategy = 'build-strategy';
 const _flagRbe = 'rbe';
 const _flagLto = 'lto';
+const _flagExtraGnArgs = 'gn-args';
 
 /// Describes what (platform, targets) and how (strategy, options) to build.
 ///
@@ -75,6 +76,11 @@ final class BuildPlan {
         }
         throw FatalError('Invalid value for --$_flagConcurrency: $value');
       }(),
+      extraGnArgs: () {
+        final value = args.multiOption(_flagExtraGnArgs);
+        _checkExtraGnArgs(value);
+        return value;
+      }(),
     );
   }
 
@@ -84,7 +90,8 @@ final class BuildPlan {
     required this.useRbe,
     required this.useLto,
     required this.concurrency,
-  }) {
+    required Iterable<String> extraGnArgs,
+  }) : extraGnArgs = List.unmodifiable(extraGnArgs) {
     if (!useRbe && strategy == BuildStrategy.remote) {
       throw FatalError(
         'Cannot use remote builds without RBE enabled.\n\n$_rbeInstructions',
@@ -92,6 +99,54 @@ final class BuildPlan {
     }
   }
 
+  /// Arguments that cannot be provided to [BuildPlan.extraGnArgs].
+  ///
+  /// Instead, provide them explicitly as other [BuildPlan] arguments.
+  @visibleForTesting
+  static const reservedGnArgs = {
+    _flagRbe,
+    _flagLto,
+    'no-$_flagRbe',
+    'no-$_flagLto',
+    // If we are to expand this list to include flags that are not a 1:1 mapping
+    // - for example we want to reserve "--foo-bar" but it's called "--use-baz"
+    // in "et", let's (a) re-think having these arguments named differently and
+    // (b) if necessary, consider changing this set to a map instead so a clear
+    // error can be presented below.
+  };
+
+  /// Error thrown when [reservedGnArgs] are used as [extraGnArgs].
+  @visibleForTesting
+  static final reservedGnArgsError = FatalError(
+    'Flags such as ${reservedGnArgs.join(', ')} should be specified as '
+    'direct arguments to "et" and not using "--gn-args". For example, '
+    '`et build --no-lto` instead of `et build --gn-args="--no-lto"`.',
+  );
+
+  /// Error thrown when a non-flag argument is provided as [extraGnArgs].
+  @visibleForTesting
+  static final argumentsMustBeFlagsError = FatalError(
+    'Arguments provided to --gn-args must be flags (booleans) and be '
+    'specified as either in the format "--flag" or "--no-flag". Options '
+    'that are not flags or are abberviated ("-F") are not currently '
+    'supported; consider filing a request: '
+    'https://fluter.dev/to/engine-tool-bug.',
+  );
+
+  static void _checkExtraGnArgs(Iterable<String> gnArgs) {
+    for (final arg in gnArgs) {
+      if (!arg.startsWith('--') || arg.contains('=') || arg.contains(' ')) {
+        throw argumentsMustBeFlagsError;
+      }
+
+      // Strip off the prefix and compare it to reserved flags.
+      final withoutPrefix = arg.replaceFirst('--', '');
+      if (reservedGnArgs.contains(withoutPrefix)) {
+        throw reservedGnArgsError;
+      }
+    }
+  }
+
   static String _defaultHostDebug() {
     return 'host_debug';
   }
@@ -175,6 +230,18 @@ final class BuildPlan {
       help: 'How many jobs to run in parallel.',
     );
 
+    // Add --gn-args.
+    parser.addMultiOption(
+      _flagExtraGnArgs,
+      help: ''
+          'Additional arguments to provide to "gn".\n'
+          'GN arguments change the parameters of the compiler and invalidate '
+          'the current build, and should be used sparingly. If there is an '
+          'engine build that should be reused and tested on CI prefer adding '
+          'the arguments to "//flutter/ci/builders/local_engine.json".',
+      hide: !environment.verbose,
+    );
+
     return builds;
   }
 
@@ -201,6 +268,13 @@ final class BuildPlan {
   /// Whether to build with LTO (link-time optimization).
   final bool useLto;
 
+  /// Additional GN arguments to use for a build.
+  ///
+  /// By contract, these arguments are always strictly _flags_ (not options),
+  /// and specified as either `--flag`, `-F`, or as the negative variant (such
+  /// as `--no-flag`).
+  final List<String> extraGnArgs;
+
   @override
   bool operator ==(Object other) {
     return other is BuildPlan &&
@@ -208,12 +282,20 @@ final class BuildPlan {
         strategy == other.strategy &&
         useRbe == other.useRbe &&
         useLto == other.useLto &&
-        concurrency == other.concurrency;
+        concurrency == other.concurrency &&
+        const ListEquality<Object?>().equals(extraGnArgs, other.extraGnArgs);
   }
 
   @override
   int get hashCode {
-    return Object.hash(build.name, strategy, useRbe, useLto, concurrency);
+    return Object.hash(
+      build.name,
+      strategy,
+      useRbe,
+      useLto,
+      concurrency,
+      Object.hashAll(extraGnArgs),
+    );
   }
 
   /// Converts this build plan to its equivalent [RbeConfig].
@@ -236,6 +318,7 @@ final class BuildPlan {
     return [
       if (!useRbe) '--no-rbe',
       if (useLto) '--lto' else '--no-lto',
+      ...extraGnArgs,
     ];
   }
 
@@ -248,6 +331,7 @@ final class BuildPlan {
     buffer.writeln('  useRbe: $useRbe');
     buffer.writeln('  strategy: $strategy');
     buffer.writeln('  concurrency: $concurrency');
+    buffer.writeln('  extraGnArgs: $extraGnArgs');
     buffer.write('>');
     return buffer.toString();
   }

engine/src/flutter/tools/engine_tool/lib/src/logger.dart

@@ -367,7 +367,7 @@ class FlutterSpinner extends Spinner {
 }
 
 /// FatalErrors are thrown when a fatal error has occurred.
-class FatalError extends Error {
+final class FatalError extends Error {
   /// Constructs a FatalError with a message.
   FatalError(this._message);
 

engine/src/flutter/tools/engine_tool/test/build_plan_test.dart

@@ -546,6 +546,98 @@ void main() {
     );
   });
 
+  test('can provide a single extra "--gn-args"', () {
+    final testEnv = TestEnvironment.withTestEngine();
+    addTearDown(testEnv.cleanup);
+
+    final testConfig = TestBuilderConfig();
+    testConfig.addBuild(
+      name: 'linux/host_debug',
+      dimension: TestDroneDimension.linux,
+    );
+
+    final parser = ArgParser();
+    final builds = BuildPlan.configureArgParser(
+      parser,
+      testEnv.environment,
+      configs: {
+        'linux_test_config': testConfig.buildConfig(
+          path: 'ci/builders/linux_test_config.json',
+        ),
+      },
+      help: false,
+    );
+
+    final result = BuildPlan.fromArgResults(
+      parser.parse(['--gn-args', '--foo']),
+      testEnv.environment,
+      builds: builds,
+    );
+    expect(result.extraGnArgs, ['--foo']);
+  });
+
+  test('can provide multiple extra "--gn-arg"s', () {
+    final testEnv = TestEnvironment.withTestEngine();
+    addTearDown(testEnv.cleanup);
+
+    final testConfig = TestBuilderConfig();
+    testConfig.addBuild(
+      name: 'linux/host_debug',
+      dimension: TestDroneDimension.linux,
+    );
+
+    final parser = ArgParser();
+    final builds = BuildPlan.configureArgParser(
+      parser,
+      testEnv.environment,
+      configs: {
+        'linux_test_config': testConfig.buildConfig(
+          path: 'ci/builders/linux_test_config.json',
+        ),
+      },
+      help: false,
+    );
+
+    final result = BuildPlan.fromArgResults(
+      parser.parse(['--gn-args', '--foo', '--gn-args', '--bar']),
+      testEnv.environment,
+      builds: builds,
+    );
+    expect(result.extraGnArgs, ['--foo', '--bar']);
+  });
+
+  test('can provide only long-form "--gn-arg"s', () {
+    final testEnv = TestEnvironment.withTestEngine();
+    addTearDown(testEnv.cleanup);
+
+    final testConfig = TestBuilderConfig();
+    testConfig.addBuild(
+      name: 'linux/host_debug',
+      dimension: TestDroneDimension.linux,
+    );
+
+    final parser = ArgParser();
+    final builds = BuildPlan.configureArgParser(
+      parser,
+      testEnv.environment,
+      configs: {
+        'linux_test_config': testConfig.buildConfig(
+          path: 'ci/builders/linux_test_config.json',
+        ),
+      },
+      help: false,
+    );
+
+    expect(
+      () => BuildPlan.fromArgResults(
+        parser.parse(['--gn-args', '-F']),
+        testEnv.environment,
+        builds: builds,
+      ),
+      throwsA(BuildPlan.argumentsMustBeFlagsError),
+    );
+  });
+
   test('build defaults to host_debug', () {
     final testEnv = TestEnvironment.withTestEngine(
       withRbe: true,
@@ -649,6 +741,116 @@ void main() {
     );
   });
 
+  test('build fails if an extra "--gn-args" contains a reserved flag', () {
+    final testEnv = TestEnvironment.withTestEngine();
+    addTearDown(testEnv.cleanup);
+
+    final testConfig = TestBuilderConfig();
+    testConfig.addBuild(
+      name: 'linux/host_debug',
+      dimension: TestDroneDimension.linux,
+    );
+
+    final parser = ArgParser();
+    final builds = BuildPlan.configureArgParser(
+      parser,
+      testEnv.environment,
+      configs: {
+        'linux_test_config': testConfig.buildConfig(
+          path: 'ci/builders/linux_test_config.json',
+        ),
+      },
+      help: false,
+    );
+
+    for (final reserved in BuildPlan.reservedGnArgs) {
+      expect(
+        () => BuildPlan.fromArgResults(
+          parser.parse(['--gn-args', '--$reserved']),
+          testEnv.environment,
+          builds: builds,
+        ),
+        throwsA(isA<FatalError>().having(
+          (e) => e.toString(),
+          'toString()',
+          contains(BuildPlan.reservedGnArgsError.toString()),
+        )),
+      );
+    }
+  });
+
+  test('builds fails if a non-flag (without =) is provided to "--gn-args"', () {
+    final testEnv = TestEnvironment.withTestEngine();
+    addTearDown(testEnv.cleanup);
+
+    final testConfig = TestBuilderConfig();
+    testConfig.addBuild(
+      name: 'linux/host_debug',
+      dimension: TestDroneDimension.linux,
+    );
+
+    final parser = ArgParser();
+    final builds = BuildPlan.configureArgParser(
+      parser,
+      testEnv.environment,
+      configs: {
+        'linux_test_config': testConfig.buildConfig(
+          path: 'ci/builders/linux_test_config.json',
+        ),
+      },
+      help: false,
+    );
+
+    expect(
+      () => BuildPlan.fromArgResults(
+        parser.parse(['--gn-args', '--foo name']),
+        testEnv.environment,
+        builds: builds,
+      ),
+      throwsA(isA<FatalError>().having(
+        (e) => e.toString(),
+        'toString()',
+        contains(BuildPlan.argumentsMustBeFlagsError.toString()),
+      )),
+    );
+  });
+
+  test('builds fails if a non-flag (with =) is provided to "--gn-args"', () {
+    final testEnv = TestEnvironment.withTestEngine();
+    addTearDown(testEnv.cleanup);
+
+    final testConfig = TestBuilderConfig();
+    testConfig.addBuild(
+      name: 'linux/host_debug',
+      dimension: TestDroneDimension.linux,
+    );
+
+    final parser = ArgParser();
+    final builds = BuildPlan.configureArgParser(
+      parser,
+      testEnv.environment,
+      configs: {
+        'linux_test_config': testConfig.buildConfig(
+          path: 'ci/builders/linux_test_config.json',
+        ),
+      },
+      help: false,
+    );
+
+    expect(
+      () => BuildPlan.fromArgResults(
+        parser.parse(['--gn-args', '--foo=name']),
+        testEnv.environment,
+        builds: builds,
+      ),
+      throwsA(isA<FatalError>().having(
+        (e) => e.toString(),
+        'toString()',
+        contains('Arguments provided to --gn-args must be flags'),
+      )),
+    );
+  });
+
   test('build fails if a config not available is requested', () {
     final testEnv = TestEnvironment.withTestEngine();
     addTearDown(testEnv.cleanup);
@@ -1009,4 +1211,62 @@ void main() {
       contains('How to prefer remote or local builds'),
     );
   });
+
+  test('shows --gn-args if verbose', () {
+    final testEnv = TestEnvironment.withTestEngine(
+      verbose: true,
+    );
+    addTearDown(testEnv.cleanup);
+
+    final testConfig = TestBuilderConfig();
+    testConfig.addBuild(
+      name: 'linux/host_debug',
+      dimension: TestDroneDimension.linux,
+    );
+
+    final parser = ArgParser();
+    final _ = BuildPlan.configureArgParser(
+      parser,
+      testEnv.environment,
+      configs: {
+        'linux_test_config': testConfig.buildConfig(
+          path: 'ci/builders/linux_test_config.json',
+        ),
+      },
+      help: true,
+    );
+
+    expect(
+      parser.usage,
+      contains('Additional arguments to provide to "gn"'),
+    );
+  });
+
+  test('hides --gn-args if not verbose', () {
+    final testEnv = TestEnvironment.withTestEngine();
+    addTearDown(testEnv.cleanup);
+
+    final testConfig = TestBuilderConfig();
+    testConfig.addBuild(
+      name: 'linux/host_debug',
+      dimension: TestDroneDimension.linux,
+    );
+
+    final parser = ArgParser();
+    final _ = BuildPlan.configureArgParser(
+      parser,
+      testEnv.environment,
+      configs: {
+        'linux_test_config': testConfig.buildConfig(
+          path: 'ci/builders/linux_test_config.json',
+        ),
+      },
+      help: true,
+    );
+
+    expect(
+      parser.usage,
+      isNot(contains('Additional arguments to provide to "gn"')),
+    );
+  });
 }

engine/src/flutter/tools/pkg/engine_build_configs/lib/src/build_config_runner.dart

@@ -14,6 +14,7 @@ import 'package:platform/platform.dart';
 import 'package:process_runner/process_runner.dart';
 
 import 'build_config.dart';
+import 'merge_gn_args.dart';
 
 /// The base class for events generated by a command.
 sealed class RunnerEvent {
@@ -366,29 +367,11 @@ final class BuildRunner extends Runner {
     return true;
   }
 
-  // GN arguments from the build config that can be overridden by extraGnArgs.
-  static const List<(String, String)> _overridableArgs = <(String, String)>[
-    ('--lto', '--no-lto'),
-    ('--rbe', '--no-rbe'),
-  ];
-
   // extraGnArgs overrides the build config args.
-  late final Set<String> _mergedGnArgs = () {
+  late final Set<String> _mergedGnArgs = Set.of(mergeGnArgs(
-    // Put the union of the build config args and extraGnArgs in gnArgs.
+    buildArgs: build.gn,
-    final Set<String> gnArgs = Set<String>.of(build.gn);
+    extraArgs: extraGnArgs,
-    gnArgs.addAll(extraGnArgs);
+  ));
-
-    // If extraGnArgs contains an arg, remove its opposite from gnArgs.
-    for (final (String, String) arg in _overridableArgs) {
-      if (extraGnArgs.contains(arg.$1)) {
-        gnArgs.remove(arg.$2);
-      }
-      if (extraGnArgs.contains(arg.$2)) {
-        gnArgs.remove(arg.$1);
-      }
-    }
-    return gnArgs;
-  }();
 
   Future<bool> _runGn(RunnerEventHandler eventHandler) async {
     final String gnPath = p.join(engineSrcDir.path, 'flutter', 'tools', 'gn');

engine/src/flutter/tools/pkg/engine_build_configs/lib/src/merge_gn_args.dart

@@ -0,0 +1,125 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+@internal
+library;
+
+import 'package:meta/meta.dart';
+
+/// Merges and returns the result of adding [extraArgs] to original [buildArgs].
+///
+/// A builder, for example `macos/ios_debug` might look like this (truncated):
+/// ```json
+/// {
+///   "gn": [
+///     "--ios",
+///     "--runtime-mode",
+///     "debug",
+///     "--no-stripped",
+///     "--no-lte"
+///   ]
+/// }
+/// ```
+///
+/// Before asking GN to generate a build from these arguments, [extraArgs] are
+/// added by ways of merging; that is, by assuming that arguments provided by
+/// [extraArgs] are strictly boolean flags in the format "--foo" or "--no-foo",
+/// and either are appended or replace an existing argument specified in
+/// [buildArgs].
+///
+/// [extraArgs] that are not boolean arguments are not supported.
+///
+/// ## Example
+///
+/// ```dart
+/// print(mergeGnArgs(
+///   buildArgs: [],
+///   extraArgs: ["--foo"]
+/// ));
+/// ```
+///
+/// ... prints "[--foo]".
+///
+/// ```dart
+/// print(mergeGnArgs(
+///   buildArgs: ["--foo"],
+///   extraArgs: ["--bar"]
+/// ));
+/// ```
+///
+/// ... prints "[--foo, --bar]".
+///
+/// ```dart
+/// print(mergeGnArgs(
+///   buildArgs: ["--foo", "--no-bar", "--baz"],
+///   extraArgs: ["--no-foo", "--bar"]
+/// ));
+/// ```
+///
+/// ... prints "[--no-foo, --bar, --baz]".
+List<String> mergeGnArgs({
+  required List<String> buildArgs,
+  required List<String> extraArgs,
+}) {
+  // Make a copy of buildArgs so replacements can be made.
+  final newBuildArgs = List.of(buildArgs);
+
+  // Index "extraArgs" as map of "flag-without-no" => true/false.
+  final indexedExtra = <String, bool>{};
+  for (final extraArg in extraArgs) {
+    if (!_isFlag(extraArg)) {
+      throw ArgumentError.value(
+        extraArgs,
+        'extraArgs',
+        'Each argument must be in the form "--flag" or "--no-flag".',
+      );
+    }
+    final (name, value) = _extractRawFlag(extraArg);
+    indexedExtra[name] = value;
+  }
+
+  // Iterate over newBuildArgs and replace if applicable.
+  for (var i = 0; i < newBuildArgs.length; i++) {
+    // It is valid to have non-flags (i.e. --runtime-mode=debug) here. Skip.
+    final buildArg = newBuildArgs[i];
+    if (!_isFlag(buildArg)) {
+      continue;
+    }
+
+    // If there is no repalcement value, leave as-is.
+    final (name, value) = _extractRawFlag(buildArg);
+    final replaceWith = indexedExtra.remove(name);
+    if (replaceWith == null) {
+      continue;
+    }
+
+    // Replace (i.e. --foo with --no-foo or --no-foo with --foo).
+    newBuildArgs[i] = _toFlag(name, replaceWith);
+  }
+
+  // Append arguments that were not replaced above.
+  for (final MapEntry(key: name, value: value) in indexedExtra.entries) {
+    newBuildArgs.add(_toFlag(name, value));
+  }
+
+  return newBuildArgs;
+}
+
+bool _isFlag(String arg) {
+  return arg.startsWith('--') && !arg.contains('=') && !arg.contains(' ');
+}
+
+(String, bool) _extractRawFlag(String flagArgument) {
+  var rawFlag = flagArgument.substring(2);
+  var flagValue = true;
+  if (rawFlag.startsWith('no-')) {
+    rawFlag = rawFlag.substring(3);
+    flagValue = false;
+  }
+  return (rawFlag, flagValue);
+}
+
+String _toFlag(String name, bool value) {
+  return "--${!value ? 'no-' : ''}$name";
+}

engine/src/flutter/tools/pkg/engine_build_configs/test/build_config_runner_test.dart

@@ -19,14 +19,7 @@ import 'fixtures.dart' as fixtures;
 
 void main() {
   // Find the engine repo.
-  final Engine engine;
+  final engine = Engine.findWithin();
-  try {
-    engine = Engine.findWithin();
-  } catch (e) {
-    io.stderr.writeln(e);
-    io.exitCode = 1;
-    return;
-  }
 
   final BuilderConfig buildConfig = BuilderConfig.fromJson(
     path: 'linux_test_config',
@@ -249,7 +242,9 @@ void main() {
     expect((events[7] as RunnerResult).okMessage, equals('OK'));
   });
 
-  test('GlobalBuildRunner passes the specified -j when explicitly provided in an RBE build', () async {
+  test(
+      'GlobalBuildRunner passes the specified -j when explicitly provided in an RBE build',
+      () async {
     final Build targetBuild = buildConfig.builds[0];
     final BuildRunner buildRunner = BuildRunner(
       platform: FakePlatform(
@@ -329,7 +324,9 @@ void main() {
     );
   });
 
-  test('GlobalBuildRunner sets RBE_disable_remote when remote builds are disabled', () async {
+  test(
+      'GlobalBuildRunner sets RBE_disable_remote when remote builds are disabled',
+      () async {
     final Build targetBuild = buildConfig.builds[0];
     final BuildRunner buildRunner = BuildRunner(
       platform: FakePlatform(
@@ -369,7 +366,9 @@ void main() {
     );
   });
 
-  test('GlobalBuildRunner sets RBE_exec_strategy when a non-default value is passed in the RbeConfig', () async {
+  test(
+      'GlobalBuildRunner sets RBE_exec_strategy when a non-default value is passed in the RbeConfig',
+      () async {
     final Build targetBuild = buildConfig.builds[0];
     final BuildRunner buildRunner = BuildRunner(
       platform: FakePlatform(
@@ -412,7 +411,9 @@ void main() {
     );
   });
 
-  test('GlobalBuildRunner passes the specified -j when explicitly provided in a non-RBE build', () async {
+  test(
+      'GlobalBuildRunner passes the specified -j when explicitly provided in a non-RBE build',
+      () async {
     final Build targetBuild = buildConfig.builds[0];
     final BuildRunner buildRunner = BuildRunner(
       platform: FakePlatform(
@@ -532,7 +533,8 @@ void main() {
 
   test('fixes gcc paths', () {
     final String outDir = path.join(io.Directory.current.path, 'foo', 'bar');
-    const String error = 'flutter/impeller/renderer/backend/metal/allocator_mtl.h:69:33: error: foobar';
+    const String error =
+        'flutter/impeller/renderer/backend/metal/allocator_mtl.h:69:33: error: foobar';
     final String fixed = BuildRunner.fixGccPaths('../../$error', outDir);
     expect(fixed, './$error');
   });

engine/src/flutter/tools/pkg/engine_build_configs/test/merge_gn_args_test.dart

@@ -0,0 +1,50 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:engine_build_configs/src/merge_gn_args.dart';
+import 'package:test/test.dart';
+
+void main() {
+  test('refuses to merge with arguments that do not start with --', () {
+    expect(
+      () => mergeGnArgs(buildArgs: [], extraArgs: ['foo']),
+      throwsArgumentError,
+    );
+  });
+
+  test('refuses to merge with arguments that contain spaces', () {
+    expect(
+      () => mergeGnArgs(buildArgs: [], extraArgs: ['--foo bar']),
+      throwsArgumentError,
+    );
+  });
+
+  test('refuses to merge with arguments that contain equals', () {
+    expect(
+      () => mergeGnArgs(buildArgs: [], extraArgs: ['--foo=bar']),
+      throwsArgumentError,
+    );
+  });
+
+  test('appends if there are no matching arguments', () {
+    expect(
+      mergeGnArgs(buildArgs: ['--foo'], extraArgs: ['--bar']),
+      ['--foo', '--bar'],
+    );
+  });
+
+  test('replaces --foo with --no-foo', () {
+    expect(
+      mergeGnArgs(buildArgs: ['--foo'], extraArgs: ['--no-foo']),
+      ['--no-foo'],
+    );
+  });
+
+  test('replaces --no-foo with --foo', () {
+    expect(
+      mergeGnArgs(buildArgs: ['--no-foo'], extraArgs: ['--foo']),
+      ['--foo'],
+    );
+  });
+}