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.

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

Compiler errors caused by map SDK v4.3.0 — NavigationMapView.preferredFramesPerSecond #1607

Closed
1ec5 opened this issue Aug 15, 2018 · 8 comments
Assignees
Labels
build Issues related to builds and dependency management. release blocker Needs to be resolved before the release.
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Aug 15, 2018

Mapbox Maps SDK for iOS v4.3.0 is scheduled for release today. If the developer has navigation SDK v0.17.0-beta.1–v0.19.0 installed, pod update and carthage update will automatically pull down this release, because the navigation SDK specifies only a minor version with the tadpole operator rather than a patch version.

s.dependency "Mapbox-iOS-SDK", "~> 4.2"

binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 4.2

v4.3.0 is slated to include mapbox/mapbox-gl-native#12501, which upstreams some of the frame rate overrides in #709. #935 named the property NavigationMapView.preferredFramesPerSecond, which conflicts with the MGLMapView.preferredFramesPerSecond property in mapbox/mapbox-gl-native#12501. So anyone who updates their dependencies after v4.3.0 is released will encounter these compiler errors:

/path/to/mapbox-navigation-ios/MapboxNavigation/NavigationMapView.swift:91:21: Overriding var must be as accessible as the declaration it overrides
/path/to/mapbox-navigation-ios/MapboxNavigation/NavigationMapView.swift:91:21: Overridden declaration is here (Mapbox.MGLMapView)
/path/to/mapbox-navigation-ios/MapboxNavigation/NavigationMapView.swift:91:21: Property 'preferredFramesPerSecond' with type 'Int' cannot override a property with type 'MGLMapViewPreferredFramesPerSecond'
/path/to/mapbox-navigation-ios/MapboxNavigation/NavigationMapView.swift:91:21: Attempt to override property here (Mapbox.MGLMapView)

We’ll need to remove the conflicting property, which is no longer necessary. To work around the incompatibility in v0.19.0, we’ll release a v0.19.1 based on the v0.19.0 tag that cherry-picks the change. Developers who are pinning to v0.19.0 specifically won’t automatically get v0.19.1; they’ll need to either update the dependency manually or explicitly constrain the map SDK dependency to v4.2.x.

To avoid issues like this in the future, we should coordinate upstreamed changes so that developers can more gracefully migrate.

/cc @mapbox/navigation-ios @mapbox/maps-ios

@1ec5 1ec5 added build Issues related to builds and dependency management. release blocker Needs to be resolved before the release. labels Aug 15, 2018
@1ec5 1ec5 self-assigned this Aug 15, 2018
@1ec5 1ec5 added this to the v0.19.1 milestone Aug 15, 2018
This was referenced Aug 15, 2018
@zugaldia
Copy link
Member

@1ec5 Could we have detected this issue with CI earlier? Basically, what if we had opened a PR on https://github.com/mapbox/mapbox-navigation-ios using espresso when the first pre-release was available (obviously we still wouldn't release the Nav SDK until the final Maps SDK landed). We would just let the CI tests run.

If CI wouldn't have caught it, can we make any changes to our CI build process to detect this kind of issue in the future?

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 16, 2018

Basically, what if we had opened a PR on https://github.com/mapbox/mapbox-navigation-ios using espresso when the first pre-release was available (obviously we still wouldn't release the Nav SDK until the final Maps SDK landed). We would just let the CI tests run.

Yes, fully agree. The map SDK release process includes upgrading the navigation SDK, but historically that step has been skipped for prereleases. Going forward, let’s make sure to at least push a branch that uses the prerelease so we can check CI status on it.

The changes required to upgrade to a prerelease look somewhat different than c46506c because Carthage doesn’t support prerelease versioning with the tadpole operator on binary releases: Carthage/Carthage#431 (comment). If I’m not mistaken, Cartfile and Cartfile.resolved would omit the tadpole operator in favor of something like:

binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" "4.3.0-beta.1"

binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 4.3

binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" "4.3.0"

@julianrex
Copy link
Contributor

Yes, fully agree. The map SDK release process includes upgrading the navigation SDK, but historically that step has been skipped for prereleases. Going forward, let’s make sure to at least push a branch that uses the prerelease so we can check CI status on it.

FYI @captainbarbosa

@chezzdev
Copy link
Contributor

The issue hasn't been resolved for installations via Carthage.
Cartfile with contents:

github "mapbox/mapbox-navigation-ios" ~> 0.19.1

is resolved to the following in Cartfile.resolved:

binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" "4.3.0"
github "ceeK/Solar" "2.1.0"
github "mapbox/MapboxDirections.swift" "v0.22.0"
github "mapbox/mapbox-events-ios" "v0.4.51"
github "mapbox/mapbox-navigation-ios" "v0.19.1"
github "mapbox/mapbox-voice-swift" "v0.0.1"
github "mapbox/turf-swift" "v0.2.0"
github "raphaelmor/Polyline" "v4.2.0"

when running carthage update.

Mapbox-iOS-SDK is still at 4.3.0 so building MapboxNavigation target produces the same errors as in original issue.

Looks like a carthage version resolving issue but I can't find anything similar in their issues at the moment.

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 23, 2018

Huh, #1609 set the Cartfile’s dependency to ~> 4.2.0, which should make v4.3.0 incompatible. Do you have any other dependencies that depend on the map SDK?

@chezzdev
Copy link
Contributor

chezzdev commented Aug 23, 2018

I created a new Cartfile in an empty folder for that experiment so navigation-ios is the only dependency.

@1ec5
Copy link
Contributor Author

1ec5 commented Sep 24, 2018

If you delete Cartfile.resolved and run carthage bootstrap, do you still get map SDK v4.3.0 or above?

@Prashanna7
Copy link

Prashanna7 commented Nov 25, 2018

Okey after some research I finally found the stable version of all the pod provided by Mapbox, and they are:

pod 'MapboxDirections.swift', '~> 0.20.0'
pod 'Mapbox-iOS-SDK' ,'~> 4.2.0'
pod 'MapboxGeocoder.swift','~> 0.10.0'
pod 'MapboxNavigation','~> 0.17.0

And this also solved my above mentioned problem. Cheers!! Happy coding !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to builds and dependency management. release blocker Needs to be resolved before the release.
Projects
None yet
Development

No branches or pull requests

5 participants