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.

Sign up for GitHub

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

Jump to bottom

[google_maps_flutter] Add examples for different iOS versions #3757

Merged
merged 17 commits into from
Apr 20, 2023
Merged

[google_maps_flutter] Add examples for different iOS versions #3757

merged 17 commits into from
Apr 20, 2023

Conversation

stuartmorgan
Copy link
Contributor

Improves the way we specify and test the GoogleMaps SDK dependency on iOS. Currently as discussed in flutter/flutter#86820, we are allowing any version of the SDK, and CocoaPods will choose the best version that's compatible with the minimum iOS deployment version of the plugin client's app. However, clients are also subject to future breakage, since any new major SDK version could have compile-time incompatibility with the plugin's wrapping code. We also have no ability to test anything in CI that's newer than the SDK that supports our oldest supported version (currently that's iOS 11, and thus 5.x).

This makes two changes:

  • Replaces the single example with N examples, each with a different minimum iOS version, corresponding to each point where the Google Maps SDK has bumped its minimum iOS requirement. Currently (as documented in the new README file)
    • The oldest example has all of the current testing.
    • The newest example has just XCTests (not integration or XCUITests).
    • Intervening examples have no tests.
  • Instead of the completely unpinned version, we allow anything up to the next major version (which should be the first one with breaking API changes).

This gives us build coverage of each of the (latest at CI runtime) resolve points that clients can have. We retain the flexibility of allowing clients to get the best version that meets their app's constraints, except that we will need to explicitly test and enable new major versions, so that they won't randomly break clients.

Fixes flutter/flutter#86820

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@stuartmorgan stuartmorgan requested a review from jmagman April 18, 2023 18:46
@stuartmorgan stuartmorgan requested a review from cyanglaz as a code owner April 18, 2023 18:46
@stuartmorgan
Copy link
Contributor Author

This looks like a huge review, but the ios12/ and ios13/ folders are just exact copies of the ios11 folder with:

  • the minimum deployment version changed, and
  • most of the tests removed.

cyanglaz
cyanglaz approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM

jmagman
jmagman reviewed Apr 19, 2023
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

  1. How is this building/running tests? Or is it? The flutter tool doesn't know anything about an ios11 directory but would be looking for an ios directory.
  2. Is there a way we can do this that just updates the minimum version instead of duplicating everything needed for the platform directory? For example, keep the single ios directory but have 3 app targets with different versions? Or set an environment variable the Podfile can parse like (I didn't test this):
min_ios_version = ENV["MINIMUM_IOS_VERSION"] || '11.0'
platform :ios, min_ios_version

plus maybe an xcconfig that can be set for the IPHONEOS_DEPLOYMENT_TARGET?

packages/google_maps_flutter/google_maps_flutter_ios/example/ios11/README.md Outdated Show resolved Hide resolved
@stuartmorgan
Copy link
Contributor Author

  1. How is this building/running tests? Or is it? The flutter tool doesn't know anything about an ios11 directory but would be looking for an ios directory.

ios11 isn't a platform directory, it's a whole project directory. This works because the repo tooling is already set up to understand two different structures for examples:

  • example/, a Flutter app project
  • example/, a directory containing any number of directories with arbitrary names which are themselves distinct example projects

The tooling sees that there are three examples, and reacts accordingly. For build-examples it builds all three. For drive-examples it runs the one with integration tests and skips the others (and passes because the tooling just requires that there be at least one example with integration tests, not that all of them have one). For native-test it runs the ones with native tests (currently ios11 and ios13), and skips the one without any test targets.

  1. Is there a way we can do this that just updates the minimum version instead of duplicating everything needed for the platform directory? For example, keep the single ios directory but have 3 app targets with different versions? Or set an environment variable the Podfile can parse like (I didn't test this):
min_ios_version = ENV["MINIMUM_IOS_VERSION"] || '11.0'
platform :ios, min_ios_version

plus maybe an xcconfig that can be set for the IPHONEOS_DEPLOYMENT_TARGET?

We'd have to build bespoke tooling that knew how to set all of this up, and also run different sets of tests for each scenario (e.g., we don't want to run integration tests three times because it would add a bunch of CI time for minimal benefit). It's doable, but it seems like a lot more trouble than just having several copies of files that are almost all just boilerplate.

I could extract the Dart for the example to a shared path-based package, so we don't have three copies of that. That would be easy, and since that part isn't boilerplate it would reduce maintenance if we need to update the examples. I was going back and forth on it because it'll make the structure a little weirder, but it's probably better on the whole to have the one copy.

@stuartmorgan
Copy link
Contributor Author

I could extract the Dart for the example to a shared path-based package, so we don't have three copies of that. That would be easy, and since that part isn't boilerplate it would reduce maintenance if we need to update the examples. I was going back and forth on it because it'll make the structure a little weirder, but it's probably better on the whole to have the one copy.

The Dart code is now shared, so what's copied is almost all flutter create boilerplate now.

jmagman
jmagman approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation @stuartmorgan, and for getting these tested. LGTM

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 19, 2023
@stuartmorgan stuartmorgan force-pushed the maps-ios-multi-os-examples branch from 8244aa1 to f8ae53f Compare April 19, 2023 23:55
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 19, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 19, 2023

auto label is removed for flutter/packages, pr: 3757, due to - The status or check suite android-platform_tests CHANNEL:master PACKAGE_SHARDING:--shardIndex 0 --shardCount 7 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 19, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 20, 2023

auto label is removed for flutter/packages, pr: 3757, due to - The status or check suite android-platform_tests CHANNEL:master PACKAGE_SHARDING:--shardIndex 1 --shardCount 7 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2023
@auto-submit auto-submit bot merged commit 7e3f5da into flutter:main Apr 20, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 21, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 21, 2023
@engine-flutter-autoroll
7e3f5da [google_maps_flutter] Add examples for different iOS versions (…
6eedfca 
…flutter/packages#3757)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 21, 2023
@engine-flutter-autoroll
Roll Packages from 746750e to 7e3f5da (1 revision) (#125301)
c183885 
flutter/packages@746750e...7e3f5da

2023-04-20 stuartmorgan@google.com [google_maps_flutter] Add examples for different iOS versions (flutter/packages#3757)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
pro100andrey added a commit to pro100andrey/packages that referenced this pull request Apr 25, 2023
@pro100andrey
Merge branch 'main' into feature/fix
1263b0f 
* main: (144 commits)
  Update test golden images for the latest Skia roll (flutter#3787)
  [various] Adds Android namespace (flutter#3791)
  [shared_preferences] Update gradle/agp in example apps (flutter#3809)
  [go_router] Adds name to TypedGoRoute (flutter#3702)
  [webview_flutter] Adds support to receive permission requests (flutter#3543)
  [google_sign_in] Fix Android Java warnings (flutter#3762)
  [local_auth] Fix enum return on Android (flutter#3780)
  [pigeon] Warn when trying to use enums in collections (flutter#3782)
  [webview_flutter_android] [webview_flutter_wkwebview] Platform implementations for supporting permission requests (flutter#3792)
  [pigeon] Update for compatibility with a future change to the analyzer. (flutter#3789)
  [camera_android] Fix Android lint warnings  (flutter#3716)
  [webview_flutter_platform_interface] Adds method to receive permission requests (flutter#3767)
  [image_picker_ios] In unit test write and read kCGImagePropertyExifUserComment property (flutter#3783)
  [go_router_builder] Fixed the return value of the generated push method (flutter#3650)
  [image_picker] Mention `launchMode: singleInstance` in README (flutter#3759)
  Revert "[pigeon] Add an initial example app" (flutter#3785)
  [google_maps_flutter] Add examples for different iOS versions (flutter#3757)
  [pigeon] Add an initial example app (flutter#3761)
  [google_maps_flutter_web] Allow marker position updates (flutter#3697)
  [Tool] [Code Excerpt] allow excerpts in example readme (flutter#3758)
  ...

# Conflicts:
#	packages/camera/camera/pubspec.yaml
@reidbaker reidbaker mentioned this pull request Apr 25, 2023
Merged
11 tasks
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
@stuartmorgan @nploi
[google_maps_flutter] Add examples for different iOS versions (flutte…
fb1bbd8 
…r#3757)

Improves the way we specify and test the `GoogleMaps` SDK dependency on iOS. Currently as discussed in flutter/flutter#86820, we are allowing any version of the SDK, and CocoaPods will choose the best version that's compatible with the minimum iOS deployment version of the plugin client's app. However, clients are also subject to future breakage, since any new major SDK version could have compile-time incompatibility with the plugin's wrapping code. We also have no ability to test anything in CI that's newer than the SDK that supports our oldest supported version (currently that's iOS 11, and thus 5.x).

This makes two changes:
- Replaces the single example with N examples, each with a different minimum iOS version, corresponding to each point where the Google Maps SDK has bumped its minimum iOS requirement. Currently (as documented in the new README file)
  - The oldest example has all of the current testing.
  - The newest example has just XCTests (not integration or XCUITests).
  - Intervening examples have no tests.
- Instead of the completely unpinned version, we allow anything up to the next major version (which should be the first one with breaking API changes).

This gives us build coverage of each of the (latest at CI runtime) resolve points that clients can have. We retain the flexibility of allowing clients to get the best version that meets their app's constraints, except that we will need to explicitly test and enable new major versions, so that they won't randomly break clients.

Fixes flutter/flutter#86820
@jmagman jmagman mentioned this pull request Nov 30, 2023
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyanglaz cyanglaz cyanglaz approved these changes

@jmagman jmagman jmagman approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: google_maps_flutter platform-ios
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pin iOS SDK GoogleMaps version to one supporting iOS 9
3 participants
@stuartmorgan @jmagman @cyanglaz