Merge Flutter assets in add to app by blasten · Pull Request #41333 · flutter/flutter

blasten · 2019-09-26T01:29:38Z

Description

PR #36805 broke add to app: flutter_assets ins't in the APK if the mergeAssets tasks runs before processManifest. This is the case if the host app uses the https://fabric.io Gradle plugin.

PR #38542 tried to fix this issue, but it didn't add a minimal reproduction / test.

Related Issues

Fixes #38286

Tests

Integration test.

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.

mklim · 2019-09-27T17:28:13Z

packages/flutter_tools/gradle/flutter.gradle

            Task cleanPackageAssets = project.tasks.findByPath(":flutter:cleanPackage${variant.name.capitalize()}Assets")
+            // In add to app scenarios, :flutter is a subproject of another Android app.
+            // We know that :flutter is used as a subproject when these tasks exist.
+            boolean isUsedAsSubproject = packageAssets && cleanPackageAssets


Are there likely to be false positives or negatives from this? I'm not familiar with either of these, not sure how reliable this is.

I don't think so since the :flutter project doesn't exist unless the app is using add 2 app and include_flutter.groovy.

I have also added an assert in line 654 to exit early if there isn't a root project named :app.

mklim · 2019-09-27T17:29:14Z

packages/flutter_tools/gradle/flutter.gradle

+            // Flutter module included as a subproject in add to app.
+            Project appProject = project.rootProject.findProject(':app')
+            assert appProject != null
+            appProject.afterEvaluate {


I know enough to know that afterEvaluate is generally not recommended in Gradle scripts because it works outside of Gradle's normal execution flow, but I don't know enough about what to recommend here instead. :| If this is the only thing that works I think it makes sense to use it, but if there's another option I would prefer that instead.

yeah. It needs to wait until the Android plugin loads. Otherwise appProject.android is undefined ☹️

Looking through the docs, it seems like afterEvaluate is the only way to perform additional configuration after all the definitions in a build script have been applied. I'm not sure if there's a better way other not adding the implicit dependency on AGP.

mklim

LGTM

kassadin · 2019-10-27T18:55:43Z

Related Issues #42214.The main android application in the project may not be called app, such as sample. And there may be more than one application, so also need to check if it rely on the flutter module.

maybe it's fixed like this

...
// Flutter module included as a subproject in add to app.
def isAppDepOnFlutter = {Project p->
    def depFlutter = p.configurations.find {
        it.dependencies.withType(ProjectDependency).find {
            it.dependencyProject.plugins.hasPlugin(FlutterPlugin)
            // maybe it.dependencyProject == this?
            // or it.dependencyProject.name == "flutter"
        }
    }
    return  depFlutter != null
}

project.rootProject.subprojects {
    it.afterEvaluate {
        if (it.plugins.hasPlugin('android') && isAppDepOnFlutter(it)) {
            it.android.applicationVariants.all { appProjectVariant ->
                // Find a compatible application variant in the host app.
...

joshuadeguzman · 2019-12-16T11:24:29Z

I encountered this issue as well, it somehow forces me to rename my existing native app module (eg. named as flapp) to app. Is this an expected turn from these changes?

Flutter Doctor

View Log

$ flutter doctor -v

[✓] Flutter (Channel v1.12.13-hotfixes, v1.12.13+hotfix.6, on Mac OS X 10.14.5 18F132, locale en-PH)
    • Flutter version 1.12.13+hotfix.6 at /Users/joshuadeguzman/Documents/Tools/flutter
    • Framework revision 18cd7a3601 (5 days ago), 2019-12-11 06:35:39 -0800
    • Engine revision 2994f7e1e6
    • Dart version 2.7.0


[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.2)
    • Android SDK at /Users/joshuadeguzman/Library/Android/sdk
    • Android NDK location not configured (optional; useful for native profiling support)
    • Platform android-29, build-tools 29.0.2
    • ANDROID_HOME = /Users/joshuadeguzman/Library/Android/sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_152-release-1343-b01)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 10.3)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 10.3, Build version 10G8
    • CocoaPods version 1.8.4

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 3.4)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin version 36.1.1
    • Dart plugin version 183.6270
    • Java version OpenJDK Runtime Environment (build 1.8.0_152-release-1343-b01)

[!] IntelliJ IDEA Ultimate Edition (version 2019.2.3)
    • IntelliJ at /Applications/IntelliJ IDEA.app
    ✗ Flutter plugin not installed; this adds Flutter specific functionality.
    ✗ Dart plugin not installed; this adds Dart specific functionality.
    • For information about installing plugins, see
      https://flutter.dev/intellij-setup/#installing-the-plugins

[✓] VS Code (version 1.40.2)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.7.1

[✓] Connected device (3 available)
    • Pixel 2 XL • 711KPGS0993839 • android-arm64  • Android 10 (API 29)
    • Chrome     • chrome         • web-javascript • Google Chrome 78.0.3904.108
    • Web Server • web-server     • web-javascript • Flutter Tools

! Doctor found issues in 1 category.

cybaker · 2020-05-06T19:25:53Z

This is still a problem in May. I am required to rename my app module to "app" instead of it's usual name.

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 26, 2019

googlebot added the cla: yes label Sep 26, 2019

blasten added the work in progress; do not review label Sep 26, 2019

blasten force-pushed the merge_resources branch 3 times, most recently from 1f69605 to c7ed63e Compare September 26, 2019 21:03

Copy Flutter assets before merging the rest of resources

d2b68a6

blasten force-pushed the merge_resources branch from c7ed63e to d2b68a6 Compare September 27, 2019 05:14

blasten changed the title ~~Copy Flutter assets before merging the rest of resources~~ Merge Flutter assets in add to app Sep 27, 2019

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Sep 27, 2019

blasten removed the work in progress; do not review label Sep 27, 2019

blasten requested review from mklim and zanderso September 27, 2019 05:31

zanderso approved these changes Sep 27, 2019

View reviewed changes

mklim reviewed Sep 27, 2019

View reviewed changes

mklim approved these changes Sep 27, 2019

View reviewed changes

blasten merged commit 2466ca5 into flutter:master Sep 27, 2019

blasten deleted the merge_resources branch September 27, 2019 17:53

blasten mentioned this pull request Sep 27, 2019

Add flutter assets #38542

Closed

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019

Copy Flutter assets before merging the rest of resources (flutter#41333)

25053e1

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge Flutter assets in add to app#41333

Merge Flutter assets in add to app#41333
blasten merged 1 commit intoflutter:masterfrom
blasten:merge_resources

blasten commented Sep 26, 2019 •

edited

Loading

Uh oh!

mklim Sep 27, 2019

Uh oh!

blasten Sep 27, 2019

Uh oh!

mklim Sep 27, 2019

Uh oh!

blasten Sep 27, 2019 •

edited

Loading

Uh oh!

mklim left a comment

Uh oh!

kassadin commented Oct 27, 2019

Uh oh!

joshuadeguzman commented Dec 16, 2019 •

edited

Loading

Uh oh!

cybaker commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

blasten commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

mklim Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

blasten Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

mklim Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

blasten Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

kassadin commented Oct 27, 2019

Uh oh!

joshuadeguzman commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cybaker commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

blasten commented Sep 26, 2019 •

edited

Loading

blasten Sep 27, 2019 •

edited

Loading

joshuadeguzman commented Dec 16, 2019 •

edited

Loading