Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flutter_tool] Use engine flutter_runner prebuilts #43381

Merged
merged 2 commits into from Oct 28, 2019
Merged

[flutter_tool] Use engine flutter_runner prebuilts #43381

merged 2 commits into from Oct 28, 2019

Conversation

zanderso
Copy link
Member

@zanderso zanderso commented Oct 23, 2019

Description

This PR switches Fuchsia targets to use the engine prebuilts instead of the prebuilts bundled with the Fuchsia SDK. The prebuilts bundled with the Fuchsia SDK are to be removed as part of the flutter_runner migration to the engine repo.

Tests

Updated existing tests, and added tests for querying FuchsiaDevice.targetPlatform.

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 tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Oct 23, 2019
@@ -106,7 +106,7 @@ void _rewriteCmx(BuildMode mode, File src, File dst) {
throwToolExit('Fuchsia does not support build mode "$mode"');
break;
}
cmx['runner'] = 'fuchsia-pkg://fuchsia.com/$runner#meta/$runner.cmx';
cmx['runner'] = 'fuchsia-pkg://flutter_tool/$runner#meta/$runner.cmx';
Copy link
Contributor

Choose a reason for hiding this comment

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

that this works blows my mind

String flutterRunnerName = 'flutter_jit_runner';
if (!debuggingOptions.buildInfo.isDebug &&
!debuggingOptions.buildInfo.isProfile) {
flutterRunnerName = 'flutter_jit_product_runner';
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use the aot runner

Copy link
Contributor

Choose a reason for hiding this comment

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

(can be a separate PR of course)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the tool doesn't know how to build aot Fuchsia packages, yet, so that'll probably be a bigger change.

}
final String appName = flutterProject.fuchsia.project.manifest.appName;
final String cmxPath = fs.path.join(
flutterProject.fuchsia.meta.path, '$appName.cmx');
final File cmxFile = fs.file(cmxPath);
if (!cmxFile.existsSync()) {
throwToolExit('Fuchsia build requires a .cmx file at $cmxPath for the app');
throwToolExit('The Fuchsia build requires a .cmx file at $cmxPath for the app.');
Copy link
Member

Choose a reason for hiding this comment

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

Could we link to an example cmx file here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a cmx file for the stocks example.

final File farArchive = package.farArchive(
debuggingOptions.buildInfo.mode);
if (!await fuchsiaPackageServer.addPackage(farArchive)) {
printError('Failed to add package to the package server');
return LaunchResult.failed();
}

// Serve the flutter_runner.
Copy link
Member

Choose a reason for hiding this comment

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

Could this use the existing artifacts.getArtifactPath(...) APIs instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

help: 'The package source to use for the flutter_runner. '
'"fuchsia.com" implies using a runner in the installed Fuchsia image. '
'"flutter_tool" implies using a runner distributed with Flutter.',
allowed: <String>['fuchsia.com', 'flutter_tool'],
Copy link
Member

Choose a reason for hiding this comment

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

nit: represent this as an enum/class or create const values to avoid stringly-typed issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

case Artifact.fuchsiaPlatformDill:
case Artifact.fuchsiaPatchedSdk:
case Artifact.fuchsiaFlutterJitRunner:
assert(false, 'Invalid local engine artifact $artifact.');
Copy link
Member Author

Choose a reason for hiding this comment

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

@iskakaushik Do you know where these would be in a local engine build?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, missed this somehow. They would be in the out/fuchsia_x64_.../flutter_aot_runner.far for example.

@zanderso
Copy link
Member Author

ptal

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM with nit

Co-Authored-By: Jonah Williams <jonahwilliams@google.com>
@zanderso zanderso merged commit 0dfabb2 into flutter:master Oct 28, 2019
@zanderso zanderso deleted the use-correct-flutter-runner branch October 28, 2019 16:38
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
* [flutter_tool] Use engine flutter_runner prebuilts

* Update packages/flutter_tools/lib/src/fuchsia/fuchsia_build.dart

Co-Authored-By: Jonah Williams <jonahwilliams@google.com>
@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
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants