Skip to content

[flutter_tool] Various fixes for 'run' for Fuchsia.#44920

Merged
zanderso merged 1 commit intoflutter:masterfrom
zanderso:fix-pkgctl-rm
Nov 14, 2019
Merged

[flutter_tool] Various fixes for 'run' for Fuchsia.#44920
zanderso merged 1 commit intoflutter:masterfrom
zanderso:fix-pkgctl-rm

Conversation

@zanderso
Copy link
Copy Markdown
Member

Description

A few things:

  • Fixes the pkgctl command to remove the tool's package repo
  • Makes the code that creates and deletes the package repo a little more resilient
  • Adds cmx files for hello_world and flutter_gallery examples
  • Stops passing the --no-link-platform flag to the Fuchsia kernel compiler. This was needed to avoid crashes with the flutter_runner most recently rolled into flutter/flutter. I suspect a similar change may be needed in dart_kernel.gni in the engine repo.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@zanderso zanderso added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 14, 2019
@fluttergithubbot fluttergithubbot added d: examples Sample code and demos c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. labels Nov 14, 2019
@fluttergithubbot
Copy link
Copy Markdown
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rmacnak-google @alexmarkov without removing this, apps crash on startup on a missing class in dart:ui. Is this expected?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like there's a mismatch between the platform being used to compile the app and the platform embedded in the runner's core snapshot. Is the missing class something that was recently introduced?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We shouldn't pass --no-link-platform in AOT mode (kernel compiler should complain).
  2. In JIT (debug) mode, the recommended way to load platform libraries is from core snapshot, and --no-link-platform will save some time and space. If app crashes when --no-link-platform is used, it means that isolate is not initialized from core snapshot, and platform libraries were loaded from kernel file. We saw similar problem with desktop embedder recently. If, for some reason, we don't want to use core snapshots then we can remove --no-link-platform, but this is less efficient compared to spawning isolates from core snapshot.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like: #44724

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed: #44925

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added TODO to add back when #44925 is fixed.

@zanderso zanderso changed the title [flutter_tool] Various fixes for 'run' for Fuchisa. [flutter_tool] Various fixes for 'run' for Fuchsia. Nov 14, 2019
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you generate this file?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's currently no automated way to generate the cmx files. I initially copied one from the Fuchsia tree for the stocks example, then copied that one here, changing the component name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll likely need to generate one when we want to support Fuchsia with 'flutter create' though.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh awesome!

Copy link
Copy Markdown
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zanderso zanderso merged commit afe4830 into flutter:master Nov 14, 2019
@zanderso zanderso deleted the fix-pkgctl-rm branch November 14, 2019 20:39
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. d: examples Sample code and demos tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants