Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
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
90 changes: 87 additions & 3 deletions tools/engine_tool/lib/src/build_plan.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -75,6 +76,11 @@ final class BuildPlan {
}
throw FatalError('Invalid value for --$_flagConcurrency: $value');
}(),
extraGnArgs: () {
final value = args.multiOption(_flagExtraGnArgs);
_checkExtraGnArgs(value);
return value;
}(),
);
}

Expand All @@ -84,14 +90,63 @@ 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',
);
}
}

/// Arguments that cannot be provided to [BuildPlan.extraGnArgs].
///
/// Instead, provide them explicitly as other [BuildPlan] arguments.
@visibleForTesting
static const reservedGnArgs = {
Copy link
Member

Choose a reason for hiding this comment

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

do we have a map of "bad flag" -> "et flag"? that might help whoever is running et --extraflags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this particular case, the arguments are identical.

I can clarify that in the error message and leave a breadcrumb that if we expand this list to ensure it's clear to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

_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';
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -201,19 +268,34 @@ 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 &&
build.name == other.build.name &&
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].
Expand All @@ -236,6 +318,7 @@ final class BuildPlan {
return [
if (!useRbe) '--no-rbe',
if (useLto) '--lto' else '--no-lto',
...extraGnArgs,
];
}

Expand All @@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion tools/engine_tool/lib/src/logger.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Loading