Make FLTEnableImpeller to be configurable in the Flutter bundle

[chingjun]

Currently we only accept the flag when it is in the info.plist file in the main bundle. The behavior this flag controls lives entirely within the Flutter bundle, so it seems natural to me that we should accept the flag in the Flutter bundle, like FLTAssetsPath. (Also it makes our internal infra easier by giving us more control.)

[jonahwilliams]

This came up with b/274227975 and may also impact add2app scenarios

[jonahwilliams]

FYI @zanderso

[zanderso]

This is probably like a 4-5 character fix, but for testing I think I need to add something like https://github.com/flutter/engine/blob/main/testing/scenario_app/ios/AppFrameworkInfo.plist to https://github.com/flutter/engine/tree/main/testing/ios/IosUnitTests, so that I can modify the test at https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterDartProjectTest.mm#L76 to try the lookup in the Flutter bundle.

I suspect that @jmagman might be able to give me a hint, or let me know if I'm on the wrong track.

[jmagman]

Something like this I think:

Hoist applicationBundle App.framework local variable up:
https://github.com/flutter/engine/blob/004d841b7efde6ae26cefa4c3ad5a4ad07cf644c/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm#L132

+  NSURL* applicationFrameworkURL =
+  [mainBundle.privateFrameworksURL URLByAppendingPathComponent:@"App.framework"
+                                                     isDirectory:YES];
+  NSBundle* applicationBundle = [NSBundle bundleWithURL:applicationFrameworkURL];
  if (flutter::DartVM::IsRunningPrecompiledCode()) {

https://github.com/flutter/engine/blob/004d841b7efde6ae26cefa4c3ad5a4ad07cf644c/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm#L154-L159

    if (settings.application_library_path.empty()) {
-     NSString* applicationFrameworkPath = [mainBundle pathForResource:@"Frameworks/App.framework"
-                                                               ofType:@""];
-     if (applicationFrameworkPath.length > 0) {
-       NSString* executablePath =
-           [NSBundle bundleWithPath:applicationFrameworkPath].executablePath;
+     if (applicationBundle != nil) {
+       NSString* executablePath = applicationBundle.executablePath;

https://github.com/flutter/engine/blob/004d841b7efde6ae26cefa4c3ad5a4ad07cf644c/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm#L210-L212

Then use it to check its Info.plist:

-  NSNumber* enableImpeller = [mainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"];
+  NSNumber* enableImpellerMainBundle = [mainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"];
+  NSNumber* enableImpeller =
+      enableImpellerMainBundle != nil
+          ? enableImpellerMainBundle
+          : [applicationBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"];
  // Change the default only if the option is present.
  if (enableImpeller != nil) {

There doesn't seem to be a test that checks if FLTEnableImpeller is YES, only a test that checks if it's NO (testEnableImpellerSettingIsCorrectlyParsed @zanderso pointed out).

[zanderso]

@jmagman That looks a bit more robust than what I was thinking. I might have misunderstood that the code here:

https://github.com/flutter/engine/blob/e159e35fb29ca1ca29581d4f1c5124f4abd3b29d/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm#L93

Would find the Info.plist for App.framework, but I guess that only works if it has the "io.flutter.flutter.app" bundle identifier, which I guess isn't guaranteed?

Anyway, it seems like one of these ways will probably work, so what I'm wondering is whether there's a way to add an App.framework like thing to these unit tests https://github.com/flutter/engine/tree/main/testing/ios/IosUnitTests, so that I can test looking up the flag in the app bundle in a new test alongside testEnableImpellerSettingIsCorrectlyParsed.

[jmagman]

Would find the Info.plist for App.framework, but I guess that only works if it has the "io.flutter.flutter.app" bundle identifier, which I guess isn't guaranteed?

I think lots of things would break if people changed that ID, so I think you're right you can use that bundle local (I didn't spot it, looks like in that other spot it's using a different way to find the same bundle.

what I'm wondering is whether there's a way to add an App.framework like thing to these unit tests https://github.com/flutter/engine/tree/main/testing/ios/IosUnitTests, so that I can test looking up the flag in the app bundle in a new test alongside testEnableImpellerSettingIsCorrectlyParsed.

I think you should be able to mock it out like this:
https://github.com/flutter/engine/blob/e159e35fb29ca1ca29581d4f1c5124f4abd3b29d/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm#L305-L307

Do you want me to try to get that working? I didn't when I saw it's assigned to @jonahwilliams but it seems like you're trying to solve it too 🙂

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.