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

[fuchsia] Make it easy to flip the whole system between flutter_jit_a… #3873

Merged
merged 2 commits into from
Jul 19, 2017
Merged

[fuchsia] Make it easy to flip the whole system between flutter_jit_a… #3873

merged 2 commits into from
Jul 19, 2017

Conversation

rmacnak-google
Copy link
Contributor

…pp and flutter_aot_app.

@krisgiesing
Copy link
Contributor

krisgiesing commented Jul 13, 2017

That was fast :)

So this does 1) and 2) from the list in the email thread, yes? And 3) would presumably be a follow on change?

@rmacnak-google
Copy link
Contributor Author

Right. As for 3), I'm open to name suggestions for a GN variable :)

@@ -4,7 +4,7 @@

import("//flutter/build/flutter_app.gni")

flutter_app("hello_flutter") {
flutter_jit_app("hello_flutter") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to change? I thought most clients would use flutter_app and follow external configuration?

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea here is to build the hello_flutter app both ways to make sure that both ways work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is to ensure there's at least one app built both ways. There's a flutter_aot_app just below here.

# disable_analysis (optional)
# Prevents the analysis script from being generated.
template("flutter_app") {
if (true) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I like the idea of this being toggled by a global flag, but I think it's good to be explicit that there's a choice here.

# disable_analysis (optional)
# Prevents the analysis script from being generated.
template("flutter_app") {
template("flutter_jit_app") {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "flutter_dynamic_app" or "dynamic_flutter_app"

Copy link
Member

Choose a reason for hiding this comment

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

And "flutter_static_app" or "static_flutter_app" below.

@rmacnak-google rmacnak-google merged commit 2455f20 into flutter:master Jul 19, 2017
@rmacnak-google rmacnak-google deleted the flutter_app branch November 8, 2017 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants