[fuchsia] Create CF v2 Flutter runner. #29142

akbiggs · 2021-10-12T21:21:55Z

Bug: fxb/50694
Tested: Ran V1 runner with Spinning Square example to verify that it doesn't crash. Still working on porting an existing Flutter component over to V2 to test the new runner. Unit tests will be created in a follow-up PR since I'm still not certain I'm doing things right in the V2 runner.

This PR was optimized for two things:

Minimizing the risk of breaking the existing CF v1 runner. The only change to the CF v1 runner is renaming Component -> ComponentV1. No logic for the V2 runner is shared with the V1 runner (with the exception of the FileInNamespaceBuffer utility).
Minimizing the diff between the CF v1 runner and CF v2 runner for easy comparison. See the component.h diff and the component.cc diff to more easily see what's changed in the V2 runner.

chaselatta · 2021-10-12T21:34:36Z

shell/platform/fuchsia/dart_runner/dart_component_controller_v2.cc

@@ -354,6 +354,13 @@ void DartComponentControllerV2::Run() {
  loop_->Run();

  if (binding_.is_bound()) {
+    // TODO(fxb/79871): This is likely a bug. We're taking the return_code


yeah, v2 doesn't have a good way to communicate the return code like a v1 component does. This is something we will need for other things outside of the runner so we should work with the CF team to figure out a plan. I think there will likely be some other lifecycle type service we need to start interacting with.

In the meantime. Can we just do a ZX_ERR_INTERNAL if the return code is not zero and ZX_OK otherwise?

Sounds good, will handle in a follow-up PR.

This is a feature request that has come up before, it and sounds reasonable. We could probably have the ComponentController dispatch a FIDL event with the return code. In the meantime, converting to INTERNAL SGTM.

Opened fxb/86666 for handling that in CF. Thanks for the feedback Gary.

akbiggs · 2021-10-13T18:40:18Z

@gebressler @ypomortsev Thanks for reviewing our CF v2 port!

Some additional context on this PR:

The V2 ComponentController is forked from our existing V1 ComponentController, which is why the diff in this PR is large. The main description for the PR has diffs between the V1 and V2 .h and .cc files which are easier to digest.
The new ComponentController is mostly the same as V1, but replacing package+LaunchStartInfo with ComponentStartInfo, and closing bindings with an epitaph.
There are going to be a lot of follow-up PRs, mostly documented as TODOs here.
CML files for the runners were added as a previous commit in the adjacent meta/ directory. [fuchsia] CML files for Flutter runner CF v2. #28982
I can't add you as reviewers directly because you're not part of the Flutter org. Happy to fix that but it requires messaging around on Discord a bit, so I'm just messaging you directly on PRs that require your review for now.

arbreng

LGTM, but be extra cautious to test on smart_display and workstation due to the runner.cc changes

shell/platform/fuchsia/flutter/component_v2.cc

shell/platform/fuchsia/flutter/component_v2.h

gebressler · 2021-10-14T17:34:04Z

LGTM from me

…

On Thu, Oct 14, 2021 at 10:22 AM Alexander Biggs ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In shell/platform/fuchsia/flutter/component_v2.cc <#29142 (comment)>: > + FML_DCHECK(ns_fd.is_valid()); + + constexpr mode_t mode = O_RDONLY | O_DIRECTORY; + + component_assets_directory_.reset( + openat(ns_fd.get(), assets_path.c_str(), mode)); + FML_DCHECK(component_assets_directory_.is_valid()); + + component_data_directory_.reset( + openat(ns_fd.get(), data_path.c_str(), mode)); + FML_DCHECK(component_data_directory_.is_valid()); + } + + // ComponentStartInfo::runtime_dir (optional). + if (start_info.has_runtime_dir()) { + runtime_dir_->Serve(fuchsia::io::OPEN_RIGHT_READABLE | Tested with a Flutter CF v2 component this morning and this part is crashing. https://paste.googleplex.com/6433282938896384 It seems like maybe it doesn't like me trying to Serve this as a directory based on this line: [02662.474881][1924][2978][klog] INFO: #0 0x000001814b84b8f8 in vfs::internal::Node::IsDirectory(const vfs::internal::Node*) ../../fuchsia/sdk/linux/pkg/vfs_cpp/include/lib/vfs/cpp/internal/node.h:99 <<VMO#641510=blob-a84f3dfd>>+0x4d98f8 sp 0xa34f7f8cf0 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29142 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQB2JV4B56ZY4F2YEH5FBETUG4GVHANCNFSM5F3T27EA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

chinmaygarde · 2021-10-14T20:05:55Z

@arbreng Should I add the CQ tag? This looks good to go.

arbreng · 2021-10-14T20:08:05Z

@arbreng Should I add the CQ tag? This looks good to go.

IIUC @akbiggs is still stress-testing this before landing it. So not good-to-go but he will land it once everything is working.

ypomortsev

LGTM w/ comments!

shell/platform/fuchsia/flutter/component_v2.cc

Also ignores: flutter#91906 2021-10-15 [email protected] [web] Add goldctl as a dependency in LUCI (flutter/engine#29168) 2021-10-15 [email protected] Revert "Set system bar appearance using WindowInsetsControllerCompat instead of the deprecated View#setSystemUiVisibility (flutter#29060)" (flutter/engine#29206) 2021-10-15 [email protected] Roll Dart SDK from e8c02a935741 to 42acd2ae8fa8 (1 revision) (flutter/engine#29205) 2021-10-15 [email protected] Roll Dart SDK from 9f3cd7a49814 to e8c02a935741 (1 revision) (flutter/engine#29204) 2021-10-15 [email protected] Roll Skia from 012f7146067a to b24bad31dc05 (3 revisions) (flutter/engine#29203) 2021-10-15 [email protected] Roll Skia from 72602b668e22 to 012f7146067a (1 revision) (flutter/engine#29202) 2021-10-15 [email protected] Roll Dart SDK from aaca2ac128ae to 9f3cd7a49814 (1 revision) (flutter/engine#29201) 2021-10-15 [email protected] Set the use_ios_simulator flag only on platforms where it is defined (iOS/Mac) (flutter/engine#29199) 2021-10-14 [email protected] [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142) 2021-10-14 [email protected] Roll Dart SDK from 82b0281cbcf3 to aaca2ac128ae (1 revision) (flutter/engine#29198) 2021-10-14 [email protected] Roll Skia from aa9656d8caa6 to 72602b668e22 (1 revision) (flutter/engine#29196) 2021-10-14 [email protected] Ignore implicit_dynamic_function analyzer error for js_util generic methods (flutter/engine#29192) 2021-10-14 [email protected] [web] use 'dart compile js' instead of 'dart2js' in web_ui and felt (flutter/engine#29179) 2021-10-14 [email protected] Roll Dart SDK from 081a57c06088 to 82b0281cbcf3 (3 revisions) (flutter/engine#29195) 2021-10-14 [email protected] Roll Skia from d0c7f636453b to aa9656d8caa6 (3 revisions) (flutter/engine#29194) 2021-10-14 [email protected] Set system bar appearance using WindowInsetsControllerCompat instead of the deprecated View#setSystemUiVisibility (flutter/engine#29060) 2021-10-14 [email protected] [UWP] Remove 1px offset to make root widget fully shown (flutter/engine#27922) 2021-10-14 [email protected] Roll Skia from ab19daec3b88 to d0c7f636453b (1 revision) (flutter/engine#29191) 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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

akbiggs requested review from chaselatta and arbreng October 12, 2021 21:21

flutter-dashboard bot added the platform-fuchsia label Oct 12, 2021

google-cla bot added the cla: yes label Oct 12, 2021

[fuchsia] Create CF v2 Flutter runner.

45a11a7

akbiggs force-pushed the flutter-runner-v2-alex-is-bad-at-git branch from af5da45 to 45a11a7 Compare October 12, 2021 21:28

chaselatta reviewed Oct 12, 2021

View reviewed changes

arbreng approved these changes Oct 13, 2021

View reviewed changes

akbiggs commented Oct 14, 2021

View reviewed changes

shell/platform/fuchsia/flutter/component_v2.cc Show resolved Hide resolved

gebressler reviewed Oct 14, 2021

View reviewed changes

shell/platform/fuchsia/flutter/component_v2.h Show resolved Hide resolved

ypomortsev approved these changes Oct 14, 2021

View reviewed changes

shell/platform/fuchsia/flutter/component_v2.cc Outdated Show resolved Hide resolved

shell/platform/fuchsia/flutter/component_v2.cc Show resolved Hide resolved

shell/platform/fuchsia/flutter/component_v2.cc Show resolved Hide resolved

[fuchsia] Address review feedback.

0fc678e

akbiggs added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Oct 14, 2021

fluttergithubbot merged commit b8b0d72 into flutter:master Oct 14, 2021

akbiggs deleted the flutter-runner-v2-alex-is-bad-at-git branch October 15, 2021 00:04

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to b8b0d722cd2e (10 revisions) flutter/flutter#91862

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

ab1082f

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to fe7861cafbfb (11 revisions) flutter/flutter#91865

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

ee43a0d

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to 41f767223488 (12 revisions) flutter/flutter#91876

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

6913eda

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to da2884a1c5bc (13 revisions) flutter/flutter#91878

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

e4e67f6

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to 32450391ac2d (15 revisions) flutter/flutter#91882

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

c64e0c3

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to 32450391ac2d (15 revisions) flutter/flutter#91888

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

dcb30f4

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to bcdd9fe95e7f (16 revisions) flutter/flutter#91892

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

486ce18

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to b1e7ad8c0103 (17 revisions) flutter/flutter#91897

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

51d955f

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to 60d0fdfa2232 (18 revisions) flutter/flutter#91901

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

540d196

engine-flutter-autoroll mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to cfa388895461 (19 revisions) flutter/flutter#91904

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2021

b8b0d72 [fuchsia] Create CF v2 Flutter runner. (flutter/engine#29142)

c5f403d

iskakaushik mentioned this pull request Oct 15, 2021

Roll Engine from 8034050e7810 to 60d0fdfa2232 (18 revisions) flutter/flutter#91907

Merged

8 tasks

kylinchen pushed a commit to XianyuTech/engine that referenced this pull request Oct 22, 2021

[fuchsia] Create CF v2 Flutter runner. (flutter#29142)

7bd2c12

akbiggs mentioned this pull request Oct 26, 2021

[fuchsia] INTERNAL epitaph for non-zero return codes #29330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[fuchsia] Create CF v2 Flutter runner. #29142

[fuchsia] Create CF v2 Flutter runner. #29142

Uh oh!

akbiggs commented Oct 12, 2021

Uh oh!

chaselatta Oct 12, 2021

Uh oh!

akbiggs Oct 12, 2021

Uh oh!

gebressler Oct 14, 2021

Uh oh!

akbiggs Oct 14, 2021

Uh oh!

akbiggs commented Oct 13, 2021 •

edited

Loading

Uh oh!

arbreng left a comment

Uh oh!

Uh oh!

Uh oh!

gebressler commented Oct 14, 2021 via email

Uh oh!

chinmaygarde commented Oct 14, 2021

Uh oh!

arbreng commented Oct 14, 2021

Uh oh!

ypomortsev left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[fuchsia] Create CF v2 Flutter runner. #29142

[fuchsia] Create CF v2 Flutter runner. #29142

Uh oh!

Conversation

akbiggs commented Oct 12, 2021

Uh oh!

chaselatta Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

akbiggs Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

gebressler Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

akbiggs Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

akbiggs commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arbreng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gebressler commented Oct 14, 2021 via email

Uh oh!

chinmaygarde commented Oct 14, 2021

Uh oh!

arbreng commented Oct 14, 2021

Uh oh!

ypomortsev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akbiggs commented Oct 13, 2021 •

edited

Loading