Swift Package Manager #24

MaartenZonneveld · 2024-02-05T08:29:38Z

Add support for Swift Package Manager.
Drop support for Carthage and Cocoapods.

boldtrn · 2024-02-05T12:50:56Z

Thanks for opening this PR @MaartenZonneveld. Any reason you closed this? Is this in favour of #23?

MaartenZonneveld · 2024-02-05T14:18:02Z

Any reason you closed this?

Reopened now 👌

Giordano-Menegazzi

CodeWise LGTM, alleen 2 stukjes code zijn commented... Mischien verwijderen of uncommenten? 🤔

XcodeProj/Examples/Swift/CustomViewController.swift

XcodeProj/MapboxNavigationTests/MapboxNavigationTests.swift

XcodeProj/RouteTest/Fixture.swift

Co-authored-by: Giordano Menegazzi <[email protected]>

boldtrn · 2024-02-06T17:47:05Z

MapboxCoreNavigation/NavigationSettings.swift

@@ -1,4 +1,5 @@
 import Foundation
+import MapboxCoreNavigationObjC


Could you explain why we sometimes use MapboxCoreNavigation and sometimes MapboxCoreNavigationObjC? Why the ObjC?

Objective-C & Swift can not co-exist in a single swift package target. Therefore a separate target for the objective-C components is created.

Ok got it. Would this mean, since we are in a swift file here, that we should rather import the swift dependency?

Sorry if this is a stupid question :)

It doesn't matter if you import it from Objective-C or Swift. If this file uses something that is defined in the Objc package, it should import the Objc package.

boldtrn · 2024-02-06T17:54:10Z

Package.swift

+    platforms: [.iOS(.v14)],
+    products: [
+        .library(
+            name: "MapboxNavigation",


Probably we should rename this to MapLibreNavigation or something like this?

I agree we should rename it at some point. However, a rename like that would involve renaming a lot more parts of the repository. I think the scope of this PR should be limited to swift package manager, as it is already a big change

Ok, if it's not just about the name here, but also refers to file names, then we can do this in a follow-up 👍

boldtrn · 2024-02-06T17:58:56Z

Package.swift

+let package = Package(
+    name: "maplibre-navigation-ios",
+    defaultLocalization: "en",
+    platforms: [.iOS(.v14)],


is iOS 14 required for maplibre-native v6?

Currently, MapboxDirections requires iOS 14 minimum

Is this a new requirement? I have seen that you added SPM support there as well, but I think the old Carthage build was compatible with iOS 8 or something like that.

Has something changed that much that requires iOS14?

The MapboxDirections fork supports back to iOS 12.0. So I now restored iOS 12 support for this PR as well.

boldtrn

Thanks a lot for working on this. I can't comment about the SPM changes itself.

If we start with a fresh SPM package I think this should be released as MapLibre and not Mapbox.

The changes in CustomViewController.swift seem to be unrelated to the SPM change. IMHO these should not be part of this PR to keep the commit history clean.

mindthefish · 2024-02-08T13:08:12Z

Is the use of Carthage and Cocoapods discouraged or why was it removed? Are the different types of integration incompatible? If not, I would be in favor of giving the user the choice.

MaartenZonneveld · 2024-02-12T08:33:57Z

Is the use of Carthage and Cocoapods discouraged or why was it removed? Are the different types of integration incompatible? If not, I would be in favor of giving the user the choice.

@mindthefish Swift Package Manager requires a certain structure, that is not compatible with how the project was structured before. With this PR it becomes structured for SPM, breaking the cocoapods and carthage integrations. It is of course possible to fix, and support all 3 integration solutions. However, that would encompass a lot more work, and tbh I don't believe cocoapods and carthage are worth the effort anymore. We could always bring back carthage and cocoapods support later if it is desired.

…ge-manager

boldtrn · 2024-03-01T07:57:38Z

@MaartenZonneveld thanks for working on this. What is the state of this PR? Do you think we can get this merged soon?

Currently CI is failing, that is something we should take care of before merging.

boldtrn · 2024-03-13T08:49:46Z

Any update on this @MaartenZonneveld? I think this would be a great addition and I'd love to merge this.

hactar · 2024-03-17T20:48:29Z

Currently using Maarten's fork in a new project we just started. It works. Would be important to merge it rather sooner than later, we'd like to do a few PRs against it to improve other things.

MaartenZonneveld · 2024-03-18T08:47:45Z

I've been very busy with other projects. I hope to be able to work on it this week.

If anyone has time to look into the failing CI, help is much appreciated 🙏

boldtrn · 2024-03-18T09:06:02Z

Thanks for the update @MaartenZonneveld.

@hactar would you be able to look into the failing CI? I think this could speed up getting this PR merged. I would love to merge this PR as well.

mindthefish · 2024-03-25T11:41:45Z

To review this PR, I took a closer look at the version of https://github.com/flitsmeister/mapbox-directions-swift referenced here. Attempting to check out, build and test it led to a bunch of problems that I cannot report in the repo because issues are disabled.
I may be wrong, but I do not feel comfortable using v0.23.3 of https://github.com/flitsmeister/mapbox-directions-swift.git in its current state.
@MaartenZonneveld , since you were involved here: There are a lot of deleted files that are still referenced in the project. I was not able to build any of the targets. Do you consider the current release of https://github.com/flitsmeister/mapbox-directions-swift.git to be mature?

Patrick-Kladek · 2024-04-02T11:37:15Z

@mindthefish you have opened MapboxDirections.xcodeproj however this part is not actively used in this PR. When you use Swift Package Manager all dependencies will use their Package.swift and not the legacy xcodeproj file.

So maplibre-navigation-ios Package.swift file references https://github.com/flitsmeister/mapbox-directions-swift which will then use https://github.com/flitsmeister/mapbox-directions-swift/Package.swift and this works perfectly. I've run the tests and they all pass *

I would say the failing test testRateLimitErrorParsing should have never been committed as its relying on an external service which will give unreliable results.

We could update to https://github.com/mapbox/mapbox-directions-swift. The License of this project allows it and the last release was on 1st March so it seems to be maintained.

Copyright © 2014–2023, Mapbox

Permission to use, copy, modify, and/or distribute this software for any purpose with or
without fee is hereby granted, provided that the above copyright notice and this permission
notice appear in all copies.

THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS
SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY
DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE
OR PERFORMANCE OF THIS SOFTWARE.

boldtrn · 2024-04-02T11:44:48Z

We could update to https://github.com/mapbox/mapbox-directions-swift. The License of this project allows it and the last release was on 1st March so it seems to be maintained.

Our goal is to be independent of Mapbox dependencies, so we should not rely on it IMHO. We could decide to fork the directions repo and start a fresh fork, ideally in the MapLibre org. I am not sure if this will introduce other issues / changes.

hactar · 2024-04-02T13:00:03Z

Yeah, MapLibre should have its own fork of Mapbox/MapLibre Directions I think too. The question is what should we fork from? mapbox's version, or flitsmeisters version? Anyone know what the differences are?

Patrick-Kladek · 2024-04-04T12:40:31Z

I've forked this branch and prepared a Pull Request to fix the CI. I've based my changes against @MaartenZonneveld fork and submitted a PR there, however no CI is running there.

My second attempt was to submit the PR against this Repo in #33 however now I need a workflow approval from someone with write access.

Now that there are 2 Pull Requests this might result in some confusion, so we should agree were we move forward (so I can close the redundant PR) and we get all feedback addressed in one place.

boldtrn · 2024-04-04T13:23:22Z

I approved the workflow, thanks for working on this 👍. @MaartenZonneveld how would you like to progress? Should we go ahead and review/merge the PR from @Patrick-Kladek? Do you want to pull the relevant changes from #33? That said CI is currently failing for #33

Patrick-Kladek · 2024-04-04T13:26:03Z

@boldtrn thanks for approving, will check check why it fails

Patrick-Kladek · 2024-04-04T13:36:46Z

So it turns out the build failed because the GitHub Actions macOS Image doesn't have the latest swift-tools version:

package 'maplibre-navigation-ios' is using Swift tools version 5.9.0 but the installed version is 5.7.1

I've pushed the changes to fix this, however I need another approval. @boldtrn can you approve all my (future) runs or only grant access for one build at a time?

boldtrn · 2024-04-04T13:56:24Z

I've pushed the changes to fix this, however I need another approval. @boldtrn can you approve all my (future) runs or only grant access for one build at a time?

I approved the CI again. Yeah, unfortunately an admin needs to approve the CI run until your first successful contribution to this repo. So for this PR I'll have to approve every CI run. If you want you can ping me in the OSMUS slack if you want to avoid writing here and if we need multiple runs.

Patrick-Kladek · 2024-04-04T20:52:54Z

Just found out you can actually create multiple Pull Requests in different Repos from the same branch. Therefore I've created a PR in my fork, enabled GitHub Actions there and made it run. This was way faster and probably less annoying than to ping you every time.

Here is the result: https://github.com/Patrick-Kladek/maplibre-navigation-ios/pull/1

My main finding for GitHub Actions are:

image: macos-latest resolves to macOS-12 for some reason, had to explicitly specify macos-14 for the image
macos-14 uses Xcode 15.0.1 as default, had to use xcode-select to configure Xcode 15.3
be patient, sometimes it seems that there is no progress during the build, after around 5 min the whole workflow completes.
from now on we will get nice build warnings & errors in the PR like this

@boldtrn please approve the workflow again so we get a green checkmark there. After that we should decide how to proceed with reviewing this PR @MaartenZonneveld.

boldtrn · 2024-04-05T08:50:00Z

Thanks a lot @Patrick-Kladek, the CI passed. @MaartenZonneveld please let us know how you would like to continue 👍

mindthefish · 2024-04-05T09:02:33Z

Great news, I can't wait to start using SPM for this repo. I'm still a little concerned that over 300 files have been changed to basically enable SPM, but I honestly don't have an overview of all those changes.

Patrick-Kladek · 2024-04-05T09:22:16Z

@mindthefish I can comment only on my part of the changes, most of which are from deleted files like the Examples (I don't think we can support that via SPM) followed by moving files to different folders.

I would guess the actual changes are 30-50 files and even there most were missing just some imports (seems like SPM is stricter here then Xcode, or Xcode adds some frameworks automatically)

I would prefer to merge this rather sooner than later as this is a huge PR already. Afterwards I would prepare more Pull Requests like:

if needed investigate how to bring the Examples back
rename everything from Mapbox to Maplibre, however for this we need to agree which external dependencies (like mapbox-directions) we fork into the maplibre organization.

Once we have that we can make a new major release from where we will support SPM and are truly Maplibre without any references to Mapbox anymore

mindthefish · 2024-04-05T10:01:56Z

Totally agree!

boldtrn · 2024-04-10T17:15:43Z

Thanks again for providing this PR @MaartenZonneveld. Given that this is a big PR and kind of blocking further changes. Would you be able to provide some feedback on how we should continue with the two PRs until tomorrow? Are you fine with the changes from @Patrick-Kladek? Do you want to pull / cherry pick them in your PR or should we close #24 and go ahead with #33, or do you have a different idea?

Given the importance, it would be great if we can go ahead with the SPM compatibility.

Patrick-Kladek · 2024-04-11T18:39:12Z

As @MaartenZonneveld is not responding I would propose to continue the official discussion in PR #33

boldtrn · 2024-04-12T08:11:22Z

Given that there hasn't been any update here, let's close this PR in favor for #33.

MaartenZonneveld added 4 commits January 26, 2024 14:55

SPM

7eff4aa

Remove carthage support

546ac8c

Delete MapboxCoreNavigation.podspec

01ba2a2

Fix ExampleSwift

622441e

MaartenZonneveld changed the title ~~Swift package manager~~ Swift Package Manager Feb 5, 2024

MaartenZonneveld requested a review from KasparElmans February 5, 2024 08:34

MaartenZonneveld marked this pull request as ready for review February 5, 2024 08:35

Add to changelog

6bf6a0d

MaartenZonneveld closed this Feb 5, 2024

MaartenZonneveld added 3 commits February 5, 2024 14:24

Examples & tests in xcodeproj

e858692

Clean up

50ffd3e

Append

08acf41

MaartenZonneveld reopened this Feb 5, 2024

Giordano-Menegazzi approved these changes Feb 5, 2024

View reviewed changes

XcodeProj/Examples/Swift/CustomViewController.swift Outdated Show resolved Hide resolved

XcodeProj/MapboxNavigationTests/MapboxNavigationTests.swift Outdated Show resolved Hide resolved

XcodeProj/RouteTest/Fixture.swift Outdated Show resolved Hide resolved

MaartenZonneveld and others added 2 commits February 6, 2024 09:13

Update XcodeProj/MapboxNavigationTests/MapboxNavigationTests.swift

b06b25d

Update XcodeProj/RouteTest/Fixture.swift

d49f418

Co-authored-by: Giordano Menegazzi <[email protected]>

iamalexantonov approved these changes Feb 6, 2024

View reviewed changes

Remove Carthage from workflow

5c7e396

boldtrn reviewed Feb 6, 2024

View reviewed changes

This was referenced Feb 7, 2024

chore: updated MapLibre Native dependency to use the ios-v6.0.0 versi… #23

Merged

Update @objc annotations to use ML or MLN instead of MB #26

Open

MaartenZonneveld force-pushed the feature/swift-package-manager branch from f70598f to 5c7e396 Compare February 12, 2024 08:36

Merge remote-tracking branch 'upstream/main' into feature/swift-packa…

eaa6797

…ge-manager

Restore iOS 12 support

929d70b

boldtrn mentioned this pull request Mar 2, 2024

dyld Library not loaded: @rpath/Mapbox.framework/Mapbox thrown by MapboxNavigation #28

Closed

MaartenZonneveld removed the request for review from KasparElmans March 18, 2024 08:52

Diego5529 approved these changes Mar 22, 2024

View reviewed changes

This was referenced Apr 4, 2024

Fix CI for Swift Package Manager MaartenZonneveld/maplibre-navigation-ios#1

Closed

Fix CI for Swift Package Manager #33

Merged

XanderD99 mentioned this pull request Apr 9, 2024

Cocoapods support #34

Open

boldtrn closed this Apr 12, 2024

		@@ -1,4 +1,5 @@
		import Foundation
		import MapboxCoreNavigationObjC

Swift Package Manager #24

Swift Package Manager #24

Conversation

MaartenZonneveld commented Feb 5, 2024 • edited Loading

boldtrn commented Feb 5, 2024

MaartenZonneveld commented Feb 5, 2024

Giordano-Menegazzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boldtrn left a comment • edited Loading

Choose a reason for hiding this comment

mindthefish commented Feb 8, 2024

MaartenZonneveld commented Feb 12, 2024 • edited Loading

boldtrn commented Mar 1, 2024

boldtrn commented Mar 13, 2024

hactar commented Mar 17, 2024

MaartenZonneveld commented Mar 18, 2024

boldtrn commented Mar 18, 2024

mindthefish commented Mar 25, 2024

Patrick-Kladek commented Apr 2, 2024

boldtrn commented Apr 2, 2024

hactar commented Apr 2, 2024

Patrick-Kladek commented Apr 4, 2024 • edited Loading

boldtrn commented Apr 4, 2024

Patrick-Kladek commented Apr 4, 2024

Patrick-Kladek commented Apr 4, 2024

boldtrn commented Apr 4, 2024

Patrick-Kladek commented Apr 4, 2024

boldtrn commented Apr 5, 2024

mindthefish commented Apr 5, 2024

Patrick-Kladek commented Apr 5, 2024 • edited Loading

mindthefish commented Apr 5, 2024

boldtrn commented Apr 10, 2024

Patrick-Kladek commented Apr 11, 2024

boldtrn commented Apr 12, 2024

MaartenZonneveld commented Feb 5, 2024 •

edited

Loading

boldtrn left a comment •

edited

Loading

MaartenZonneveld commented Feb 12, 2024 •

edited

Loading

Patrick-Kladek commented Apr 4, 2024 •

edited

Loading

Patrick-Kladek commented Apr 5, 2024 •

edited

Loading