Skip to content

Allow flavors and custom build types in host app#36805

Merged
blasten merged 5 commits intoflutter:masterfrom
blasten:fix-merge-assets
Jul 29, 2019
Merged

Allow flavors and custom build types in host app#36805
blasten merged 5 commits intoflutter:masterfrom
blasten:fix-merge-assets

Conversation

@blasten
Copy link

@blasten blasten commented Jul 24, 2019

Description

In add2app scenarios, it is pretty common that the host app may have flavors or custom build types. However, when a Flutter module is added, you can't build the APK anymore. The Flutter project doesn't know about the parent project flavor when merging the assets. As a result you end up with a failure at build time. In the case of custom build types, if you build debug, then the debug APK doesn't have the snapshots since they are under the assets/flutter_assets directory.

This PR reverts #22707 since I'm unable to reproduce issue #19818 in master after reverting it.

Related Issues

Fixes all these issues:
#30916
#34089
#36479
#29648

Tests

I added the following tests:

Integration test: module_host_with_custom_build.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

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.

@blasten blasten added tool Affects the "flutter" command-line tool. See also t: labels. t: gradle "flutter build" and "flutter run" on Android a: existing-apps Integration with existing apps via the add-to-app flow a: build Building flutter applications with the tool labels Jul 24, 2019
@blasten blasten requested review from xster and zanderso July 24, 2019 05:32
@fluttergithubbot fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jul 24, 2019
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Jul 24, 2019

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

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

/cc @dnfield

@blasten
Copy link
Author

blasten commented Jul 24, 2019

@dnfield would it make sense for the bot to also check for dev/devicelab/*?

@blasten blasten force-pushed the fix-merge-assets branch from 9298404 to c64368c Compare July 24, 2019 17:25
xster
xster reviewed Jul 24, 2019
View reviewed changes
Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks!!

LGTM

...n_tests/module_host_with_custom_build/app/src/main/java/io/flutter/add2app/MainActivity.java
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(Flutter.createView(this, getLifecycle(), "route1"));
Copy link
Member

@xster xster Jul 24, 2019

Choose a reason for hiding this comment

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

Please don't use the facade classes. Use https://github.com/flutter/flutter/wiki/Experimental:-Add-Flutter-Activity instead.

Copy link
Member

@xster xster Jul 24, 2019

Choose a reason for hiding this comment

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

Good point on current fragment dependencies on the new embedding.

I think the easiest way to "exercise the engine and VM" which I think is the goal here is to just let MainActivity extend io.flutter.app.FlutterActivity instead of using the facade classes.
cc @matthew-carroll

zanderso
zanderso approved these changes Jul 24, 2019
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm after addressing other reviewers' comments.

@xster
Copy link
Member

xster commented Jul 25, 2019

LGTM

@xster xster mentioned this pull request Jul 25, 2019
Open
@blasten blasten merged commit 9f39cad into flutter:master Jul 29, 2019
@blasten blasten deleted the fix-merge-assets branch July 29, 2019 16:26
@blasten blasten mentioned this pull request Jul 29, 2019
Closed
@xster xster mentioned this pull request Jul 30, 2019
Closed
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@johnsonmh
Allow flavors and custom build types in host app (flutter#36805)
122a54a 
Fixes these issues:
flutter#30916
flutter#34089
flutter#36479
flutter#29648
@blasten blasten mentioned this pull request Sep 27, 2019
Merged
9 tasks
tarek360
tarek360 reviewed Dec 30, 2019
View reviewed changes
packages/flutter_tools/gradle/flutter.gradle
if (packageAssets) {
String mainModuleName = "app"
try {
String tmpModuleName = project.rootProject.ext.mainModuleName
Copy link

@tarek360 tarek360 Dec 30, 2019

Choose a reason for hiding this comment

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

These days many many projects have a custom name for the app module, actually, many projects have multiple app modules.
So hardcoding the :app completely breaks many projects.

by removing this script, that makes this issue come back again.

We need a way to let developers set their app module name, or it will be better if you can detect it automatically in this script.

Please let me know if I can help!
Thank you!
@blasten @zanderso @xster

Copy link
Author

@blasten blasten Jan 2, 2020

Choose a reason for hiding this comment

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

Potentially, we could iterate through all the subprojects and find the one that applies the com.android.application plugin. Is this something you would like to explore?

Copy link

@tarek360 tarek360 Jan 7, 2020

Choose a reason for hiding this comment

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

Sure, I'd figure out that!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@xster xster xster left review comments

@matthew-carroll matthew-carroll matthew-carroll left review comments

@zanderso zanderso zanderso approved these changes

+1 more reviewer

@tarek360 tarek360 tarek360 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: build Building flutter applications with the tool a: existing-apps Integration with existing apps via the add-to-app flow c: contributor-productivity Team-specific productivity, code health, technical debt. t: gradle "flutter build" and "flutter run" on Android tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@blasten @fluttergithubbot @xster @zanderso @matthew-carroll @tarek360 @googlebot