fml::scoped_nsobject and fml::scoped_nsprotocol classes are no longer
necessary now that the entire codebase has been migrated to ARC.
This also eliminates the fml_arc_unittests target which tests that code.
All references to that target in documentation, run_tests.py have been
removed. This target was used in various `et` build/query unit tests and
has been replaced with the fml_unittests target in those cases.
Issue: https://github.com/flutter/flutter/issues/137801
Closes https://github.com/flutter/flutter/issues/157945.
```sh
% et cleanup --dry-run
Checking /Users/matanl/Developer/engine/src/out...
The following directories were accessed later than 2024-10-02:
android_debug
android_debug_arm64
android_debug_unopt
android_debug_unopt_arm64_ide
host_debug_unopt_arm64_ide
ios_debug
ios_debug_sim_unopt_arm64
ios_debug_unopt
ios_debug_unopt_arm64
license_script_output
wasm_release
web_tests
Run without --dry-run to reclaim 2.79gb.
```
```sh
flutter % et cleanup
Checking /Users/matanl/Developer/engine/src/out...
Deleted 12 output directories and reclaimed 2.79gb.
```
Closes https://github.com/flutter/flutter/issues/157870.
Looks a lot better - I even noticed a few places where I was doing
unnecessary (copy/pasted) configuration.
> `1 file changed, 240 insertions(+), 768 deletions(-)`
Closes https://github.com/flutter/flutter/issues/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.
Closes https://github.com/flutter/flutter/issues/156260.
Fixes the degenerate case where you specify an unrecognized target, and it falls back to rebuilding the entire engine as if you specified nothing. In addition, added recognition of `group`.
Closes https://github.com/flutter/flutter/issues/148444 (code de-duplicated).
Closes https://github.com/flutter/flutter/issues/150877 (`--build-strategy=local`).
Closes https://github.com/flutter/flutter/issues/150884 (`--build-strategy=remote`).
Replaces duplicate code across ~3 commands (query, test, build) with a class called `BuildPlan`, which encapsulates (a) concurrency, (b) build configuration, (c) RBE, (d) LTO, and (e) build strategy. I also moved all of the validation of the build plan into `build_plan_test`, which gives us better coverage at a lower cost (less things to think about in that suite of tests).
I know the diff looks scary, but 1K of the 1.4K is tests.
/cc @gaaclarke @flar
`build`, `test`, and `run` commands all support the `--exec-strategy`
flag ('auto', 'local', 'remote').
This flag controls the RBE task execution strategy (when RBE is
enabled).
Closes#150877
Work towards https://github.com/flutter/flutter/issues/147013.
I think I still need applicable metadata about the output, and then make
sure `et test` understands everything:
```sh
% et test //flutter/tools/engine_tool/...
[2024-10-03 13:26:42.657137] ERROR: No test targets found
```
Does a few things:
- Refactors `run_command_test` significantly to reduce global fixtures
- Replaced stringly-typed things with enum-like objects
- Adds a lot stronger coverage for `run_command` to make future refactors safer
- Takes advantage of `package:test` having a workable matchers system and uses it
- Changes `return 1` into `throw FatalError(...)` where it makes sense in `run_command`
As a result of the refactoring work, I also fixed a bug: Closes https://github.com/flutter/flutter/issues/147646.
The logical "part 2.5" of the work started in https://github.com/flutter/engine/pull/55540.
Test output does not change, the tests just no longer rely on the global `fixtures.dart` data.
The logical "part 2" of the work started in https://github.com/flutter/engine/pull/55540.
Test output does not change, the tests just no longer rely on the global `fixtures.dart` data.
Partial work towards https://github.com/flutter/flutter/issues/148420, unblocks https://github.com/flutter/engine/pull/55537.
These tests can now be more precise, and changing the fixtures no longer has side-effects on tests across the entire repository. There are about 11 other usages (there were ~50 in these) after this PR that I'll get to, as well as the hard-coded `gn desc` output, before retiring `fixtures.dart`
Closes https://github.com/flutter/flutter/issues/155769.
This is a variant of the approach in https://github.com/flutter/engine/pull/52241, based on feedback from @jakemac53 and @jonahwilliams (who originally sped up `dart test` significantly by using `frontend_server` under the scenes: https://github.com/dart-lang/test/pull/1399), in short:
```gn
# tools/engine_tool/BUILD.gn
import("//flutter/build/dart/internal/gen_dartcli_call.gni")
gen_dartcli_call("tests") {
args = [ "test" ]
cwd = "//flutter/tools/engine_tool"
}
```
This stanza, when built (`ninja -C ../out/host_debug flutter/tools/engine_tool:tests`) generates the following file:
```sh
# ../out/host_debug/gen/flutter/tools/engine_tool/tests
set -e
# Needed because if it is set, cd may print the path it changed to.
unset CDPATH
# Store the current working directory.
prev_cwd=$(pwd)
# Set a trap to restore the working directory.
trap 'cd "$prev_cwd"' EXIT
CD_PATH="/Users/matanl/Developer/engine/src/flutter/tools/engine_tool"
if [ -n "$CD_PATH" ]; then
cd "$CD_PATH"
fi
/Users/matanl/Developer/engine/src/flutter/prebuilts/macos-arm64/dart-sdk/bin/dart "test"
```
In turn, when executed (`../out/host_debug/gen/flutter/tools/engine_tool/tests`) it just runs `dart test`:
```sh
flutter % ../out/host_debug/gen/flutter/tools/engine_tool/tests
Building package executable...
Built test:test.
00:00 +0: test/test_command_test.dart: test command executes test
00:00 +3: test/run_command_test.dart: run command invokes flutter run
...
00:00 +117: All tests passed!
```
There is no actual compilation performed by the tool (that is handled implicitly by `dart test`), and as a result neither a `depfile` is needed, nor generating a pre-compiled artifact like a snapshot or AOT elf/assembly.
---
This work is incomplete, that is, we'd want to properly tag the executable so `et` can find it, and create a wrapper template (i.e. `dart_test`) that tightens things up a bit, but I wanted to show the work at this intermediate step to get feedback before moving forward.
/cc @jonahwilliams, @jtmcdole as well.
There are almost no behavioral changes.
Because `dart test` runs with assertions, and the former command does not, I had to tweak some of the engine tool tests because they would assert that there were duplicate names - but I didn't change any actual code besides the test expectations themselves/the fixtures.
This is not all of the engine, but is approximately 1/3 of all imports of `package:litetest` migrated.
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.
