Skip to content

Conversation

@GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Aug 14, 2020

Master PR for deferred components build and setup verification tooling.

Primary issue: #57617

go/flutter-aot-split

@flutter-dashboard flutter-dashboard bot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Aug 14, 2020
@GaryQian GaryQian force-pushed the splittools branch 2 times, most recently from 412c3da to 9e4bf2b Compare August 25, 2020 01:26
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.
  • The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux build_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux hostonly_devicelab_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite analyze-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-general-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-commands-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-integration-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite firebase_test_lab_tests-0-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite firebase_test_lab_tests-1-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite firebase_test_lab_tests-2_last-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite deploy_gallery-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-general-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-commands-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-integration-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite deploy_gallery-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.

Copy link
Contributor

Choose a reason for hiding this comment

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

use platform file separator?

Copy link
Contributor

Choose a reason for hiding this comment

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

This also needs to be in the output files. Is there anything wrong with always generating it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This option is the switch that determines if gen_snapshot produces split aot or not. WIthout, it produces a regular aot snapshot.

Copy link
Contributor

Choose a reason for hiding this comment

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

whats going on here?

Copy link
Contributor Author

@GaryQian GaryQian Jan 20, 2021

Choose a reason for hiding this comment

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

Copying all app.so files + manifest.json instead of just app.so

@jonahwilliams
Copy link
Contributor

All of the additional files that are generated need to be tracked somehow so that build invalidations work. There are a few mechanisms for doing so in gradle and in the tool (see input/output files and the build_system doc comment).

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.
  • The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite analyze-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-commands-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite deploy_gallery-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite deploy_gallery-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.

@kangwang1988
Copy link
Contributor

Watching this thread in order to try it ASAP.

@GaryQian GaryQian force-pushed the splittools branch 2 times, most recently from ac732e9 to 5772d39 Compare October 23, 2020 06:55
@jonahwilliams
Copy link
Contributor

Something I've noticed is that I don't see where we're tracking the addition .so files output by gen snapshot - we need those to correctly tell gradle how to clean stuff up

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.

Second pass. Getting closer!

Copy link
Member

Choose a reason for hiding this comment

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

does this inherit from the base apk's build.gradle? i.e. does profile build work and does release build work without signing locally while testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to remove these and let developers opt into it manually if they wish.

Copy link
Member

Choose a reason for hiding this comment

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

Say it does nothing (presumably) if set to true for a project with no deferred components?

Maybe add a link to documentation (at some point) to describe when true really becomes (verifiably) true? When deferred imports are used or when the new tag is added to pubspec.yaml presumably?

Copy link
Member

Choose a reason for hiding this comment

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

@blasten here we're trying to add entries into the android { } section of a user's build.gradle. We can just ask them to apply from: 'deferred_components.gradle' and then we'll just own that file and freely manipulate it right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to remove the need to add this section manually. It is now added programmatically in flutter.gradle.

Copy link
Member

Choose a reason for hiding this comment

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

+1. It's already in our tools dependencies

Copy link
Member

Choose a reason for hiding this comment

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

didn't you make a file for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite. This is strings.xml. The contents here are too dynamic for a template to 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.

I want to try to change this to use AndroidManifest.xml + applicationInfo to store the mapping rather than strings

Copy link
Member

Choose a reason for hiding this comment

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

describe the problem statement first. i.e. why is there a golden file and what is it solving

Copy link
Member

Choose a reason for hiding this comment

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

This means if their file is 1000 lines and we're making it 1010 lines, they have to diff it themselves. It's ok but if we can make a .patch file, even better.

@GaryQian
Copy link
Contributor Author

Tests coming soon.

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.

Please let @jonahwilliams take another look. This still needs tests but otherwise LGTM

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if we printed a message when we fallback too

Copy link
Member

Choose a reason for hiding this comment

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

nit: line length

Copy link
Member

Choose a reason for hiding this comment

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

mini-nit: I assume this can just be a final variable instead of a getter?

Copy link
Member

Choose a reason for hiding this comment

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

nit: line breaks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll be sure to give this a full formatting pass :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd generally print something here. Since there's a degree of dynamism wrt to the command flag defaulting to true but it being only true when the pubspec tag is also present. So print something when it's really on. On the same line, state whether the verification is also on or off.

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 still not sure I understood what the answer to my question was. If the loading units have a list of their libraries, why do you need to keep another list of the libraries?

Copy link
Member

Choose a reason for hiding this comment

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

Describe a bit what the implications are? Maybe it's just an asset component?

Copy link
Member

Choose a reason for hiding this comment

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

nit: line length

t # This is a combination of 3 commits.

Separate prebuild and build tooling, early working(ish) version

Refactor, cleanup

Integrate dart library names

golden generation refactor

REmove extra deps

Remove conflicts

Sync version numbers with local.properties in modules

Remove no-use-bare-instructions

Use strings.xml for loading unit -> module mapping

Copy assets prototype

Begin assets packaging

Pack assets into split mdoule

Package assets

Fix after rebase

Add shrinking disable back to support multi-apk builds

Rename to deferred_component_config

Move commands to proper location

Move commands to new locations, cleanup

Additional cleanup, rename to deferred d components

Begin transition into single command setup

Basic one-command verification and setup

Build multiple ABIs

Assets work

Working intermediates system

Shift .so multiplexing into dart, cleanup

Consolidation and additional cleanup

Goldens work

Migrate so copy to new target

Move setup to prebuild

Working prebuild for setup

Setup verification work

Working setup system

Cleanup and begin backwards/debug compatibility

Cleanup

More cleanup:

Refactor verification and setup into calss, split into two parts: pre and post assembly

Refactor setup code, address comments

Refactoring, begin templates impl, address comments

Working template based generation

Docs, refactor, rename

Refactor, libs copy depfile

More depfiles work, debug run assets working

Add dynamicFeatures build.gradle programmatically

More refactoring, depfile fixes, improvement to copying and loading unit parsing

Include unused loading units in base module

Crash fixes:

Prevent validation on debug builds, fix flutter run behavior

Flutter run working again

AndroidManifest.xml validation and writing

Move check to Target to track dependencies.

Resolve various crashes and flutter run issues

Fix multi-split assets

Generate diff

Message formatting, async fixes

Fix strings.xml edits and log recommended command to apply diff

Improve strings.xml algorithm

Improve regex, Add unit tests

Begin validator tests, custom loggers

Null safety

Passing initial tests

Add many validator tests

Component android dir tests

Add tests for AndroidManifest mapping and string.xml names

Finish validator tests

Test for deferred components .so copying

Add validator Target test

flutter manifest tests

Round 1 analyzer fixes

Refactor, passes analyzer

Fix casts

Merge with upstream

Merge

Fix abi mismatch when switching targets between builds

Pass abi to validator

Switch to BufferLogger

Begin integration tests for tooling

Building deferred components apps works in integration test

Passing build appbundle integration tests

Fix tests and analyzer, cleanup:

Whitespaces

Update diff

Sync with validator PR

Sync with validator PR tests

Integrate with new validator

Sync with splitbuild

Fix null dep

Merge changes

Add tests for target
@GaryQian
Copy link
Contributor Author

GaryQian commented Mar 5, 2021

#76192 contains everything remaining in this PR and it has just landed. Closing.

@GaryQian GaryQian closed this Mar 5, 2021
@GothicFox
Copy link

@kangwang1988 FYI, if you wanted to try this, this tooling PR is currently in an extremely rough working state. There are a few extra steps and limitations, but it should be able to produce basic working Flutter apps with Dynamic Features enabled. I can share a WIP doc with instructions if you wish, but the overall process is to write a split aot flutter app (example: https://github.com/GaryQian/Flutter-Split-AOT-Sample), run flutter build setupsplitaot, make any changes to bundle_config.yaml, and run flutter build splitaot --target-platform=android_arm64 --release (these commands and limitations are temporary)

I would be happy to hear any feedback or provide more detailed information on testing early versions of this feature. I have left the API open enough for custom non-google-play-store apps to write their own DynamicFeatureManager on Android and inject the manager into Flutter.

Hi Gary, I'm focusing on the aot-split feature of flutter, so I'm trying to run the demo: https://github.com/GaryQian/Flutter-Split-AOT-Sample, but failed, it just shows that no splitaot command found for flutter build,I'm using the newest flutter-sdk and flutter-engine and dart-sdk of the master branch, so what version should I use? or how? thanks

@GaryQian
Copy link
Contributor Author

GaryQian commented Apr 20, 2021

@BottleChain Ahh, I apologize, that demo is very outdated. The actual landed version is completely different. I'll likely update that sample repo at some point. In the meantime, the new WIP instructions/guide is at https://docs.google.com/document/d/10ngyMlLIHiDiD_DueE_NHxzl-obqnfdzvMQj5_WV3Z4/edit?usp=sharing

You (and anyone else interested) can go ahead and request access, I'll be happy to grant it. I didn't make it public initially, so it is a bit of a logistical bump to properly publicize, so this will have to do for now. It should be published soon.

I'd love any feedback on the feature/process/etc!

Quick reference: The new command is just flutter build appbundle. The tool will build deferred by default if opted-in via config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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.

7 participants