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

Adding support for android app bundle - Issue #17829 #24440

Merged
merged 13 commits into from Dec 21, 2018

Conversation

Projects
None yet
@pranayairan
Copy link
Contributor

pranayairan commented Nov 16, 2018

Implementation Details
This PR adds support for Android App Bundle . Android App bundle is a new packaging format that helps in reducing app size and enabled new features like dynamic delivery for android apps.

I added a new build command appbundle to the flutter tool and introduced bulding app bundle as another option of building APK.

To test : follow all steps to modify your build.gradle file as listed here https://flutter.io/docs/deployment/android. To generate a android app bundle run following command flutter build appbundle

In my current testing this with my app, I have not found any improvements in app size, since flutter apk build only adding arm file. But as we add support to more architectures for flutter app and add more resources, android app bundle can help in optimizing the app size significantly.

Affected Areas
Flutter tool build section.

Issue Link
#17829

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 16, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
@pranayairan

This comment has been minimized.

Copy link
Contributor Author

pranayairan commented Nov 16, 2018

I signed it!

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 16, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Nov 16, 2018

bin/flutter Outdated
# FLUTTER_TOOL_ARGS="--checked $FLUTTER_TOOL_ARGS"
# FLUTTER_TOOL_ARGS="$FLUTTER_TOOL_ARGS --observe=65432"
#FLUTTER_TOOL_ARGS="--checked $FLUTTER_TOOL_ARGS"
#FLUTTER_TOOL_ARGS="$FLUTTER_TOOL_ARGS --observe=65432"

This comment has been minimized.

@Hixie

Hixie Nov 16, 2018

Contributor

nit: probably best to leave it as before to avoid churn

'The build process for Android has changed, and the current project configuration\n'
'is no longer valid. Please consult\n\n'
' https://github.com/flutter/flutter/wiki/Upgrading-Flutter-projects-to-build-with-gradle\n\n'
'for details on how to upgrade the project.'

This comment has been minimized.

@Hixie

Hixie Nov 16, 2018

Contributor

nit: indentation is off (each string should be the same indentation)

printError(message, wrap: false);
}
throwToolExit('Try re-installing or updating your Android SDK.');
}

This comment has been minimized.

@Hixie

Hixie Nov 16, 2018

Contributor

can we factor out these checks so that all the commands that care can just call the one method that does all of this?

This comment has been minimized.

@pranayairan

pranayairan Nov 16, 2018

Author Contributor

do you think androidSdk is the place where I should add this check? it already has a method validateSdkWellFormed that is doing the major heavy lifting of checking. All this code is doing is printing it on the console and throwing an error.


if(isBuildingBundle){
assembleTask = project.bundleTaskFor(buildInfo);
}else{

This comment has been minimized.

@Hixie

Hixie Nov 16, 2018

Contributor

nit: please use the same format as other places in the file (e.g. argument goes on the next line, spaces after if and before { and around else, etc)

@@ -346,7 +356,7 @@ Future<void> _buildGradleProjectV2(
}
}
final Status status = logger.startProgress(
"Gradle task '$assembleTask'...",
"Gradle task'$assembleTask'...",

This comment has been minimized.

@Hixie

Hixie Nov 16, 2018

Contributor

missing space

@Hixie

This comment has been minimized.

Copy link
Contributor

Hixie commented Nov 16, 2018

cc @jason-simmons for detailed review

Thanks so much for this contribution!

@jason-simmons

This comment has been minimized.

Copy link
Contributor

jason-simmons commented Nov 16, 2018

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 16, 2018

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 16, 2018

pairan and others added some commits Nov 16, 2018

pairan
@pranayairan

This comment has been minimized.

Copy link
Contributor Author

pranayairan commented Nov 16, 2018

I signed it!

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 16, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@Hixie Hixie added cla: yes and removed cla: no labels Nov 16, 2018

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 16, 2018

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@jason-simmons
Copy link
Contributor

jason-simmons left a comment

Overall approach LGTM

if (bundleFile.existsSync())
return bundleFile;
if (buildInfo.flavor != null) {
// Android Studio Gradle plugin v3 adds flavor to path. for bundle the folder name is flavorModename

This comment has been minimized.

@jason-simmons

jason-simmons Nov 19, 2018

Contributor

nit: would write this as Android Studio Gradle plugin v3 adds the flavor to the path. For the bundle the folder name is the flavor plus the mode name.

return bundleFile;
if (buildInfo.flavor != null) {
// Android Studio Gradle plugin v3 adds flavor to path. for bundle the folder name is flavorModename
bundleFile = fs.file(fs.path.join(project.bundleDirectory.path, buildInfo.flavor+modeName, bundleFileName));

This comment has been minimized.

@jason-simmons

jason-simmons Nov 19, 2018

Contributor

nit: use spaces around operators: flavor + modeName

@@ -447,7 +504,7 @@ Map<String, String> get _gradleEnv {
}

class GradleProject {
GradleProject(this.buildTypes, this.productFlavors, this.apkDirectory);
GradleProject(this.buildTypes, this.productFlavors, this.apkDirectory,this.bundleDirectory);

This comment has been minimized.

@jason-simmons

jason-simmons Nov 19, 2018

Contributor

nit: add a space after the comma: , this.bundleDirectory


@override
final String description = 'Build an Android App Bundle file from your app.\n\n'
'This command can build debug and release versions app bundle for your application. \'debug\' builds support '

This comment has been minimized.

@jason-simmons

jason-simmons Nov 19, 2018

Contributor

nit: versions of an app bundle

appSize = ' (${getSizeAsMB(apkFile.lengthSync())})';
}
printStatus('Built ${fs.path.relative(apkFile.path)}$appSize.');
}else{

This comment has been minimized.

@jason-simmons

jason-simmons Nov 19, 2018

Contributor

nit: add spaces before and after else

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 19, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 20, 2018

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@Hixie

This comment has been minimized.

Copy link
Contributor

Hixie commented Nov 20, 2018

This patch is fantastic, thanks so much for this.

Unfortunately as of today we're in a freeze because of the Flutter Live event, but we'll make sure to land it afterwards. Thanks again for your contribution!

@pranayairan

This comment has been minimized.

Copy link
Contributor Author

pranayairan commented Nov 20, 2018

Awesome :) happy to help. Will circle back after Flutter live.

@yeshwanthvshenoy

This comment has been minimized.

Copy link

yeshwanthvshenoy commented Dec 1, 2018

@pranayairan Quick Question: Does this work with iOS as well or is it more towards the Android system?

@Taormina

This comment has been minimized.

Copy link

Taormina commented Dec 2, 2018

@yeshwanthvshenoy This should be an Android only thing for submission to the Google Play Store. You can read more about app bundles here: https://developer.android.com/guide/app-bundle/

@pranayairan

This comment has been minimized.

Copy link
Contributor Author

pranayairan commented Dec 5, 2018

@Hixie is the restriction for merging lifted? If yes can we merge the changes?

@vandie

This comment has been minimized.

Copy link

vandie commented Dec 5, 2018

Just out of interest, is this likely to be merged in any time soon?

@pranayairan

This comment has been minimized.

Copy link
Contributor Author

pranayairan commented Dec 6, 2018

@vandie hoping to get this merged in a day or 2, we were waiting for flutter live to be done.

updating the comment to re-run the test
updating the comment to re-run the test
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 19, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Dec 19, 2018

pranayairan added some commits Dec 20, 2018

updating comments to re-trigger build
updating comments to re-trigger build

@pranayairan pranayairan merged commit 368cd7d into flutter:master Dec 21, 2018

12 of 13 checks passed

cla/google CLAs are signed, but unable to verify author consent
analyze
Details
build_tests-linux
Details
build_tests-macos
Details
build_tests-windows
Details
codelabs-build-test
Details
docs
Details
tests-linux
Details
tests-macos
Details
tests-windows
Details
tool_tests-linux
Details
tool_tests-macos
Details
tool_tests-windows
Details

@pranayairan pranayairan deleted the pranayairan:app_bundle branch Dec 21, 2018

kangwang1988 added a commit to alibaba-flutter/flutter that referenced this pull request Dec 26, 2018

Merge branch 'flt_master' into flutter_driver_refactor
* flt_master: (143 commits)
  Roll engine 215ca15..d8c5ec0 (12 commits) (flutter#25728)
  Provide some more locations for the FAB. (flutter#24736)
  Undeprecated BigInteger support, but document what it actually does. (flutter#24511)
  ClipPath.shape and related fixes (flutter#24816)
  Handle errors in `compute()` by propagating them to the Future. (flutter#24848)
  Fix merge conflict. (flutter#25718)
  Some minor tweaks to InputDecoration (mainly docs). (flutter#24643)
  Expose font fallback API in TextStyle, Roll engine 54a3577..215ca15 (8 commits) (flutter#25585)
  Updated Shrine demo (flutter#25674)
  Pin the goldens repo to a specific commit in the android_views test. (flutter#25678)
  Friendlier flags for Dart compilation training. (flutter#25645)
  Revert dependency upgrade to see if it helps with build times and APK size (flutter#25642)
  Depend on the goldens repo through git. (flutter#25479)
  no period after an alone link in see also section (flutter#25604)
  Update links for China help (flutter#25238)
  Roll engine 6a90418..54a3577 (23 commits) (flutter#25649)
  Roll engine e859296..6a90418 (4 commits) (flutter#25643)
  Adding support for android app bundle - Issue flutter#17829 (flutter#24440)
  Revert "[O] Remove many timeouts. (flutter#23531)" (flutter#25646)
  [O] Remove many timeouts. (flutter#23531)
  ...
@MassiveFermion

This comment has been minimized.

Copy link

MassiveFermion commented Jan 9, 2019

So... What happened? Is this merged now? If yes, is there any documentation on how to use it?

@Hixie

This comment has been minimized.

Copy link
Contributor

Hixie commented Jan 15, 2019

flutter build bundle --help should describe how to use this command. Please file a bug if it's not clear. Thanks!

@alexandranb

This comment has been minimized.

Copy link

alexandranb commented Feb 5, 2019

Hi,

Could you explain on how we could create the bundle and include both versions for 32-bit and 64-bit? For now, my bundle only has armeabi-v7a and I can't seem to figure it out how to include armeabi-v81.

Thanks

@pranayairan

This comment has been minimized.

Copy link
Contributor Author

pranayairan commented Feb 5, 2019

Unfortunately, as of now, it will only include armeabi-v7a. I am not sure if there is armeabi-v81 altogether for flutter. There is an internal code that gets only the armeabi-v7a files for both bundle and APK. I will check if we can generate both.

@creativecreatorormaybenot

This comment has been minimized.

Copy link
Contributor

creativecreatorormaybenot commented Feb 21, 2019

@Hixie Should be flutter build appbundle --help.

@rockwotj

This comment has been minimized.

Copy link

rockwotj commented Mar 4, 2019

@pranayairan if you could add an option to generate both 32 bit and 64 bit you would fix #23055

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.