-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[google_maps_flutter_ios]: Add swift package manager compatibility #8288
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.
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
base: main
Are you sure you want to change the base?
[google_maps_flutter_ios]: Add swift package manager compatibility #8288
Conversation
@cbracken Seems like the CI runs tests with NOTE: There is two integration test sets, one for iOS14 and one for iOS15, I removed 'OCMock' pod only for iOS15 example (and replaced with swift package) as SPM cannot be used with iOS14 example. TestRunnerUI is not available for iOS15, so these tests are runned only for iOS14 at the moment (with cocoapods packages). Should iOS14 support be dropped for CocoaPods as well (even while it works?) |
objcHeaderOut: 'ios/Classes/messages.g.h', | ||
objcSourceOut: 'ios/Classes/messages.g.m', | ||
objcHeaderOut: | ||
'ios/google_maps_flutter_ios/Sources/google_maps_flutter_ios/messages.g.h', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
'ios/google_maps_flutter_ios/Sources/google_maps_flutter_ios/messages.g.h', | |
'ios/google_maps_flutter_ios/Sources/google_maps_flutter_ios/include/google_maps_flutter_ios/messages.g.h', |
(It looks like the header file is already in the right location. Just this config is stale)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
objcHeaderOut: | ||
'ios/google_maps_flutter_ios/Sources/google_maps_flutter_ios/messages.g.h', | ||
objcSourceOut: | ||
'ios/google_maps_flutter_ios/Sources/google_maps_flutter_ios/messages.g.m', | ||
objcOptions: ObjcOptions(prefix: 'FGM'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
objcOptions: ObjcOptions(prefix: 'FGM'), | |
objcOptions: ObjcOptions( | |
prefix: 'FGM', | |
headerIncludePath: './include/google_maps_flutter_ios/messages.g.h', | |
), |
Also, the messages.g.m
file should be updated to something like:
#import "./include/google_maps_flutter_ios/messages.g.h"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@jokerttu Apologies for the review delay! FYI, this PR by @vashworth is excellent prior art for migrating this plugin: https://github.com/flutter/packages/pull/6639/files The implementation files' - #import "GoogleMapMarkerController.h"
+ #import "./include/google_maps_flutter_ios/GoogleMapMarkerController.h" It looks like the explicit module Test {
- header "GoogleMapController_Test.h"
- header "GoogleMapMarkerController_Test.h"
+ header "./google_maps_flutter_ios/GoogleMapController_Test.h"
+ header "./google_maps_flutter_ios/GoogleMapMarkerController_Test.h"
} |
.package(url: "https://github.com/googlemaps/ios-maps-sdk", .upToNextMajor(from: "9.0.0")), | ||
.package(url: "https://github.com/googlemaps/google-maps-ios-utils", .upToNextMajor(from: "6.1.0")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like .upToNextMajor
is deprecated. Consider:
.package(url: "https://github.com/googlemaps/ios-maps-sdk", .upToNextMajor(from: "9.0.0")), | |
.package(url: "https://github.com/googlemaps/google-maps-ios-utils", .upToNextMajor(from: "6.1.0")), | |
.package(url: "https://github.com/googlemaps/ios-maps-sdk", "9.0.0"..<"10.0.0")), | |
.package(url: "https://github.com/googlemaps/google-maps-ios-utils", "6.1.0"..<"7.0.0"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Would you have suggestions how to solve the issue with tests and minimun supported GoogleMaps SDK versions between cocoapods and swift package manager discussed here: #8288 (comment) So should we keep support for |
Let's move this discussion to the issue, as there are important policy and technical questions to resolve that I want to have captured in a central place even if the PRs change. |
(Mechanically, once we resolve the policy questions about what we want to do with support in general, if we need to fix this it should be doable with some repo tooling adjustments.) |
@stuartmorgan pointed out here that iOS14 support should/might work after all, flutter/flutter#146920 (comment) I re-tested with package range supporting iOS 14 and it seems to use proper supported version of the package without build issues that I had earlier... I will need to update the package later, to match the current state of the repo, as this moves quite a lot files that have changed after initial commit. I will address rest of the @loic-sharma review comments later as well. |
Actually, maybe we won't need tooling changes. Can we just add this to the |
For this error: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8725011347110109681/+/u/Run_package_tests/build_examples/stdout
This happens because SPM strictly enforces that a package's platform requirements are equal to or higher than its dependencies. Since GoogleMaps 9.0.0+ requires iOS 15, google_maps_flutter_ios must also require iOS 15 or higher. |
Yes if min ios platform is set to ios14 at
But if min platform is set to I didn't find any way to have conditional setup, and for dependencies, conditions only support platform type (macOS or iOS), not platform version. I will address the existing issues, and remove support for iOS14 for now by reverting last changes (adding support for iOS14). I will also add disable-swift-package-manager to iOS14 example for now. As README lists iOS14 as supported platform, this change might need some note that SPM is supported only for iOS15 and above, and developers need to disable swift package manager if iOS14 is targeted? |
Right. I can do a follow-up pull request to update |
Are these updates really needed? - s.source_files = 'Classes/**/*.{h,m}'
- s.public_header_files = 'Classes/**/*.h'
- s.module_map = 'Classes/google_maps_flutter_ios.modulemap'
+ s.source_files = 'google_maps_flutter_ios/Sources/google_maps_flutter_ios/**/*.{h,m}'
+ s.public_header_files = 'google_maps_flutter_ios/Sources/google_maps_flutter_ios/include/**/*.h'
+ s.module_map = 'google_maps_flutter_ios/Sources/google_maps_flutter_ios/include/google_maps_flutter_ios.modulemap' And this for SPM: + cSettings: [
+ .headerSearchPath("include/google_maps_flutter_ios")
+ ] And on other packages, these are also without paths: |
9b6ef7e
to
09adf9b
Compare
@@ -36,6 +36,7 @@ Downloaded by pub (not CocoaPods). | |||
s.xcconfig = { | |||
'LIBRARY_SEARCH_PATHS' => '$(inherited) $(TOOLCHAIN_DIR)/usr/lib/swift/$(PLATFORM_NAME)/ $(SDKROOT)/usr/lib/swift', | |||
'LD_RUNPATH_SEARCH_PATHS' => '$(inherited) /usr/lib/swift', | |||
'GCC_PREPROCESSOR_DEFINITIONS' => '$(inherited) FGM_USING_COCOAPODS=1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this FGM_USING_COCOAPODS
preprocessor definition used to detect if cocoapods are used or not.
This is used for example at: https://github.com/flutter/packages/pull/8288/files#diff-dcc8100c7e606a3918ac1f3f2cd0aa16c2c6e551e09b9ff717f724a671ce5323R8
Mac_x64 ios_build_all_packages checks fail:
Not sure why, as ios14 project builds just fine locally with cocoapods and |
For this error:
All imports to the plugin's public headers need to be updated to - #import "GoogleMapController.h"
+ #import "./include/google_maps_flutter_ios/GoogleMapController.h" For this error:
You'll need to wrap packages/packages/camera/camera_avfoundation/example/ios/RunnerTests/StreamingTest.m Lines 6 to 8 in 8024c08
|
The reason why it's different is that |
a8f87db
to
45e9116
Compare
From triage: @jokerttu What's the status of this PR? It looks like there are still build failures in CI. |
|
], | ||
dependencies: [ | ||
.package(url: "https://github.com/googlemaps/ios-maps-sdk", "9.0.0"..<"10.0.0"), | ||
.package(url: "https://github.com/googlemaps/google-maps-ios-utils", "6.1.0"..<"7.0.0"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jokerttu Can you explain why the SwiftPM implementation can only be used in iOS 15+? Why can't it include google-maps-ios-utils
5.0 like the CocoaPods version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See discussion here; apparently the SDK's SPM configuration was broken in the version of the SDK that still supported iOS 14 :(
@jokerttu Since you're still working on this, I'm marking it as a draft. Please re-request when you're ready! |
I filed flutter/flutter#169376 with a design document to explore how we are going to handle dependency versioning in Package.swift going forward (e.g., to account for the recently-released 10.0). In theory we could release this without addressing that design doc, since currently this PR only allows one major version of the SDK anyway, but since several of the options there impact how the plugin will be structured we should probably make a decision before landing this. |
Makes
google_maps_flutter_ios
available as a Swift Package to Flutter for iOS 15+ while maintaining compatibility with CocoaPods for iOS 14+.It follows the same pattern used for other packages in the repository to ensure consistency.
The
GoogleMapsUtils
library introduces a separate target for Objective-C when using Swift Package Manager (SPM). As a result, package imports are handled differently depending on whether CocoaPods or SPM is used. This is controlled using the FGM_USING_COCOAPODS flag, which is defined in the package's Podfile.Closes #146920
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.