Add and use `mergeGnArgs` with `--gn-args` from `et`. #56228

matanlurey · 2024-10-30T02:19:46Z

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.

gaaclarke

lgtm, only things I'd change is fix the name typo and remove some of the duplication to the test

gaaclarke · 2024-10-30T16:01:36Z

tools/engine_tool/lib/src/build_plan.dart

    required this.concurrency,
-  }) {
+    required Iterable<String> extraGnArgs,
+  }) : extaGnArgs = List.unmodifiable(extraGnArgs) {


Suggested change

}) : extaGnArgs = List.unmodifiable(extraGnArgs) {

}) : extraGnArgs = List.unmodifiable(extraGnArgs) {

Done, thank you!

gaaclarke · 2024-10-30T16:05:27Z

tools/engine_tool/lib/src/build_plan.dart

+          'Both --rbe and --lto (and their --no-x variants) should be '
+          'specified as direct arguments to "et" and not using "--gn-args".',


nit: the error message should probably be generated from reservedGnArgs

gaaclarke · 2024-10-30T16:09:50Z

tools/engine_tool/test/build_plan_test.dart

+    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,
+    );


This whole section is duplicated across tests, can we factor it out?

As this PR is a net positive and unblocks the Dart SDK team, let me land as-is and I'll follow-up shortly in flutter/flutter#157870.

gaaclarke · 2024-10-30T16:11:00Z

tools/engine_tool/test/build_plan_test.dart

+    );
+
+    final result = BuildPlan.fromArgResults(
+      parser.parse(['--gn-args', '--foo', '--gn-args', '--bar']),


The arg parser doesn't support this being something like --gn-args --foo --bar --?

It does not.

gaaclarke · 2024-10-30T16:12:30Z

tools/engine_tool/test/build_plan_test.dart

+      throwsA(isA<FatalError>().having(
+        (e) => e.toString(),
+        'toString()',
+        contains('Arguments provided to --gn-args must be'),


nit: pull this text into a variable or don't assert the text so it isn't duplicated here. It's nice when there is a single place people can update the text.

zanderso · 2024-10-30T16:07:29Z

tools/engine_tool/lib/src/build_plan.dart

+          'Arguments provided to --gn-args must be flags (booleans) and be '
+          'specified as either in the format "--flag". Options that are not '


Did you mean to say either --flag or --no-flag here?

Yup! Thanks, done!

zanderso · 2024-10-30T16:16:40Z

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

+bool _isFlag(String arg) {
+  return !arg.contains('=') && !arg.contains(' ') && !arg.startsWith('--');
+}
+
+(String, bool) _extractRawFlag(String flagArgument) {
+  assert(flagArgument.startsWith('--'), 'Must be a valid flag argument.');
+  var rawFlag = flagArgument.substring(2);
+  var flagValue = true;
+  if (rawFlag.startsWith('no-')) {
+    rawFlag = rawFlag.substring(3);
+    flagValue = false;
+  }
+  return (rawFlag, flagValue);
+}


Looks like these were moved to another file, and need to be deleted here.

zanderso · 2024-10-30T16:17:34Z

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

+}
+
+(String, bool) _extractRawFlag(String flagArgument) {
+  assert(_isFlag(flagArgument), 'Must be a valid flag argument.');


et doesn't run with asserts enabled, IIRC, but I think tests are run with asserts enabled? With that setup, is there a test that will hit this assert? Should this be throwing an ArgumentError instead of asserting?

Tests are run, but this assertion is stale - it's handled above. Removed.

gaaclarke

lgtm!

jtmcdole · 2024-10-30T17:48:26Z

tools/engine_tool/lib/src/build_plan.dart

+  ///
+  /// Instead, provide them explicitly as other [BuildPlan] arguments.
+  @visibleForTesting
+  static const reservedGnArgs = {


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

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.

auto-submit · 2024-10-30T20:46:48Z

auto label is removed for flutter/engine/56228, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ter/engine#56228)

…157893) flutter/engine@9295eeb...090c33a 2024-10-30 skia-flutter-autoroll@skia.org Roll Skia from 85b77db25fa3 to 3c62d4a94d78 (1 revision) (flutter/engine#56248) 2024-10-30 skia-flutter-autoroll@skia.org Roll Dart SDK from 4566845d8e30 to 6a8058eef22c (1 revision) (flutter/engine#56246) 2024-10-30 skia-flutter-autoroll@skia.org Roll Skia from f334411b0a08 to 85b77db25fa3 (5 revisions) (flutter/engine#56245) 2024-10-30 matanlurey@users.noreply.github.com Add and use `mergeGnArgs` with `--gn-args` from `et`. (flutter/engine#56228) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

@zanderso

…#56228) Closes flutter#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.

matanlurey added 2 commits October 29, 2024 19:16

Add and use mergeGnArgs with --gn-args from et.

a16ea82

Unformat.

d697dbf

matanlurey requested review from gaaclarke and zanderso October 30, 2024 02:19

gaaclarke reviewed Oct 30, 2024

View reviewed changes

zanderso reviewed Oct 30, 2024

View reviewed changes

Address feedback.

fec4bde

gaaclarke approved these changes Oct 30, 2024

View reviewed changes

matanlurey requested a review from zanderso October 30, 2024 17:39

matanlurey mentioned this pull request Oct 30, 2024

[et] Add support for building Dart SDK from source #55824

Closed

jtmcdole reviewed Oct 30, 2024

View reviewed changes

zanderso approved these changes Oct 30, 2024

View reviewed changes

++

602f24f

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024

++

eb44411

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024

auto-submit bot merged commit 1c488c7 into flutter:main Oct 30, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Oct 30, 2024

Roll Flutter Engine from 9295eeb6d3ce to 1c488c73404a (1 revision) flutter/flutter#157887

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2024

1c488c734 Add and use mergeGnArgs with --gn-args from et. (flut…

02accf0

…ter/engine#56228)

engine-flutter-autoroll mentioned this pull request Oct 30, 2024

Roll Flutter Engine from 9295eeb6d3ce to 381bc1519c0c (2 revisions) flutter/flutter#157889

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2024

1c488c734 Add and use mergeGnArgs with --gn-args from et. (flut…

e0ea0eb

…ter/engine#56228)

engine-flutter-autoroll mentioned this pull request Oct 31, 2024

Roll Flutter Engine from 9295eeb6d3ce to 2cc9c9992ab3 (3 revisions) flutter/flutter#157891

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 31, 2024

1c488c734 Add and use mergeGnArgs with --gn-args from et. (flut…

77a8877

…ter/engine#56228)

engine-flutter-autoroll mentioned this pull request Oct 31, 2024

Roll Flutter Engine from 9295eeb6d3ce to 090c33aeae83 (4 revisions) flutter/flutter#157893

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 31, 2024

1c488c734 Add and use mergeGnArgs with --gn-args from et. (flut…

ab06cd1

…ter/engine#56228)

	}) : extaGnArgs = List.unmodifiable(extraGnArgs) {
	}) : extraGnArgs = List.unmodifiable(extraGnArgs) {

		'Both --rbe and --lto (and their --no-x variants) should be '
		'specified as direct arguments to "et" and not using "--gn-args".',

		'Arguments provided to --gn-args must be flags (booleans) and be '
		'specified as either in the format "--flag". Options that are not '

Add and use mergeGnArgs with --gn-args from et. #56228

Add and use mergeGnArgs with --gn-args from et. #56228

Uh oh!

Conversation

matanlurey commented Oct 30, 2024

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Oct 30, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add and use `mergeGnArgs` with `--gn-args` from `et`. #56228

Add and use `mergeGnArgs` with `--gn-args` from `et`. #56228