Adds a runner to engine_build_configs #50342

zanderso · 2024-02-04T21:05:52Z

This PR adds an API to the engine_build_configs package that executes an engine build config locally . This API is a building block for an eventual command line tool to streamline workflows in the engine repo. In particular, it will allow the command line tool to execute the same config, build, and test commands that are run on CI.

flutter-dashboard · 2024-02-05T16:13:58Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #50342 at sha 6e9417b

zanderso · 2024-02-09T16:31:43Z

Friendly ping. Happy to chat about this or split it up into smaller pieces if that would be helpful.

loic-sharma · 2024-02-09T19:34:14Z

tools/pkg/engine_build_configs/bin/run.dart

+// For example:
+// $ dart bin/run.dart mac_unopt host_debug_unopt
+//
+// The build config names are the namess of the json files under ci/builders


Suggested change

// The build config names are the namess of the json files under ci/builders

// The build config names are the names of the json files under ci/builders

loic-sharma · 2024-02-09T19:35:00Z

tools/pkg/engine_build_configs/bin/run.dart

+// $ dart bin/run.dart mac_unopt host_debug_unopt
+//
+// The build config names are the namess of the json files under ci/builders
+// The build names are the "name" fields of the maps in the list of "builds".


This is good info, could we also add it to a README.md in //flutter/tools/pkg/engine_build_configs/ for improved readability?

I don't feel strongly about this, feel free to skip

Good idea. Added a light README.md.

loic-sharma · 2024-02-09T19:37:28Z

tools/pkg/engine_build_configs/bin/run.dart

+    configName = args[0];
+    buildName = args[1];
+  } else {
+    io.stderr.writeln('Wrong arguments');


Can we make this error message actionable? Perhaps something like:

Suggested change

io.stderr.writeln('Wrong arguments');

io.stderr.writeln('''

Usage:

$ dart bin/run.dart [build config name] [build name]

For example:

$ dart bin/run.dart mac_unopt host_debug_unopt

The build config names are the names of the json files under ci/builders

''');

johnmccutchan · 2024-02-09T20:31:05Z

tools/pkg/engine_build_configs/bin/run.dart

+  }
+
+  // Check the parsed build configs for validity.
+  BuildConfig? targetConfig;


replace the loop below with:

targetConfig = configs[name];

Thanks. Done.

loic-sharma · 2024-02-09T22:47:48Z

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

+    for (final String dimension in droneDimensions) {
+      os ??= switch (dimension.split('=')) {
+        ['os', 'Linux'] => Platform.linux,
+        ['os', 'Windows'] => Platform.windows,


It looks like we'll need to support Windows-10 and Windows-11. Example:

engine/ci/builders/windows_host_engine.json

Line 22 in 41daa5b

"os=Windows-10"

Oh, good catch. Done.

loic-sharma · 2024-02-09T22:55:45Z

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

+    }
+
+    if (runGn) {
+      if (!await _runGN(eventHandler)) {


Nit: consider consistent casing _runGn to align with runGn

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

loic-sharma · 2024-02-09T23:03:59Z

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

+  String toString() {
+    final String ts = '[${_timestamp(timestamp)}]';
+    final String pct = '${percent.toStringAsFixed(1)}%';
+    return '$ts[$name]: $pct $what';


The percentages feel slower than ninja's <completed>/<total> output. My computer might display 68.9% for a while even though it's actually completed several steps.

Could you expand on why you chose percentages instead of raw numbers?

Didn't think about this too deeply, but it does make sense for the progress object to include the number of completed steps and the total number of steps so that the client of the API can do whatever it likes with them, so I added them.

loic-sharma

This is awesome, excellent work!

…143265) flutter/engine@e08b6c8...c587bd6 2024-02-10 [email protected] Adds a runner to engine_build_configs (flutter/engine#50342) 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 [email protected],[email protected],[email protected] 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 · 2024-02-20T16:06:19Z

xref flutter/flutter#132807

zanderso marked this pull request as draft February 4, 2024 21:06

zanderso force-pushed the build-config-runner branch from 6cb8a50 to 6e9417b Compare February 5, 2024 04:57

zanderso marked this pull request as ready for review February 5, 2024 16:12

zanderso requested review from johnmccutchan, loic-sharma and matanlurey February 5, 2024 16:12

flutter-dashboard bot added the will affect goldens label Feb 5, 2024

loic-sharma reviewed Feb 9, 2024

View reviewed changes

johnmccutchan reviewed Feb 9, 2024

View reviewed changes

johnmccutchan approved these changes Feb 9, 2024

View reviewed changes

loic-sharma reviewed Feb 9, 2024

View reviewed changes

tools/pkg/engine_build_configs/lib/src/build_config_runner.dart Show resolved Hide resolved

loic-sharma reviewed Feb 9, 2024

View reviewed changes

loic-sharma approved these changes Feb 9, 2024

View reviewed changes

Adds a runner to engine_build_configs

d448d7b

zanderso force-pushed the build-config-runner branch from 6e9417b to 9adbbee Compare February 9, 2024 23:37

Review feedback

f06a6f0

zanderso force-pushed the build-config-runner branch from 9adbbee to f06a6f0 Compare February 10, 2024 01:02

zanderso merged commit c587bd6 into flutter:main Feb 10, 2024

zanderso deleted the build-config-runner branch February 10, 2024 02:42

engine-flutter-autoroll mentioned this pull request Feb 10, 2024

Roll Flutter Engine from e08b6c899c42 to c587bd69985f (1 revision) flutter/flutter#143265

Merged

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

c587bd699 Adds a runner to engine_build_configs (flutter/engine#50342)

565d685

	// The build config names are the namess of the json files under ci/builders
	// The build config names are the names of the json files under ci/builders

Adds a runner to engine_build_configs #50342

Adds a runner to engine_build_configs #50342

Uh oh!

Conversation

zanderso commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Feb 5, 2024

Uh oh!

zanderso commented Feb 9, 2024

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!

loic-sharma Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Feb 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zanderso commented Feb 4, 2024 •

edited

Loading

loic-sharma Feb 9, 2024 •

edited

Loading

zanderso Feb 9, 2024 •

edited

Loading