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

Replace CocoaPods with Carthage for demo project #100

Closed
wants to merge 12 commits into from
Closed

Replace CocoaPods with Carthage for demo project #100

wants to merge 12 commits into from

Conversation

simonseyer
Copy link
Contributor

@simonseyer simonseyer commented Nov 16, 2016

This PR

Please note that it is based on the swift3 branch.

Open ToDos:

  • Test integration with Carthage
  • Fix tests
  • Test integration with CocoaPods
  • Add missing Mapbox iOS SDK dependency

I'm on a bad network (for a longer time). Maybe someone else can support me with the last two ToDos?

* Adds Carthage compatibility
* Breaks demo-build because of missing Mapbox SDK (todo)
@simonseyer simonseyer changed the title WIP: Replace CocoaPods with Carthage [WIP] Replace CocoaPods with Carthage Nov 16, 2016
@simonseyer
Copy link
Contributor Author

Get the Bitrise tests not executed for PRs?

@1ec5
Copy link
Contributor

1ec5 commented Nov 16, 2016

Add missing Mapbox iOS SDK dependency

This will be difficult due to mapbox/mapbox-gl-native#1623. Instead, there would have to be a shell script to download the SDK manually.

Get the Bitrise tests not executed for PRs?

I’ve reenabled PR builds for this repository. (We usually have them off because they make it more likely for this repo’s builds to hold up builds in the more fast-paced mapbox-gl-native repository.)

@simonseyer
Copy link
Contributor Author

This will be difficult due to mapbox/mapbox-gl-native#1623. Instead, there would have to be a shell script to download the SDK manually.

Yeah, thought about the same. Can add a script tomorrow. However, I will be unable to test it ;-)

I’ve reenabled PR builds for this repository.

👍

@1ec5
Copy link
Contributor

1ec5 commented Nov 16, 2016

I like Carthage as much as anyone, but the situation around mapbox/mapbox-gl-native#1623 unfortunately makes it harder to justify moving this project to Carthage.

Is #88 the motivation for this PR? This framework, MapboxGeocoder.swift, and MapboxStatic.swift are supposed to work in both CocoaPods and Carthage already. Perhaps the fix for #88 is far simpler.

@simonseyer
Copy link
Contributor Author

CocoaPods has a build step which makes sure that the dependencies are satisfied. This can only be done (as far as I know) by running pod install. Carthage, without having knowledge about this requirement, has no chance of satisfying it before building. That's the reason for the error.

Another solution would be to have separate projects for the framework and the example App. The main project would have no dependency to mapbox-gl-native/CocoaPods and the example project could still use CocoaPods without interfering with the Carthage build process. This would save us from writing the script but would lead to two projects.

What do you think?

@1ec5
Copy link
Contributor

1ec5 commented Nov 16, 2016

Yes, I think putting the example applications and frameworks in separate projects could work. We could still use CocoaPods to tie them together by putting a relative path in the Podfile. (That makes them “development pods”, which seems correct to me.)

By the way, we plan on publishing one final Swift 2.3 release before merging #64 and making Swift 3 the main development branch. If you need Carthage compatibility soon, would you mind retargeting this PR at master? Then I can take care of merging to the swift3 branch. (Fun times!) Otherwise, targeting swift3 is fine, but it can’t land in master until we cut over to Swift 3 officially.

What do you think, @incanus @friedbunny? We’d want to do exactly the same thing in MapboxGeocoder.swift and MapboxStatic.swift.

@incanus
Copy link
Contributor

incanus commented Nov 16, 2016

I like the idea of splitting. In my mind, complicating the broad use case (the library) for the narrow one (maybe I need to see an example project) is not great—cleaning that up would be 👌.

@simonseyer
Copy link
Contributor Author

Sounds good. Will update my pull request.

The reason I branched off swift3 is that we at COBI use Carthage as well as requiring Swift 3. When this pull request could be merged into the swift3 branch I could reference the main repository with the swift3 branch instead of our fork.

@simonseyer
Copy link
Contributor Author

simonseyer commented Nov 17, 2016

Would it be a lot of work for you to implement #12?

For running the tests carthage update now has to be executed beforehand. By having the configuration in the repository it would be easy to just configure it for this PR.

@simonseyer simonseyer changed the title [WIP] Replace CocoaPods with Carthage Replace CocoaPods with Carthage Nov 17, 2016
@simonseyer
Copy link
Contributor Author

Made the changes. Please have a look.

@simonseyer
Copy link
Contributor Author

Do you have any feedback for this PR?

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay – I’m a bit tied up with getting the iOS SDK out the door right now. This branch looks good, other than the one change that should be reverted. In the meantime, I’m working on reconfiguring the Bitrise bots to build the frameworks with Carthage and also look for the example projects in the right place.

@@ -5,7 +5,7 @@ import Mapbox

// A Mapbox access token is required to use the Directions API.
// https://www.mapbox.com/help/create-api-access-token/
let MapboxAccessToken = "<# your Mapbox access token #>"
let MapboxAccessToken = "pk.eyJ1IjoiY29iaS1iaWtlIiwiYSI6ImNpdmwwMnZ3ejAwODQyenA0cTMzanFiZjYifQ.Sbd_5nOiDjCKtr2ZYuKIFw"
Copy link
Contributor

Choose a reason for hiding this comment

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

Before we can merge this PR, this change needs to be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Removed the access token.

@simonseyer
Copy link
Contributor Author

No worries. Sounds good! Thanks for your feedback

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

The iOS builds are failing because the Example (Swift) and Example (Objective-C) schemes went missing when you split out DirectionsExample.xcworkspace. You probably still have them in your workspace, but you need to mark them as shared so the Bitrise bots can find them.

@simonseyer
Copy link
Contributor Author

Mhm, still one test failing. Can you forward the error message to me?

@1ec5
Copy link
Contributor

1ec5 commented Dec 6, 2016

Sure. The watchOS failure is a known issue that affects the master and swift3 branches, too, so I wouldn’t worry about it for now:

▸ Processing Info.plist

❌  /Users/vagrant/git/MapboxDirections/MBRouteLeg.swift:1:8: no such module 'Polyline'

import Polyline
       ^

The iOS failure is strange – Bitrise is missing the details needed to clone your repository:

Git Pull Request Configs:
- PullRequestURI: 
- PullRequestID: 100
- BranchDest: 
- PullRequestMergeBranch: 
- ResetRepository: No
Bitrise Build Configs:
- BuildURL: https://www.bitrise.io/build/cc3609f722d56e3b
- BuildAPIToken: ****

Git clone repository
=> git "init"
=> git "remote" "add" "origin" "git@github.com:mapbox/MapboxDirections.swift.git"
=> git "fetch"
=> git "checkout" "0c20ad07f0d45b704dfeba7e1c28ebb07a0194a0"
Failed, error: fatal: reference is not a tree: 0c20ad07f0d45b704dfeba7e1c28ebb07a0194a0

but the same step works fine in the other three Bitrise apps:

Git Pull Request Configs:
- PullRequestURI: https://github.com/cobi-bike/MapboxDirections.swift.git
- PullRequestID: 100
- BranchDest: swift3
- PullRequestMergeBranch: pull/100/merge
- ResetRepository: No
Bitrise Build Configs:
- BuildURL: https://www.bitrise.io/build/2420e0d3901760fd
- BuildAPIToken: ****

Git clone repository
=> git "init"
=> git "remote" "add" "origin" "git@github.com:mapbox/MapboxDirections.swift.git"
=> git "fetch" "origin" "pull/100/merge:pull/100"
=> git "checkout" "pull/100"
=> git "submodule" "update" "--init" "--recursive"

@incanus incanus changed the title Replace CocoaPods with Carthage Replace CocoaPods with Carthage for demo project Dec 6, 2016
@simonseyer
Copy link
Contributor Author

0c20ad07f0d45b704dfeba7e1c28ebb07a0194a0 is definitely a valid sha in my fork. But it seems the bot tries to checkout this commit on the original mapbox/MapboxDirections.swift repository. That can't work for obvious reasons.

Checking out pull/100 (like the other bot does) seems to do the trick (it seems GitHub is doing some magic there, good to know).

But it definitely has to do something with the configuration of the build server. So I don't think I can do something about it.

@1ec5
Copy link
Contributor

1ec5 commented Dec 6, 2016

Agreed, that was out of your control.

I went ahead and pushed an identical carthage-integration branch to this repository as a dry run, and the iOS push build it triggered failed for a different reason:

2016/12/06 10:58:19 Failed to send command to cmd-bridge server: Post http://localhost:…/cmd: dial tcp [::1]:…: getsockopt: connection refused

This is not my day. 😆

@1ec5
Copy link
Contributor

1ec5 commented Dec 6, 2016

The iOS build is failing at the “Analyze Swift Example” step:

▸ Analyzing DirectionsExample/Example (Swift) [Debug]
▸ Check Dependencies

❌  Code signing is required for product type 'Application' in SDK 'iOS 10.1'

If we add a test bundle for the example applications and add it to the two schemes, we can replace that analyze step with a test step that’ll pass.

A reminder to myself: before merging this PR, I need to map the swift3 branch to the carthage-integration workflow.

@simonseyer
Copy link
Contributor Author

simonseyer commented Dec 6, 2016

If we add a test bundle for the example applications and add it to the two schemes

done

@1ec5
Copy link
Contributor

1ec5 commented Dec 7, 2016

2016-12-06 14:40:30.457 Directions (Swift)[2993:17801] Failed to load test bundle from file:///Users/vagrant/Library/Developer/Xcode/DerivedData/DirectionsExample-haqgdtwaqzzzzockkvrktekipgrg/Build/Products/Debug-iphonesimulator/Directions%20(Swift).app/PlugIns/Example%20Tests%20(Swift).xctest/: Error Domain=NSCocoaErrorDomain Code=4 "The bundle “Example Tests (Swift)” couldn’t be loaded because its executable couldn’t be located." UserInfo={NSLocalizedFailureReason=The bundle’s executable couldn’t be located., NSLocalizedRecoverySuggestion=Try reinstalling the bundle., NSBundlePath=/Users/vagrant/Library/Developer/Xcode/DerivedData/DirectionsExample-haqgdtwaqzzzzockkvrktekipgrg/Build/Products/Debug-iphonesimulator/Directions (Swift).app/PlugIns/Example Tests (Swift).xctest, NSLocalizedDescription=The bundle “Example Tests (Swift)” couldn’t be loaded because its executable couldn’t be located.}

I can’t reproduce this error locally. If you set up a Bitrise account, I can give you access to the builds, although for some reason the iOS build only gets this far with push builds. I’ll keep at it.

@simonseyer
Copy link
Contributor Author

My Bitrise username is SimonSeyer.

@simonseyer
Copy link
Contributor Author

@1ec5 I'm not experienced with Bitrise. How does it come that the second build works (fails because of real test issues) and the first one cannot even be checked out (git)? When/How does the second build-type get triggered?

screen shot 2016-12-07 at 14 38 16

@1ec5
Copy link
Contributor

1ec5 commented Dec 7, 2016

I honestly don’t know what’s going on. It could be that I was fiddling with the iOS bot’s settings while you were pushing to the PR and that confused Bitrise. The GitHub webhooks are sending the same data to each bot.

I’ve been tag-teaming with you, pushing each of your commits to this repository when I notice them. That’s why you sometimes see the /push builds that get a little further along. To save both of us some hassle, I’ve added you as a contributor to this repository as well. Thanks for your persistence!

@simonseyer
Copy link
Contributor Author

Ok, thank you. You are welcome.

Is there something in the Bitrise workflow that satisfies the following assertion before running the tests?
DirectionsObjc[10097:330926] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'You must set MapboxAccessToken to your Mapbox access token.'

@simonseyer simonseyer mentioned this pull request Dec 7, 2016
@simonseyer simonseyer closed this Dec 7, 2016
@1ec5
Copy link
Contributor

1ec5 commented Dec 7, 2016

Is there something in the Bitrise workflow that satisfies the following assertion before running the tests?

It doesn't make sense to use a real access token in unit tests, since we're using test fixtures and avoiding network requests anyways. So in the setUp method, we can set an environment variable to disable the access token check and modify the assertions to check for that environment variable.

@simonseyer
Copy link
Contributor Author

I solved it by running the tests in release mode. Since there no real test, it shouldn't make a difference with which configuration they are executed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants