Handle Flutter assets outside FLX #4343

szakarias · 2017-11-09T14:13:16Z

This is another step towards removing app.flx.

A flutter_assets directory is introduced and engine is now invoked with the path to this.

If present, the snapshot is read from flutter_assets and queries on the asset channel will be answered with the path to the asset.

On Android we add the flutter_assets directory and its content to the app's data directory by handling directories in the ResourceExtractor.

Some files are still read from app.flx. This will be cleaned up in a follow up PR.

This is a breaking change since the initializers in FlutterDartProject.h are renamed.

Companion Flutter PR flutter/flutter#12944

szakarias · 2017-11-09T20:59:29Z

cc @chinmaygarde

chinmaygarde · 2017-11-09T23:53:30Z

Instead of defining a very specifically named directory along with paths within that directory in which to find certain assets, I think it would be better to make the APIs explicitly take the paths (or buffers) to the individual artifacts. This gives us the most flexibility moving forward.

This, along with the fact that this change will break embedders makes me wary of landing this patch. We should be able to make header compatible updates to the public API to accomodate this change.

chinmaygarde · 2017-11-09T23:40:40Z

runtime/dart_init.cc

+    std::vector<uint8_t>* data,
+    std::unique_ptr<DirectoryAssetBundle>& directory_asset_bundle,
+    fxl::RefPtr<ZipAssetStore>& asset_store) {
+  return (directory_asset_bundle &&


Can we maybe split this up into multiple lines with early returns on failure conditions for readability?

It could probably be split up, but there are no failure conditions as such here. The best we could do would be something like

if (directory_asset_bundle && directory_asset_bundle->GetAsBuffer(name, data)) { return true; } return asset_store && asset_store->GetAsBuffer(name, data);

(I kind of prefer the current version, though, which is copied from engine. :)

Oops, I didn't realize this was copied directly from the engine. Our preferences don't match but I don't have a strong opinion about this. Lets keep it as it.

chinmaygarde · 2017-11-09T23:48:18Z

shell/platform/darwin/ios/framework/Source/FlutterDartSource.h

 @property(nonatomic, readonly) NSURL* dartMain;
 @property(nonatomic, readonly) NSURL* packages;
-@property(nonatomic, readonly) NSURL* flxArchive;
+@property(nonatomic, readonly) NSURL* flutterAssets;


This is modifying a header that is published as part of the Flutter.framework. Existing users of the framework (iOS embedders) will break if they are currently using this API. We should decide whether to keep this API as-is and have the initializer return nil when queried with FLX assets for the sake of backwards header compatibility. We don't want to get into a situation where a Flutter upgrade causes the developer's embedder to break. I would land this patch once all the changes that remove FLXs are consolidated and we can make one breaking change along with notifications on appropriate channels and changelogs.

The change above is not in the public API, however the change in FlutterDartProject.h is, and I agree with postponing the breaking change until FLX is completely removed. I'll change this PR to keep the FLX initializer and add an initializer with Flutter assets directory, WDYT?

This will be a breaking change once we stop putting assets in app.flx anyway, so we might as well break it (with the right notices, of course) once.

Since no-one reads notices ahead of time anyway, I think I'd prefer to find out about this by having my build break up front, rather than having the build work but assets mysteriously no longer appear in my app. :)

I'll change this PR to keep the FLX initializer and add an initializer with Flutter assets directory, WDYT?

Sounds good to me.

This will be a breaking change once we stop putting assets in app.flx anyway, so we might as well break it (with the right notices, of course) once.

Since we will be updating the engine and the supporting tooling (that generates the FLX files and configures application launch) in one go, there should be no runtime misbehavior due to the embedder looking for, but not finding the FLX in a particular spot. However, embedders using the old API will break even though those APIs will never be invoked by the application launch mechanism.

Since no-one reads notices ahead of time anyway, I think I'd prefer to find out about this by having my build break up front,

A kinder, more tactful, approach in dealing with this update would be to decorate the old API with NS_DEPRECATED attributes with a clear message regarding the direction we are going to take moving forward and what the user should do to rectify the warning. Adroit users already have warnings set as errors and will be able to handle the update on their own (especially if we indicate that the old API is a no-op that the application launch mechanism wont invoke anyway). Users who are perhaps less familar with the native side of mobile development and build systems wont have their builds break on them due to a routine update.

You mentioned a doc with instructions on how this should be done. Are you able to find it? If not, could you please let me know more specifically how to do the tagging, and the update of the changelog etc. ?

szakarias

I agree that relying on specific names is less transparent and should be avoided when possible, however I am not sure if we can do it here. The tooling on iOS only includes project files listed in the project.pbxproj which is part of the flutter create template. Hence we need a specifically named directory in which to place Flutter artifacts. If you know of a way around this, it would be great.

szakarias · 2017-11-12T19:04:53Z

shell/platform/darwin/ios/framework/Source/FlutterDartSource.h

 @property(nonatomic, readonly) NSURL* dartMain;
 @property(nonatomic, readonly) NSURL* packages;
-@property(nonatomic, readonly) NSURL* flxArchive;
+@property(nonatomic, readonly) NSURL* flutterAssets;


The change above is not in the public API, however the change in FlutterDartProject.h is, and I agree with postponing the breaking change until FLX is completely removed. I'll change this PR to keep the FLX initializer and add an initializer with Flutter assets directory, WDYT?

jakobr-google

LGTM, but defer to @chinmaygarde.

jakobr-google · 2017-11-13T09:42:26Z

runtime/dart_init.cc

+    std::vector<uint8_t>* data,
+    std::unique_ptr<DirectoryAssetBundle>& directory_asset_bundle,
+    fxl::RefPtr<ZipAssetStore>& asset_store) {
+  return (directory_asset_bundle &&


It could probably be split up, but there are no failure conditions as such here. The best we could do would be something like

if (directory_asset_bundle && directory_asset_bundle->GetAsBuffer(name, data)) { return true; } return asset_store && asset_store->GetAsBuffer(name, data);

(I kind of prefer the current version, though, which is copied from engine. :)

jakobr-google · 2017-11-13T09:45:15Z

runtime/dart_init.cc

+          zip_asset_store = fxl::MakeRefCounted<ZipAssetStore>(
+              GetUnzipperProviderForPath(flx_path));
+        }
+        GetAssetAsBuffer(kKernelAssetKey, &kernel_data, directory_asset_,


directory_asset_bundle?

jakobr-google · 2017-11-13T09:57:38Z

shell/platform/android/io/flutter/view/ResourceExtractor.java

+                    }
                    InputStream is = null;
                    OutputStream os = null;
                    try {


Not your code, but if we're on a modern Java version (>=7), we should be able to do

try (InputStream is = manager.open(asset)) { try (OutputStream os = new FileOutputStream(output)) { ... } }

And not have to bother with the nested finally/try/if/finally/... mess.

https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

jakobr-google · 2017-11-13T10:05:14Z

shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm

-    // Default to "app.flx"
-    flxName = @"app";
+- (NSString*)pathForFlutterAssetsFromBundle:(NSBundle*)bundle {
+  NSString* flutterAssetsName = [bundle objectForInfoDictionaryKey:@"FLTFlxName"];


Is this still the right key, or should we use something like FLTAssetsPath?

(The key isn't defined in our template's Info.plist, so I don't know if anyone's actually using this.)

I haven't been able to find any usages. I'll keep it with an updated key name so that it is still configurable in Info.plist.

jakobr-google · 2017-11-13T10:12:55Z

shell/platform/darwin/ios/framework/Source/FlutterDartSource.h

 @property(nonatomic, readonly) NSURL* dartMain;
 @property(nonatomic, readonly) NSURL* packages;
-@property(nonatomic, readonly) NSURL* flxArchive;
+@property(nonatomic, readonly) NSURL* flutterAssets;


This will be a breaking change once we stop putting assets in app.flx anyway, so we might as well break it (with the right notices, of course) once.

Since no-one reads notices ahead of time anyway, I think I'd prefer to find out about this by having my build break up front, rather than having the build work but assets mysteriously no longer appear in my app. :)

chinmaygarde · 2017-11-14T03:07:40Z

Github won't let me reply to some of the comments inline. So adding replies to specific comments here:

I agree that relying on specific names is less transparent and should be avoided when possible, however I am not sure if we can do it here. The tooling on iOS only includes project files listed in the project.pbxproj which is part of the flutter create template. Hence we need a specifically named directory in which to place Flutter artifacts. If you know of a way around this, it would be great.

We already have a hook into the build system via xcode_backend.sh, I wonder if we can use that hook to place assets within the application bundle at very specific points and have those be surfaced up via keys in the Info.plist (we already have keys reserved there). I guess I am worried that we are boxing ourselves in to supporting only one Flutter application per bundle. When, in reality, we can have multiple applications running in multiple views on screen. Having the paths be explicit solves that. I may be overthinking this and the more complex cases can require user interaction via the existing public API. It is fine if you timebox the investigation into using the exiting hook into the build system.

The change above is not in the public API, however the change in FlutterDartProject.h is,

Lets add NS_DEPRECATED attributes with messages that include a link to the changelog to the exiting public API and add a replacement in its place. We can schedule removal of the API in due course. We did the same thing where we removed FlutterMain.

szakarias · 2017-11-15T14:51:18Z

I looked into xcode_backend.sh again and unfortunately I don't think we are able to place assets within the bundle from here. The only way I see is to edit project.pbxproj and then let Xcode include them in the bundle. I've found some third-party libraries for handling this nicely, but in the end it still boils down to changing project.pbxproj.

szakarias · 2017-11-29T11:11:46Z

@chinmaygarde would it be possible for you to have a look at the changes? Maybe we can wait with the deprecation part in order to move this forward as I now have a couple of PRs depending on this.

chinmaygarde · 2017-11-29T22:20:53Z

@szakarias I have drawn up a proposal for how we could manage breaking API changes in the public engine API. To help with the deprecations, a few macros were introduced in this pull request. Can you decorate the deprecated methods with the same and add a note in the changelog as mentioned in the document please?

chinmaygarde · 2017-11-29T23:31:24Z

Users of the public embedder API (non iOS or Android embedders) will break because of the updates in the current version of that patch.

For backward compatibility, I suggest you still attempt to initialize the asset bundle from the path if it is a regular file (you are returning early with a log message currently). If it is a directory, then you can initialize the directory asset bundle and then the zip asset store with an optional app.flx in that directory.

szakarias · 2017-12-11T14:48:34Z

Thanks for the comments! PTAL. Decorated the deprecated methods, and also reverted the change in asset retrieval. We still return assets as byte data instead of the asset paths. I am postponing this to a later change. Updated the flutter PR correspondingly.

jakobr-google

LGTM

jakobr-google · 2017-12-13T08:48:32Z

shell/platform/darwin/ios/framework/Headers/FlutterDartProject.h

+        "This initializer is no longer used since the FLX will be deprecated. "
+        "Instead, use [initWithFlutterAssets].");
+
+- (instancetype)initWithFLXArchiveWithScriptSnapshot:(NSURL*)flutterAssetsURL


Nit: Shouldn't it still be archiveURL here?

Requested changes were made.

aam · 2017-12-13T17:33:27Z

@szakarias have you tried running flutter examples with these changes?
I am seeing blank white screen when starting flutter gallery with engine past 28bc7b1 commit.

aam · 2017-12-13T17:46:04Z

cc'ing @crelier who is doing dart-engine-flutter roll today as this is potential blocker for flutter engine to flutter tools roll.

crelier · 2017-12-13T17:49:38Z

@aam, it is my turn to do a Dart roll, but the flutter gallery is not running correctly:

regis@belenos:~/flutter/examples/flutter_gallery$ flutter run --release --local-engine=android_release
Running "flutter packages get" in flutter_gallery... 5.4s
Initializing gradle... 1.1s
Resolving dependencies... 46.3s
Launching lib/main.dart on Nexus 4 in release mode...
Running 'gradlew assembleRelease'...
Dart snapshot generator failed with exit code 253
Error: Cannot find entry point dart:ui :: _getLocaleClosure

FAILURE: Build failed with an exception.

Where:
Script '/usr/local/google/home/regis/flutter/packages/flutter_tools/gradle/flutter.gradle' line: 362
What went wrong:
Execution failed for task ':app:flutterDependenciesRelease'.

Process 'command '/usr/local/google/home/regis/flutter/bin/flutter'' finished with non-zero exit value 1

Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
Gradle build failed: 1

aam · 2017-12-13T17:55:08Z

Yes @crelier , I would try to verify that at 28bc7b1 of flutter engine things are looking good(they did for me, but I only looked at debug) and then unless there is a response from @szakarias rollback three seemingly related commits by @szakarias from earlier this morning to get back to that 28bc7b1, roll from there.

crelier · 2017-12-13T18:02:26Z

@aam, a debug run is failing too. So I'll wait until this gets fix to continue with the dart roll.

jakobr-google · 2017-12-13T18:41:39Z

I believe @szakarias has two related PRs on flutter/flutter that might need to go in with the engine roll. Could you see if applying flutter/flutter#12944 (and maybe also flutter/flutter#13011) fix the blank screen issue?

The snapshotter issue looks unrelated, but I don't know.

szakarias · 2017-12-13T23:13:03Z

flutter/flutter#12944 and flutter/flutter#13011 are merged now.

tvolkert · 2017-12-21T04:38:54Z

shell/platform/android/io/flutter/view/FlutterMain.java

        FlutterMain.class.getName() + '.' + FLX_KEY;
    public static final String PUBLIC_SNAPSHOT_BLOB_KEY =
        FlutterMain.class.getName() + '.' + SNAPSHOT_BLOB_KEY;
+    public static final String PUBLIC_FLUTTER_ASSETS_DIR_KEY =


It looks like this was never wired up in initConfig(), so even if apps specify this key in their manifest, we're still looking in the default assets dir.

Ah yes, thanks!

googlebot added the cla: yes label Nov 9, 2017

szakarias requested a review from jakobr-google November 9, 2017 14:13

szakarias mentioned this pull request Nov 9, 2017

Include a directory with Flutter assets flutter/flutter#12944

Merged

szakarias force-pushed the flutterAssets branch from 76ac034 to 48ecfbd Compare November 9, 2017 14:43

chinmaygarde previously requested changes Nov 9, 2017

View reviewed changes

szakarias commented Nov 12, 2017

View reviewed changes

jakobr-google reviewed Nov 13, 2017

View reviewed changes

szakarias force-pushed the flutterAssets branch 2 times, most recently from 18c6dbd to cc66d15 Compare November 15, 2017 14:15

This was referenced Nov 20, 2017

Select fonts from asset directory instead of FLX. #4376

Closed

Read platform kernel blob from asset directory instead of FLX #4385

Closed

szakarias force-pushed the flutterAssets branch 2 times, most recently from f4af7a0 to 3126059 Compare November 23, 2017 14:44

szakarias force-pushed the flutterAssets branch 2 times, most recently from e9b9e08 to 838bbaf Compare December 11, 2017 14:12

jakobr-google approved these changes Dec 13, 2017

View reviewed changes

szakarias added 5 commits December 13, 2017 10:16

Handle Flutter assets outside FLX

6d617bd

comments

a720c44

fix bug after rebase

834b85c

Expect engine to be called with Flutter asset dir

2efa7f0

Added deprecated attributes and reverted asset path change

7e1815b

szakarias force-pushed the flutterAssets branch from 838bbaf to d4a30e0 Compare December 13, 2017 09:16

parameter name

ece8080

szakarias force-pushed the flutterAssets branch from d4a30e0 to ece8080 Compare December 13, 2017 09:42

szakarias merged commit 4cbe26d into flutter:master Dec 13, 2017

aam mentioned this pull request Dec 13, 2017

Fix Platform.localeName by setting _Platform._localeClosure #4450

Merged

tvolkert reviewed Dec 21, 2017

View reviewed changes

Handle Flutter assets outside FLX #4343

Handle Flutter assets outside FLX #4343

Uh oh!

Conversation

szakarias commented Nov 9, 2017

Uh oh!

szakarias commented Nov 9, 2017

Uh oh!

chinmaygarde commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szakarias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobr-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Nov 14, 2017

Uh oh!

szakarias commented Nov 15, 2017

Uh oh!

szakarias commented Nov 29, 2017

Uh oh!

chinmaygarde commented Nov 29, 2017

Uh oh!

chinmaygarde commented Nov 29, 2017

Uh oh!

szakarias commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobr-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aam commented Dec 13, 2017

Uh oh!

aam commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crelier commented Dec 13, 2017

Uh oh!

aam commented Dec 13, 2017

Uh oh!

crelier commented Dec 13, 2017

Uh oh!

jakobr-google commented Dec 13, 2017

Uh oh!

szakarias commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

szakarias commented Dec 11, 2017 •

edited

Loading

aam commented Dec 13, 2017 •

edited

Loading