Android only for now but this avoids rebuilding a bunch of test targets that aren't necessary to do a `flutter run` with a custom engine.
I need to fill in some blanks for other platforms before landing.
Closes https://github.com/flutter/flutter/issues/147666.
This is a large change, I'd be happy to either review synchronously or
change commands one at a time, but I think this is overall the right
approach. I also didn't see any reason to reuse the `BuildRunner` code
for these commands, the flow is basically:
```mermaid
graph LR
A[et targetsOrPatterns] --> B[gn desc --format=json]
B --> C[existing code that runs ninja/workers]
```
Quick summary of changes:
- Introduced a [`Label` and `TargetPattern`][1] type to avoid awkward
string parsing/manipulation all over
- Replaced `gn_utils` (i.e. `targetsFromCommandLine` and friends) with
[an invocation of `gn desc`][2]
- Some tests were brittle in terms of expected output, I mostly left
them alone and wrote TODOs where applicable
Here is the fun part, results (and some manual integration tests):
```sh
# build, previously was 10-15s, is now ~3s
flutter % time et build -c host_debug_unopt_arm64 //flutter/impeller:impeller_unittests
[2024-05-14T19:05:55.163][macos/host_debug_unopt_arm64: GN]: OK
[2024-05-14T19:05:56.119][macos/host_debug_unopt_arm64: RBE startup]: Proxy started successfully.
[macos/host_debug_unopt_arm64: ninja] 0.0% (0/1) Regenerating ninja files[2024-05-14T19:05:57.309][macos/host_debug_unopt_arm64: ninja]: OK
[2024-05-14T19:05:57.573][macos/host_debug_unopt_arm64: RBE shutdown]: Actions completed: 0
$ENGINE/flutter/bin/et build -c host_debug_unopt_arm64 3.20s user 0.87s system 107% cpu 3.776 total
# test, also benefits from speedup, but I didn't time it
flutter % et test -c host_debug_unopt_arm64 //flutter/fml:fml_unittests
[2024-05-14T19:07:01.843][macos/host_debug_unopt_arm64: GN]: OK
[2024-05-14T19:07:02.707][macos/host_debug_unopt_arm64: RBE startup]: Proxy started successfully.
[2024-05-14T19:07:07.400][macos/host_debug_unopt_arm64: ninja]: 100.0% (3/3) LINK ./fml_unittests
[2024-05-14T19:07:07.404][macos/host_debug_unopt_arm64: ninja]: OK
[2024-05-14T19:07:07.748][macos/host_debug_unopt_arm64: RBE shutdown]: Actions completed: 1 (1 racing local)
OKAY: 7s.95ms //flutter/fml:fml_unittests
# query, also benefits from speedup
flutter % time et query targets -c host_debug_unopt_arm64
//flutter/display_list:display_list_benchmarks
//flutter/display_list:display_list_builder_benchmarks
# ... many targets omitted ...
$ENGINE/flutter/bin/et query targets -c host_debug_unopt_arm64 1.27s user 0.18s system 147% cpu 0.978 total
```
In other words, ~5x improvement on `et build` and `et query` is _much_
faster as well.
[1]:
https://github.com/flutter/engine/compare/main...matanlurey:engine:gn-label?expand=1#diff-ac008475ce9e209f4ecf6594b0cd48d12d86a1ffa0aa7d555375ff43b983eb2b
[2]:
https://github.com/flutter/engine/compare/main...matanlurey:engine:gn-label?expand=1#diff-ad116b8504dd0a500bf758a327c7052e5733a2b465d3295e35b74458d483276f
I started on a refactoring of
https://github.com/flutter/flutter/issues/147666, and was frustrated
with the JSON decoding of `gn desc --format=json`, so I used a similar
pattern from my micro-json library (https://pub.dev/packages/jsonut) and
encapsulated it as an extension type.
My favorite is the `JsonObject.map` function which replaces:
```dart
/// Construct a RunTarget from a JSON map.
factory RunTarget.fromJson(Map<String, Object> map) {
final List<String> errors = <String>[];
final String name = stringOfJson(map, _nameKey, errors)!;
final String id = stringOfJson(map, _idKey, errors)!;
final String targetPlatform =
stringOfJson(map, _targetPlatformKey, errors)!;
if (errors.isNotEmpty) {
throw FormatException('Failed to parse RunTarget: ${errors.join('\n')}');
}
return RunTarget._(name, id, targetPlatform);
}
```
... with:
```dart
/// Construct a RunTarget from a JSON map.
factory RunTarget.fromJson(Map<String, Object> map) {
return JsonObject(map).map((JsonObject json) => RunTarget._(
json.string(_nameKey),
json.string(_idKey),
json.string(_targetPlatformKey),
));
}
```
For illustrative purposes:
```sh
$ et build | grep '.*'
```
... should still get line-per-line status updates, but it does not without this patch.
It's hard to write tests because of global state, so I've declined to do so at the moment.
Closes https://github.com/flutter/flutter/issues/147903.
Previously, `Logger.test` was a leaky abstraction that stored a `@visibleForTesting` variable that was sometimes unused. It saved a few lines of code for tests, but doesn't seem necessary, so I swapped it out for a callback.
Closes https://github.com/flutter/flutter/issues/147894.
While doing this PR I realized we were basically passing `(bool verbose, Envrionment)` as a tuple around, so I just moved the concept _into_ `Environment` directly, and made the necessary code changes across the tool and tests.
To clarify, this does _not_ mimic the output of `ninja --verbose` _today_, because we also don't stream the output directly, and instead do terminal magic. Combined with a hypothetical fix for https://github.com/flutter/flutter/issues/147903, this would work exactly the same as before.
/cc @loic-sharma @johnmccutchan
This removes goma most places except for the arguments to the `gn`
script, which are referenced by recipes. This does not remove goma
capabilities from the GN build entirely. That requires changes in the
buildroot which have to be staged after this change.
https://github.com/flutter/engine/pull/52021 introduced a bug where `ci`
build could only be specified when also passing the `--verbose` flag.
This PR fixes the issue so that by default the `help` message will only
show `ci` builds when `--verbose` is passed to `help`, but the `ci`
builds can be specified even without the `--verbose` flag.
Adds a `Matcher` that can be used against a `List<ExecutedProcess>` to check if a command that matches a predicate was executed. Fails if no such command was found.
An alternative would be to take a single regex instead of a closure.
[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
The `Command`s added to a `CommandRunner` don't automatically inherit
the `CommandRunner`'s usage line length limit. This PR does the
necessary plumbing. Help messages look nicer now.
This PR does two things. First, in many of the `ci` builds, LTO is
enabled by default. This is usually not what we want when doing local
builds, so this PR adds an `--lto` flag to the `build` command which is
disabled by default, causing `--no-lto` to be passed to GN. When `--lto`
is passed to the `build` command, `--lto` is passed to GN, which results
in the build using LTO.
Second, this PR eagerly parses the `--verbose` flag out of the command
line so that help messages can optionally show less information. In
particular, in this PR, `ci` and `web_test` builds are only displayed by
`help build` when `--verbose` is passed to the `help` command. There's
some extra text in the help message as well indicating that passing
`--verbose` to `help` will show more builds.
Addresses the notes I left on
https://github.com/flutter/engine/pull/51868. Mainly fixes an issue in
which the `build`, `query`, and `test` commands would fail if GN had not
yet been run for the requested configuration. Instead, in this PR, if
the build output directory doesn't exist yet, then it will run GN before
querying the targets.
Additionally, for the `build` command, if no targets are specified on
the command line, the PR uses the targets in the build config as the
default rather than all targets. `query` and `test` keep the same
behavior.
- s/TestTarget/BuildTarget.
- Use a more informative way of querying for build targets from gn
- Port existing code to use new interfaces
- Replace 'query tests' with 'query targets' and a --testonly flag
- Extend 'et build' with support for build target selectors.
- Extend 'et query targets' with support for build target selectors.
- Default to dumping out lint logs (can be disabled with `--quiet`
flag).
- Add Logger.fatal which logs an error and throws a FatalError which is
caught in main.
- Simplify `findDartBinDirectory` implementation.
- Make JSON serialized process artifacts more human readable.
- Invokes lints for dart, python, c, and java.
- Captures all output of executions into an artifacts file (a json
file).
- Runs lints concurrently.
- Cool status display while running.
- Tests.
Allow users to update dependencies. Examples:
* `et fetch` Fetch dependencies
In the future, `et build` will update dependencies if it detects that they have changed.
Also:
* Updates the status in the README
* Adds instructions on how to run tests
* Fixes `et run`'s description
* Makes the `--verbose` flag global
The `run` command builds both the host and target engines and then invokes `flutter run` so that it runs the app using the custom engine builds.
It is expected that 'et run' be used inside the directory of a flutter application.
Command line flags passed after `--` will be forwarded to `flutter run`.
Some examples:
- `et run` Build the debug variant and runs the app in that mode.
- `et run -- --profile` Build the profile variant and runs the app in that mode.
- `et run -- --release` Build the release variant and runs the app in that mode.
Also:
- Start a local_engine.json builder definition (it is missing a lot)
- Tweak the test fixture.
- Add a new Matcher to litetest.
- Tweaked the build config linter to only care about duplicate build names within the same builder.
* Adds a 'build' command to `et`.
* Renamed `query builds` to `query builders` because that seemed to be
more precise(?)
Some extra requests during review:
1. 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.
2. 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.
Towards https://github.com/flutter/flutter/issues/132807.
This is needed so the engine tool can map the names of CI builders
listed in `.ci.yaml` to the names of the configurations in the build
config json files in `ci/builders`.
This is mostly just trying to get some structure in place. I suspect
that the implementation of the `query` command in this PR will get
entirely replaced through improvements.
