diff --git a/engine/src/flutter/tools/engine_tool/lib/src/build_plan.dart b/engine/src/flutter/tools/engine_tool/lib/src/build_plan.dart index ccfbfeb20e..fdd8e636c7 100644 --- a/engine/src/flutter/tools/engine_tool/lib/src/build_plan.dart +++ b/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 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 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 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().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(); } diff --git a/engine/src/flutter/tools/engine_tool/lib/src/logger.dart b/engine/src/flutter/tools/engine_tool/lib/src/logger.dart index 0892487197..411d8d1902 100644 --- a/engine/src/flutter/tools/engine_tool/lib/src/logger.dart +++ b/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); diff --git a/engine/src/flutter/tools/engine_tool/test/build_plan_test.dart b/engine/src/flutter/tools/engine_tool/test/build_plan_test.dart index 529641fa2d..e2875f6335 100644 --- a/engine/src/flutter/tools/engine_tool/test/build_plan_test.dart +++ b/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().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().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().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"')), + ); + }); } diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/build_config_runner.dart b/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/build_config_runner.dart index c15aead301..f96de99426 100644 --- a/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/build_config_runner.dart +++ b/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 _mergedGnArgs = () { - // Put the union of the build config args and extraGnArgs in gnArgs. - final Set gnArgs = Set.of(build.gn); - 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; - }(); + late final Set _mergedGnArgs = Set.of(mergeGnArgs( + buildArgs: build.gn, + extraArgs: extraGnArgs, + )); Future _runGn(RunnerEventHandler eventHandler) async { final String gnPath = p.join(engineSrcDir.path, 'flutter', 'tools', 'gn'); diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/merge_gn_args.dart b/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/merge_gn_args.dart new file mode 100644 index 0000000000..9558be5c1d --- /dev/null +++ b/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 mergeGnArgs({ + required List buildArgs, + required List 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 = {}; + 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"; +} diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/test/build_config_runner_test.dart b/engine/src/flutter/tools/pkg/engine_build_configs/test/build_config_runner_test.dart index 48c00256e3..a090a85fd2 100644 --- a/engine/src/flutter/tools/pkg/engine_build_configs/test/build_config_runner_test.dart +++ b/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; - try { - engine = Engine.findWithin(); - } catch (e) { - io.stderr.writeln(e); - io.exitCode = 1; - return; - } + final engine = Engine.findWithin(); 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'); }); diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/test/merge_gn_args_test.dart b/engine/src/flutter/tools/pkg/engine_build_configs/test/merge_gn_args_test.dart new file mode 100644 index 0000000000..9762678513 --- /dev/null +++ b/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'], + ); + }); +}