Add an initial 'build' command to engine_tool #50681

johnmccutchan · 2024-02-15T05:55:26Z

Adds a 'build' command to et.
Renamed query builds to query builders because that seemed to be more precise(?)

Some extra requests during review:

I've left some questions I'd like answers to at the top of build_command.dart I suspect @zanderso and @loic-sharma can give me (some?) answers.
I suspect I'm holding the FakeProcessManager wrong or there is a better way to write the tests in build_command_test.dart. Pointers to good examples are appreciated.

zanderso · 2024-02-15T06:51:48Z

Thanks for starting on this =)

I hadn't thought this command all the way through, but my vague thought was that it would leverage the library here as much as makes sense, similar to the example code here.

There are kind of two paths for the build command. Down one path it uses the pre-canned builds from the json files, possibly with some options/targets overridden/added. Down the second path, it ignores the pre-canned builds, and you specify everything, falling back on defaults. The second path can still use the GlobalBuildRunner by hand-crafting a GlobalBuild instead of deserializing one from the json files. I'm not sure what that looks like in terms of command line flags for this command.

zanderso · 2024-02-15T06:53:29Z

tools/engine_tool/lib/src/commands/build_command.dart

+// Some wrinkles I'd like to iron out:
+//  - There can be multiple GlobalBuild objects with the same name. How
+//    should we deal with these conflicts? "mac_clang_tidy" and
+//    "mac_host_engine" both define "host_debug".


As far as the CI recipes are concerned, the names don't matter. We can go through the json files and make them unique, and then enforce it in the script here.

zanderso · 2024-02-15T06:54:13Z

tools/engine_tool/lib/src/commands/build_command.dart

+//  - There can be multiple GlobalBuild objects with the same name. How
+//    should we deal with these conflicts? "mac_clang_tidy" and
+//    "mac_host_engine" both define "host_debug".
+//  - Naming: Should BuildConfig be BuilderConfig and GlobalBuild be BuildConfig?


That makes sense to me. I'm not married to any of the names, and happy if we can find better ones.

zanderso · 2024-02-15T06:56:06Z

tools/engine_tool/lib/src/commands/build_command.dart

+//    "mac_host_engine" both define "host_debug".
+//  - Naming: Should BuildConfig be BuilderConfig and GlobalBuild be BuildConfig?
+//  - The BuildConfig database seems to be lacking things I expected:
+//    - There is no "android_debug_*" GlobalBuild in any of the builders that run on mac?


Yeah, the configurations built on CI are a subset of what's useful to build locally. I have some thoughts about that. One is to provide an option to ignore the platform that the build config is asking for. Another is to create some new json files that we only run locally and not on CI. Open to suggestions about what to do.

It could also be a sign that we have too many configurations (or too few on CI)

That being said, the easiest suggestion I think is to have local JSON files.

Another is to create some new json files that we only run locally and not on CI. Open to suggestions about what to do.

I lean strongly towards introducing new builders for local development. The CI builders run targets/tests that aren't necessary for local development, resulting in a slower inner loop. For example: producing engine .zip archives is rarely necessary when developing.

Crank's builders are a fork of the CI builders targeted for local development scenarios: https://github.com/loic-sharma/crank/blob/4b114fed1668a383e1540b2bf4d29dc0b1dc1148/lib/builders.dart#L5-L359

zanderso · 2024-02-15T07:01:41Z

tools/engine_tool/lib/src/commands/build_command.dart

+//  - Naming: Should BuildConfig be BuilderConfig and GlobalBuild be BuildConfig?
+//  - The BuildConfig database seems to be lacking things I expected:
+//    - There is no "android_debug_*" GlobalBuild in any of the builders that run on mac?
+//    - The gn flags specified in the GlobalBuilds don't reference rbe or goma? Should this be a local configuration that the user selects?


The gn script and CI try to enable Goma implicitly, so the flag doesn't show up too much in the CI configs.

The migration to RBE is in-progress, but there are a few builds that have it. It has to be requested explicitly both locally and in CI.

zanderso · 2024-02-15T07:02:36Z

tools/engine_tool/lib/src/commands/build_command.dart

+// TODO(johnmccutchan): List all available build targets and allow the user
+// to specify which one(s) we should build on the cli.
+
+// TODO(johnmccutchan): Can we show a progress indicator like 'running gn...'?


Yeah, I think we can copy some code from flutter_tool for this.

matanlurey · 2024-02-15T17:00:05Z

tools/engine_tool/lib/src/build_utils.dart

+/// returns false.
+Map<String, BuildConfig> filterBuildConfigs(
+    Map<String, BuildConfig> input, ConfigFilter test) {
+  final Map<String, BuildConfig> r = <String, BuildConfig>{};


This is a great place for the inline-for, imo:

return { for (final entry in input.entries) if (test(entry.name, entry.value)) entry.name: entry.value };

matanlurey · 2024-02-15T17:01:16Z

tools/engine_tool/lib/src/build_utils.dart

+
+List<GlobalBuild> filterBuilds(
+    Map<String, BuildConfig> input, BuildFilter test) {
+  final List<GlobalBuild> r = <GlobalBuild>[];


Ditto here, I'd just use the inline-for:
https://dart.dev/language/collections#control-flow-operators

matanlurey · 2024-02-15T17:04:22Z

tools/engine_tool/lib/src/build_utils.dart

+
+/// A function that returns true or false when given a [BuildConfig] and its
+/// name.
+typedef ConfigFilter = bool Function(String name, BuildConfig config);


Personally (here and below) I'd just inline the function definition.

That avoids having to explain these generically "returns true or false" and you can just document how it's used in the (single?) function below.

matanlurey · 2024-02-15T17:04:59Z

tools/engine_tool/lib/src/commands/build_command.dart

+// Some wrinkles I'd like to iron out:
+//  - There can be multiple GlobalBuild objects with the same name. How
+//    should we deal with these conflicts? "mac_clang_tidy" and
+//    "mac_host_engine" both define "host_debug".


matanlurey · 2024-02-15T17:05:11Z

tools/engine_tool/lib/src/commands/build_command.dart

+//  - There can be multiple GlobalBuild objects with the same name. How
+//    should we deal with these conflicts? "mac_clang_tidy" and
+//    "mac_host_engine" both define "host_debug".
+//  - Naming: Should BuildConfig be BuilderConfig and GlobalBuild be BuildConfig?


matanlurey · 2024-02-15T17:05:40Z

tools/engine_tool/lib/src/commands/build_command.dart

+//    "mac_host_engine" both define "host_debug".
+//  - Naming: Should BuildConfig be BuilderConfig and GlobalBuild be BuildConfig?
+//  - The BuildConfig database seems to be lacking things I expected:
+//    - There is no "android_debug_*" GlobalBuild in any of the builders that run on mac?


It could also be a sign that we have too many configurations (or too few on CI)

That being said, the easiest suggestion I think is to have local JSON files.

matanlurey · 2024-02-15T17:08:12Z

tools/engine_tool/lib/src/commands/build_command.dart

+    argParser.addOption(
+      _configFlag,
+      abbr: 'c',
+      defaultsTo: 'host_debug',


Definitely a TODO (unless you feel inspired) but I have the perfect default for you here:

https://github.com/flutter/engine/tree/main/tools/pkg/engine_repo_tools

Specifically:

engine/tools/pkg/engine_repo_tools/lib/engine_repo_tools.dart

Lines 156 to 169 in df0dc1f

/// Returns the most recently modified output target in [outDir].

///

/// If there are no output targets, returns `null`.

Output? latestOutput() {

final List<Output> outputs = this.outputs();

if (outputs.isEmpty) {

return null;

}

outputs.sort((Output a, Output b) {

return b.path.statSync().modified.compareTo(a.path.statSync().modified);

});

return outputs.first;

}

}

You can see a recent example of this being used here (not exactly the same, but close):

engine/testing/scenario_app/bin/android_integration_tests.dart

Lines 44 to 49 in df0dc1f

defaultsTo:

engine?.

outputs().

where((Output o) => basename(o.path.path).startsWith('android_')).

firstOrNull?.

path.path,

Personally I'd rather have a deterministic default.

For Crank, you can set your default builder in a ~/.config/crank/config.json config file.

This config file also lets you add your own custom builders, which is useful if the CI builders don't support your desired scenario like Android from a macOS host. See: https://github.com/loic-sharma/crank#custom-builders

Hardcoding a default for now sgtm. I suspect we'll want a config file with defaults at some point, but that's big enough for a separate PR.

loic-sharma · 2024-02-15T17:32:31Z

tools/engine_tool/lib/src/commands/build_command.dart

+        .runProcess(commandLine, workingDirectory: environment.engine.outDir);
+    if (r.exitCode != 0) {
+      // TODO(johnmccutchan): We probably want to stream the ninja output
+      // so we can see build progress and compilation failures immediately.


This also loses output colors, which are really helpful. You probably want something like:

Future<int> runProcess(String executable, List<String> arguments) async { var process = await Process.start( executable, arguments, runInShell: true, mode: ProcessStartMode.inheritStdio, ); return await process.exitCode; }

This code has been deleted.

loic-sharma · 2024-02-15T17:40:17Z

@johnmccutchan If you haven't already, consider playing with Crank to see what its UX is like: https://github.com/loic-sharma/crank#crank-

If you'd like, I'd be happy to do a quick call to demo it!

johnmccutchan · 2024-02-21T22:42:18Z

Thanks for starting on this =)

I hadn't thought this command all the way through, but my vague thought was that it would leverage the library here as much as makes sense, similar to the example code here.

There are kind of two paths for the build command. Down one path it uses the pre-canned builds from the json files, possibly with some options/targets overridden/added. Down the second path, it ignores the pre-canned builds, and you specify everything, falling back on defaults. The second path can still use the GlobalBuildRunner by hand-crafting a GlobalBuild instead of deserializing one from the json files. I'm not sure what that looks like in terms of command line flags for this command.

I've updated the PR to use GlobalBuildRunner.

tools/engine_tool/lib/src/build_utils.dart

tools/engine_tool/lib/src/commands/build_command.dart

zanderso · 2024-02-22T15:26:16Z

tools/engine_tool/lib/src/commands/build_command.dart

+    argParser.addOption(
+      _configFlag,
+      abbr: 'c',
+      defaultsTo: 'host_debug',


Hardcoding a default for now sgtm. I suspect we'll want a config file with defaults at some point, but that's big enough for a separate PR.

tools/engine_tool/lib/src/commands/build_command.dart

tools/engine_tool/test/build_command_test.dart

matanlurey · 2024-02-23T04:36:31Z

@johnmccutchan Do you still need help with FakeProcess?

I admit that class sucks, I'd be willing to either (a) work with you to improve it or (b) help you with the specific test(s).

Ping me tomorrow!

zanderso

This is great! I'll make some improvements to the logger after this lands that will help with clearing the progress log messages. (Even when disabling newlines and appending '\r', if a message goes over a single line, then the terminal scrolls...)

…ne#50681)

…144041) flutter/engine@b5bebfe...fbc9b88 2024-02-23 [email protected] Roll Skia from 037e08e92598 to 49dd7ed24bec (3 revisions) (flutter/engine#50916) 2024-02-23 [email protected] Add an initial 'build' command to engine_tool (flutter/engine#50681) 2024-02-23 [email protected] Roll Dart SDK from 7f6eccd65255 to 9849ca5fcaec (1 revision) (flutter/engine#50915) 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

johnmccutchan requested review from matanlurey, loic-sharma and zanderso February 15, 2024 05:55

zanderso reviewed Feb 15, 2024

View reviewed changes

matanlurey reviewed Feb 15, 2024

View reviewed changes

loic-sharma reviewed Feb 15, 2024

View reviewed changes

johnmccutchan force-pushed the et_build1 branch from fb87ec8 to 95630e9 Compare February 21, 2024 22:42

johnmccutchan requested review from zanderso, loic-sharma and matanlurey February 21, 2024 22:45

zanderso reviewed Feb 22, 2024

View reviewed changes

johnmccutchan force-pushed the et_build1 branch 3 times, most recently from 7bcee93 to 551edbd Compare February 23, 2024 02:23

Add an initial 'build' command to engine_tool

49caed0

johnmccutchan force-pushed the et_build1 branch from 551edbd to 49caed0 Compare February 23, 2024 02:23

zanderso approved these changes Feb 23, 2024

View reviewed changes

johnmccutchan merged commit 0f21eba into flutter:main Feb 23, 2024
23 checks passed

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

Roll Flutter Engine from b5bebfe43d29 to fbc9b889aee9 (3 revisions) flutter/flutter#144041

Merged

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

0f21eba10 Add an initial 'build' command to engine_tool (flutter/engi…

610265c

…ne#50681)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an initial 'build' command to engine_tool #50681

Add an initial 'build' command to engine_tool #50681

johnmccutchan commented Feb 15, 2024

zanderso commented Feb 15, 2024

zanderso Feb 15, 2024

matanlurey Feb 15, 2024

zanderso Feb 15, 2024

matanlurey Feb 15, 2024

zanderso Feb 15, 2024

matanlurey Feb 15, 2024

loic-sharma Feb 15, 2024 •

edited

Loading

zanderso Feb 15, 2024

zanderso Feb 15, 2024

matanlurey Feb 15, 2024

johnmccutchan Feb 21, 2024

matanlurey Feb 15, 2024

johnmccutchan Feb 21, 2024

matanlurey Feb 15, 2024

matanlurey Feb 15, 2024

matanlurey Feb 15, 2024

matanlurey Feb 15, 2024

matanlurey Feb 15, 2024

loic-sharma Feb 15, 2024 •

edited

Loading

zanderso Feb 22, 2024

loic-sharma Feb 15, 2024

johnmccutchan Feb 21, 2024

loic-sharma commented Feb 15, 2024 •

edited

Loading

johnmccutchan commented Feb 21, 2024

zanderso Feb 22, 2024

matanlurey commented Feb 23, 2024

zanderso left a comment

	/// Returns the most recently modified output target in [outDir].
	///
	/// If there are no output targets, returns `null`.
	Output? latestOutput() {
	final List<Output> outputs = this.outputs();
	if (outputs.isEmpty) {
	return null;
	}
	outputs.sort((Output a, Output b) {
	return b.path.statSync().modified.compareTo(a.path.statSync().modified);
	});
	return outputs.first;
	}
	}

	defaultsTo:
	engine?.
	outputs().
	where((Output o) => basename(o.path.path).startsWith('android_')).
	firstOrNull?.
	path.path,

Add an initial 'build' command to engine_tool #50681

Add an initial 'build' command to engine_tool #50681

Conversation

johnmccutchan commented Feb 15, 2024

zanderso commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma commented Feb 15, 2024 • edited Loading

johnmccutchan commented Feb 21, 2024

Choose a reason for hiding this comment

matanlurey commented Feb 23, 2024

zanderso left a comment

Choose a reason for hiding this comment

loic-sharma Feb 15, 2024 •

edited

Loading

loic-sharma Feb 15, 2024 •

edited

Loading

loic-sharma commented Feb 15, 2024 •

edited

Loading