[et] Lookup output filesystem path, not label #52248

cbracken · 2024-04-19T05:43:51Z

Sets BuildTarget.executable to the root_out_dir-relative path of the executable (e.g. displaylist_unittests) instead of its label (e.g. //out/host_debug/displaylist_unittests). This is required since, in the lines following the output lookup, we assume it to be a path relative to the build output directory.

Also breaks out functions for:

_runGnDesc: returns the JSON output of gn desc buildDir target
_runGnOutputs: returns the output files listed by gn outputs buildDir target

Noticed while working on: flutter/flutter#147071

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Sets BuildTarget.executable to the `root_out_dir`-relative path of the executable (e.g. `displaylist_unittests`) instead of its label (e.g. `//out/host_debug/displaylist_unittests`). This is required since, in the lines following the output lookup, we assume it to be a path relative to the build output directory. Also breaks out functions for: * `_gnDesc`: returns the JSON output of `gn desc buildDir target` * `_gnOutputs`: returns the output files listed by `gn outputs buildDir target` Noticed while working on: flutter/flutter#147071

cbracken · 2024-04-21T17:59:11Z

tools/engine_tool/test/build_command_test.dart

-      expect(testEnv.processHistory.length, greaterThanOrEqualTo(2));
-      expect(testEnv.processHistory[3].command[0], contains('ninja'));
-      expect(testEnv.processHistory[3].command[2], endsWith('/host_debug'));
+      expect(testEnv.processHistory.length, greaterThan(6));


Changed to a strictly greater-than check so that it matches the processHistory index we refer to, which will make it a little more obvious to people who modify this in the future.

cbracken · 2024-04-21T18:05:59Z

tools/engine_tool/test/test_command_test.dart

@@ -83,7 +85,7 @@ void main() {
        '//third_party/protobuf:protoc',
      ]);
      expect(result, equals(1));
-      expect(testEnvironment.processHistory.length, lessThan(3));
+      expect(testEnvironment.processHistory.length, lessThan(6));


Here and in the test above it: This expectation is impacted by implementation details of the test command. I don't think the number of processes executed is the right test, and instead we should just test that test processes are executed and that non-test processes are executed.

I can send out a followup that fixes that. (I realise I also just added this test in a previous patch so I'm 100% to blame for this one :P)

Yes, I think this count is too brittle. Better to have something like:

expect(testEnvironment.processHistory, executed('gn desc'));

johnmccutchan · 2024-04-22T17:47:11Z

tools/engine_tool/test/test_command_test.dart

@@ -83,7 +85,7 @@ void main() {
        '//third_party/protobuf:protoc',
      ]);
      expect(result, equals(1));
-      expect(testEnvironment.processHistory.length, lessThan(3));
+      expect(testEnvironment.processHistory.length, lessThan(6));


Yes, I think this count is too brittle. Better to have something like:

expect(testEnvironment.processHistory, executed('gn desc'));

…ne#52248)

…147190) flutter/engine@a4b71f0...33c828f 2024-04-22 [email protected] Roll Skia from eb29b46236e6 to 3b32e3280b67 (1 revision) (flutter/engine#52298) 2024-04-22 [email protected] Break dependency cycle of FlutterViewController <-> FlutterPlatformView (flutter/engine#52271) 2024-04-22 [email protected] [et] Lookup output filesystem path, not label (flutter/engine#52248) 2024-04-22 [email protected] Roll Skia from 975859a96f8f to eb29b46236e6 (9 revisions) (flutter/engine#52297) 2024-04-22 [email protected] [canvaskit] Add configuration for maximum canvases (flutter/engine#51735) 2024-04-22 [email protected] Fix link in BlendMode.saturation (flutter/engine#52156) 2024-04-22 [email protected] Refactor and migrate FlutterUndoManagerPlugin to ARC (flutter/engine#52234) 2024-04-22 [email protected] Roll Fuchsia Linux SDK from Rr9lFiKCPhMXDGa89... to L533ubzhjWwW7jxbc... (flutter/engine#52293) 2024-04-22 [email protected] Roll Dart SDK from 0a83dd7e61b1 to 0ed66a4d77cb (1 revision) (flutter/engine#52294) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from Rr9lFiKCPhMX to L533ubzhjWwW 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

cbracken force-pushed the et-output-path branch from 2f83a74 to 502cf97 Compare April 20, 2024 17:05

cbracken marked this pull request as ready for review April 20, 2024 17:05

cbracken requested review from johnmccutchan and zanderso April 20, 2024 17:05

cbracken force-pushed the et-output-path branch 12 times, most recently from b2ad2e8 to f5fde03 Compare April 20, 2024 21:39

cbracken force-pushed the et-output-path branch from f5fde03 to 51f7612 Compare April 21, 2024 00:52

cbracken commented Apr 21, 2024

View reviewed changes

johnmccutchan approved these changes Apr 22, 2024

View reviewed changes

cbracken merged commit 219ec84 into flutter:main Apr 22, 2024
26 checks passed

cbracken deleted the et-output-path branch April 22, 2024 18:13

engine-flutter-autoroll mentioned this pull request Apr 22, 2024

Roll Flutter Engine from a4b71f02a1c7 to bb4c58dcf718 (8 revisions) flutter/flutter#147188

Closed

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

219ec843a [et] Lookup output filesystem path, not label (flutter/engi…

30d9e89

…ne#52248)

engine-flutter-autoroll mentioned this pull request Apr 22, 2024

Roll Flutter Engine from a4b71f02a1c7 to 33c828fb3ff5 (9 revisions) flutter/flutter#147190

Merged

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

219ec843a [et] Lookup output filesystem path, not label (flutter/engi…

f9b354a

…ne#52248)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[et] Lookup output filesystem path, not label #52248

[et] Lookup output filesystem path, not label #52248

Uh oh!

cbracken commented Apr 19, 2024 •

edited

Loading

Uh oh!

cbracken Apr 21, 2024

Uh oh!

cbracken Apr 21, 2024

Uh oh!

johnmccutchan Apr 22, 2024

Uh oh!

johnmccutchan Apr 22, 2024

Uh oh!

Uh oh!

Uh oh!

[et] Lookup output filesystem path, not label #52248

[et] Lookup output filesystem path, not label #52248

Uh oh!

Conversation

cbracken commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

cbracken Apr 21, 2024

Choose a reason for hiding this comment

Uh oh!

cbracken Apr 21, 2024

Choose a reason for hiding this comment

Uh oh!

johnmccutchan Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

johnmccutchan Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cbracken commented Apr 19, 2024 •

edited

Loading