Add observatory Bonjour service to built iOS Info.plist bundle #64988

jmagman · 2020-09-01T00:38:54Z

Description

Add NSBonjourServices _dartobservatory._tcp and NSLocalNetworkUsageDescription to debug and profile Info.plist in the produced, bundled .app (not the user-facing Info.plist created in the template and included in Xcode).

This prompts the "Your app would like to find and connect to devices on your local network" permission popup, which is required to unblock the observatory publisher on iOS 14.

NSLocalNetworkUsageDescription is underneath:

Note: this does not add the keys for add-to-app host apps, so they still have the issue of blocking the observatory publisher.

Related Issues

Fixes #60634
Fixes #35707.
Related to conversation at #63893, but that is for add-to-app.

Tests

Updated ios_content_validation_test to validate Release does not have the keys, but Debug does.

Kudos

Thanks to @gaaclarke for suggesting to just add the keys in the generated bundle Info.plist, and not the one that's created in the template. That avoided a huge migration headache.

Checklist

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 read the [Tree Hygiene] wiki page, which explains my responsibilities.
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

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.

jmagman · 2020-09-01T00:39:38Z

dev/devicelab/bin/tasks/ios_content_validation_test.dart

        }
-      });

-      if (foundProjectName) {


Moved these failures to where they are detected, instead of the end of the test.

jmagman · 2020-09-01T00:40:10Z

packages/flutter_tools/bin/xcode_backend.sh

+      EchoError "========================================================================"
+      exit -1;;
+  esac
+  retval="${build_mode}"


This works, but is there a more idiomatic way to return a string from a bash function?

https://stackoverflow.com/questions/17336915/return-value-in-a-bash-function probably

jmagman · 2020-09-01T00:40:29Z

packages/flutter_tools/bin/xcode_backend.sh


  local bitcode_flag=""
-  if [[ $ENABLE_BITCODE == "YES" ]]; then
+  if [[ "$ENABLE_BITCODE" == "YES" ]]; then


Unrelated to this change, but I noticed the quotes were missing.

jmagman · 2020-09-01T00:40:56Z

packages/flutter_tools/bin/xcode_backend.sh

+    "observatory_bonjour_service")
+      AddObservatoryBonjourService ;;


This is a new xcode_backend entrypoint. At the least, we can tell add-to-app customers to call this in a new build script phase.

I think I would rather not add this for add-to-app.

Those customers should be sophisticated enough to read an error log and know which plist(s) to update.

This is likely to get things wrong for some customer and we'll get requests to change it or a specific CI system or wahtever.

What about .frameworks? Will those cases never work unless users type in something manually in their real checked-in info.plist?

Those customers should be sophisticated enough to read an error log and know which plist(s) to update.

How would they see an error message from the VM if they can't attach? Edit: oh right, they would see it in Xcode.
It definitely requires some Xcode-foo to know you can edit the Info.plist per configuration. You need to know how to expand the build setting, which people seem confused by.

Or they can do something like Info-${CONFIGURATION}.plist
We'll have to update the docs, for sure.

Will those cases never work unless users type in something manually in their real checked-in info.plist?

Right, they would need to edit their plist.

At least in the CocoaPods case they can add the build phase I suggested in #63893 (comment)

jonahwilliams · 2020-09-01T00:56:59Z

Why do this in bash and not in dart?

jmagman · 2020-09-01T00:57:32Z

packages/flutter_tools/bin/xcode_backend.sh

+  # Don't override the local network description the Flutter app developer specified (uncommon).
+  # This text will appear below the "Your app would like to find and connect to devices on your local network" permissions popup.
+  if ! plutil -extract NSLocalNetworkUsageDescription xml1 -o - "${built_products_plist}"; then
+    RunCommand plutil -insert NSLocalNetworkUsageDescription -string "Allow Flutter tools on your computer to connect and debug your Flutter application. This prompt will not appear on release builds." "${built_products_plist}"


Check out the screenshot in the description. We can probably remove the second "Flutter". Maybe also "connect"?

Allow Flutter tools on your computer to debug your application. This prompt will not appear on release builds.

On the second connect. I'd maybe leave it. Otherwise people might think I'm just flutter running and then maybe taking a screenshot or something. I'm not technically "debugging", why doesn't it work?

On the second Flutter, don't have a strong opinion. To be specific, we're not really debugging their (objective-c) application. This is strictly dart/observatory/flutter stuff.

jmagman · 2020-09-01T00:58:47Z

dev/devicelab/bin/tasks/ios_content_validation_test.dart

+          await flutter('clean');
+        });
+
+        section('Validate debug contents');


This section is new.

jmagman · 2020-09-01T00:59:15Z

dev/devicelab/lib/framework/ios.dart

+      ),
+    ],
+    canFail: true,
+  ) == 0;


Don't really care what it says since that seems like a moving target, just that it exists.

jmagman · 2020-09-01T01:57:41Z

Why do this in bash and not in dart?

@jonahwilliams We don't do any of the post-assemble app bundling in dart (embedding frameworks, codesigning, thinning). Are you suggesting a new flutter command? Or a new xcode_backend.dart script maybe?

jmagman · 2020-09-01T03:23:29Z

packages/flutter_tools/test/integration.shard/xcode_backend_test.dart

@@ -0,0 +1,177 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.


This was moved from general.shard, it's not a hermetic test.

jmagman · 2020-09-01T03:24:01Z

packages/flutter_tools/test/integration.shard/xcode_backend_test.dart

+    await expectXcodeBackendFails(unknownFlutterBuildMode);
+    await expectXcodeBackendFails(localEngineDebugBuildModeRelease);
+    await expectXcodeBackendFails(localEngineProfileBuildeModeRelease);
+  });


These were disabled in #35707, but as an integration test they should get a precache, so I fixed them up and reenabled.

jmagman · 2020-09-01T03:25:34Z

packages/flutter_tools/test/integration.shard/xcode_backend_test.dart

+    await expectXcodeBackendFails(localEngineProfileBuildeModeRelease);
+  });
+
+  test('Xcode backend warns archiving a non-release build.', () async {


This one is new, the behavior changed #57688 but the installWithoutRelease test was being skipped so it didn't fail.

jonahwilliams · 2020-09-01T03:40:04Z

packages/flutter_tools/bin/xcode_backend.sh

+  if [[ "${build_mode}" == "release" ]]; then
+    return
+  fi
+  local built_products_plist="${BUILT_PRODUCTS_DIR}/${INFOPLIST_PATH}"


I might be misreading this a bit, isn't this ultimately the same plist as https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/build_system/targets/ios.dart#L341 ?

It's not the same file, that's the project's ios/Runner/Info.plist (build setting INFOPLIST_FILE), and that's checked into source code, and where the docs tell users to update some fields before they submit to the app store.

This PR is editing the Xcode-generated Info.plist that gets embedded into the Runner.app.
After Xcode compiles everything and it embeds resources (storyboards, images, etc), it creates a Runner.app/Info.plist, which is a combination what's in the user's Info.plist in Xcode, plus a bunch of other Xcode-generated keys and metadata

By editing this final generated Info.plist, we don't need to update the user's checked in Info.plist, update the template, or have a complicated migration where we set different Info.plists depending on configuration, which would cause flavor-users a bunch of pain. We know the user needs this service in debug and profile when copied to the device, and it also means it won't slip into the release version that gets submitted to the App Store.

Actually, looking again, https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/build_system/targets/ios.dart#L341 is a different third Info.plist. It's the one that gets embedded in the App.framework.

Does it matter which Plist these permissions are added to, or could it be done to the flutter framework plist itself?

It needs to be on the app bundle, so Runner.app/Info.plist. A framework can't set app-level permission requests or entitlements. Otherwise this problem would be a whole lot simpler.

understood.

dnfield · 2020-09-01T04:47:02Z

For the add to app case, can't we just log an appropriate error with instructions on how to update the plist in FlutterObservatoryPublisher.mm if the returncode is kDnsServiceErr_NoAuth?

xster

LGTM. Though I'd like to figure out a workaround plan for add-to-app.

xster · 2020-09-01T06:02:01Z

dev/devicelab/lib/framework/ios.dart

+
+Future<bool> localNetworkUsageFound(String appBundlePath) async =>
+  await exec(
+    'plutil',


oh lol, TIL. So there's an actual built-in tool to turn plists to things like json so it can be more easily consumed by things like dart?

Yup, it's being used for other plist parsing, see

flutter/packages/flutter_tools/lib/src/ios/plist_parser.dart

Line 53 in 80619f1

executable, '-convert', 'json', '-o', '-', normalizedPlistPath,

Except the json part doesn't totally work... #62160

oh lol, sad

xster · 2020-09-01T06:03:24Z

packages/flutter_tools/bin/xcode_backend.sh

 }

+ParseFlutterBuildMode() {
+  # Use FLUTTER_BUILD_MODE if it's set, otherwise use the Xcode build configuration name


oh cool, is this a whole new feature (that confused matt forever) bundled here? :D

This has been in there for .. a while now.

Basically, we try to use the $CONFIGURATION because that's probably what the user wants if they're running from Xcode (in particular, we don't want instruments tests from debug mode Dart, and we set up instruments to run in a "PROFILE" configuration by default).

But we do let a user override it if necessary.

Yeah this logic and error messaging was just moved into a common function so I could use it in two places.

we should go have Matt write this down in https://flutter.dev/docs/deployment/flavors :D

xster · 2020-09-01T06:04:37Z

packages/flutter_tools/bin/xcode_backend.sh

+      EchoError "ERROR: Unknown FLUTTER_BUILD_MODE: ${build_mode}."
+      EchoError "Valid values are 'Debug', 'Profile', or 'Release' (case insensitive)."
+      EchoError "This is controlled by the FLUTTER_BUILD_MODE environment variable."
+      EchoError "If that is not set, the CONFIGURATION environment variable is used."


Also add a common name (as seen in the xcode gui) for this? People might not know what the CONFIGURATION setting maps to.

This was all just moved.

xster · 2020-09-01T06:05:51Z

packages/flutter_tools/bin/xcode_backend.sh

+      EchoError ""
+      EchoError "You can fix this by either adding an appropriately named build"
+      EchoError "configuration, or adding an appropriate value for FLUTTER_BUILD_MODE to the"
+      EchoError ".xcconfig file for the current build configuration (${CONFIGURATION})."


Also add a gui description if possible (I assume people can go into the build settings tab, add a new row, expand the configurations and type in something in xcode).

CONFIGURATION is an env var provided by Xcode. It's available in the build settings if you dig into them. There's no special GUI name.

By default anXcode project has Debug and Release. We added profile to our default template when this was added.

xster · 2020-09-01T06:07:20Z

packages/flutter_tools/bin/xcode_backend.sh

    RunCommand codesign --force --verbose --sign "${EXPANDED_CODE_SIGN_IDENTITY}" -- "${xcode_frameworks_dir}/Flutter.framework/Flutter"
  fi
+
+  AddObservatoryBonjourService


How come this works here and not for add-to-app? Because our template build phase puts xcode_backend.sh later than where cocoapods can help us put it?

Because our template build phase puts xcode_backend.sh later than where cocoapods can help us put it?

Bingo. I put some more details in #63893 (comment).

xster · 2020-09-01T06:08:44Z

packages/flutter_tools/bin/xcode_backend.sh

+
+  # Don't override the local network description the Flutter app developer specified (uncommon).
+  # This text will appear below the "Your app would like to find and connect to devices on your local network" permissions popup.
+  if ! plutil -extract NSLocalNetworkUsageDescription xml1 -o - "${built_products_plist}"; then


Display an error message otherwise?

If the developer sets their own NSLocalNetworkUsageDescription because their app is doing their own local networking we don't override it.

that's ok. I just meant if that's the case, we should print something in the console to indicate that you'll get a popup in debug and we can't set the message in it but it's for flutter tools to connect.

xster · 2020-09-01T06:10:45Z

packages/flutter_tools/bin/xcode_backend.sh

+  # Don't override the local network description the Flutter app developer specified (uncommon).
+  # This text will appear below the "Your app would like to find and connect to devices on your local network" permissions popup.
+  if ! plutil -extract NSLocalNetworkUsageDescription xml1 -o - "${built_products_plist}"; then
+    RunCommand plutil -insert NSLocalNetworkUsageDescription -string "Allow Flutter tools on your computer to connect and debug your Flutter application. This prompt will not appear on release builds." "${built_products_plist}"


On the second connect. I'd maybe leave it. Otherwise people might think I'm just flutter running and then maybe taking a screenshot or something. I'm not technically "debugging", why doesn't it work?

On the second Flutter, don't have a strong opinion. To be specific, we're not really debugging their (objective-c) application. This is strictly dart/observatory/flutter stuff.

xster · 2020-09-01T06:12:20Z

packages/flutter_tools/bin/xcode_backend.sh

+    "observatory_bonjour_service")
+      AddObservatoryBonjourService ;;


What about .frameworks? Will those cases never work unless users type in something manually in their real checked-in info.plist?

dnfield

The more I think about this, the more I'm opposed to adding more "API surface" to our dirty little bash script :)

Is there some way we could avoid having a new "method" on there that we tell people to call?

jmagman · 2020-09-01T07:19:46Z

For the add to app case, can't we just log an appropriate error with instructions on how to update the plist in FlutterObservatoryPublisher.mm if the returncode is kDnsServiceErr_NoAuth?

That's definitely a good idea.

jmagman · 2020-09-01T07:24:05Z

The more I think about this, the more I'm opposed to adding more "API surface" to our dirty little bash script :)

Is there some way we could avoid having a new "method" on there that we tell people to call?

If we decide not to allow the workaround build script I suggested at the end of #63893 (comment), we can remove this "method" but still keep the plutil logic in EmbedFlutterFrameworks.

xster · 2020-09-01T07:30:44Z

The more I think about this, the more I'm opposed to adding more "API surface" to our dirty little bash script :)

How is this different from us having a bunch of methods in our podhelper.rb we ask people to call?

I think having dart files xcode phases calling into would be cooler but I don't know if this is particularly bad.

Maybe your comment is just saying inline it into the existing function body which is fine too.

gaaclarke

Too many cooks in the kitchen so I just gave it a quick glance through with a couple of notes. I raised a question about a tweak to this approach that might be better suited for add-to-app here: #63893 (comment)

gaaclarke · 2020-09-01T17:22:49Z

dev/devicelab/lib/framework/ios.dart

  return !emptyBitcodeMarkerFound;
 }

+Future<bool> bonjourServiceFound(String appBundlePath) async =>


Can we call this dartObservatoryBonjourServiceFound? or take in _dartobservatory._tcp as a parameter? From the callsite it just looks like we are throwing if any bonjour service is found.

gaaclarke · 2020-09-01T17:25:47Z

dev/devicelab/bin/tasks/ios_content_validation_test.dart

+        if (await bonjourServiceFound(outputAppPath)) {
+          throw TaskResult.failure('Release bundle has unexpected NSBonjourServices');
+        }
+        if (await localNetworkUsageFound(outputAppPath)) {


Is this throwing an error if a user legitimately has their own mdns service for their release build? We want to make sure to support that.

This is an integration test, it doesn't have any mDNS service set. The xcode_build script handles that case, it detects if the NSBounjourServices key is set, and adds it to the list.

jonahwilliams

LGTM

Good that we have test coverage for the new stuff, even though it is bash I think that should help substantially with the maintainability.

jmagman · 2020-09-02T02:07:39Z

The more I think about this, the more I'm opposed to adding more "API surface" to our dirty little bash script :)

Is there some way we could avoid having a new "method" on there that we tell people to call?

@dnfield I kept the endpoint for the integration tests but renamed to test_observatory_bonjour_service with a comment that you're a jerk if you try to actually use it.

dnfield

LGTM

#64988)" This reverts commit 4fde217.

#64988)" (#65109) This reverts commit 4fde217.

…st bundle (#64988)" (#65109)" This reverts commit cb81950.

…er#64988)

flutter#64988)" (flutter#65109) This reverts commit 4fde217.

Add observatory Bonjour service to Info.plist

e5afee2

jmagman added platform-ios iOS applications specifically t: xcode "xcodebuild" on iOS and general Xcode project management labels Sep 1, 2020

jmagman requested review from dnfield, gaaclarke and xster September 1, 2020 00:38

jmagman self-assigned this Sep 1, 2020

flutter-dashboard bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Sep 1, 2020

googlebot added the cla: yes label Sep 1, 2020

jmagman commented Sep 1, 2020

View reviewed changes

jmagman mentioned this pull request Sep 1, 2020

flutter attach for add-to-app modules doesn't work on iOS 14, document how to add _dartobservatory._tcp to host app #63893

Closed

jmagman commented Sep 1, 2020

View reviewed changes

Add xcode_backend_tests

612818e

jmagman force-pushed the bonjour-service branch from 6a4cbd0 to 612818e Compare September 1, 2020 03:22

jmagman commented Sep 1, 2020

View reviewed changes

jonahwilliams reviewed Sep 1, 2020

View reviewed changes

xster reviewed Sep 1, 2020

View reviewed changes

dnfield suggested changes Sep 1, 2020

View reviewed changes

gaaclarke reviewed Sep 1, 2020

View reviewed changes

jonahwilliams approved these changes Sep 1, 2020

View reviewed changes

Review edits

af636eb

xster approved these changes Sep 2, 2020

View reviewed changes

dnfield approved these changes Sep 2, 2020

View reviewed changes

jmagman merged commit 4fde217 into flutter:master Sep 2, 2020

jmagman deleted the bonjour-service branch September 2, 2020 18:11

jmagman added a commit that referenced this pull request Sep 2, 2020

Revert "Add observatory Bonjour service to built iOS Info.plist bundle (

b1df0a2

#64988)" This reverts commit 4fde217.

jmagman mentioned this pull request Sep 2, 2020

Revert "Add observatory Bonjour service to built iOS Info.plist bundle" #65109

Merged

jmagman added a commit that referenced this pull request Sep 2, 2020

Revert "Add observatory Bonjour service to built iOS Info.plist bundle (

cb81950

#64988)" (#65109) This reverts commit 4fde217.

jmagman added a commit that referenced this pull request Sep 2, 2020

Revert "Revert "Add observatory Bonjour service to built iOS Info.pli…

2a9c373

…st bundle (#64988)" (#65109)" This reverts commit cb81950.

jmagman mentioned this pull request Sep 2, 2020

Require xcode_backend.sh arguments (introduced in Flutter v0.0.7) #65124

Merged

10 tasks

jmagman added a commit to jmagman/flutter that referenced this pull request Sep 3, 2020

Add observatory Bonjour service to built iOS Info.plist bundle (flutt…

f8aedfb

…er#64988)

jmagman mentioned this pull request Sep 3, 2020

Add observatory Bonjour service to built iOS Info.plist bundle #65138

Merged

10 tasks

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Add observatory Bonjour service to built iOS Info.plist bundle (flutt…

d35e62a

…er#64988)

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Revert "Add observatory Bonjour service to built iOS Info.plist bundle (

5d96101

flutter#64988)" (flutter#65109) This reverts commit 4fde217.

jmagman mentioned this pull request Sep 26, 2020

Add to app iOS 14 Info.plist network permissions flutter/samples#554

Merged

jmagman mentioned this pull request Mar 13, 2021

Allow local networking Info.plist insertion to fail #78085

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2021

		"observatory_bonjour_service")
		AddObservatoryBonjourService ;;

		@@ -0,0 +1,177 @@
		// Copyright 2014 The Flutter Authors. All rights reserved.

Add observatory Bonjour service to built iOS Info.plist bundle #64988

Add observatory Bonjour service to built iOS Info.plist bundle #64988

Uh oh!

Conversation

jmagman commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Kudos

Checklist

Breaking Change

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!

jmagman Sep 1, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 1, 2020 • 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!

jonahwilliams commented Sep 1, 2020

Uh oh!

jmagman Sep 1, 2020 • 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!

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!

jmagman commented Sep 1, 2020

Uh oh!

jmagman Sep 1, 2020 • 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!

jmagman Sep 1, 2020 • 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!

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!

dnfield commented Sep 1, 2020

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 1, 2020 • 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!

jmagman commented Sep 1, 2020 •

edited

Loading

jmagman Sep 1, 2020 •

edited

Loading

jmagman Sep 1, 2020 •

edited

Loading

jmagman Sep 1, 2020 •

edited

Loading

jmagman Sep 1, 2020 •

edited

Loading

jmagman Sep 1, 2020 •

edited

Loading

jmagman Sep 1, 2020 •

edited

Loading