1223403 - Use Carthage correctly for importing dependencies

Status: RESOLVED FIXED

(Firefox for iOS :: Build & Test, defect)

Description

[Stephan Leroux [:sleroux]]

We're currently using Carthage as a git client to pull down the source of our dependencies and linking them to our project. Using Carthage correctly means being able to just link to the provided frameworks with ease and no strange work around like including headers from the Carthage import directory source to our bridging header.

Some of dependencies also don't use Carthage but support it so we should move towards using the Carthage version of those instead.

Other dependencies don't support Carthage. We should think about adding support for those and contributing the patches back. Win-win for everyone.

Comment 1

[Stephan Leroux [:sleroux]]

One thing I noticed is that all of the Carthage dependencies include a .xccodeproj file into our project. For the existing 'correct' Carthage dependencies we see these included at the top level. Now that we're changing some third party sources to be pulled in from Carthage I wonder if we should do the following:

1. Move any of the .xccodeproj references that are being pulled in from Carthage into a 'Carthage' or 'Dependencies' group.
2. As we transition Third Party code to rely on Carthage (such as the Deferred/SwiftKeychainWrapper PRs), remove their source file references from 'Third-Party Source' and include their .xccodeproj file under the 'Carthage' group.

The convention would then be:

1. Any Non-Carthage dependencies would have their .xccodeproj or raw source (what ever is available) live in `Third-Party Sources`.
2. Any Carthage dependency would be placed in the 'Carthage' or 'Dependencies' folder by adding a reference to its .xccodeproj file.

Would something like that work?

Comment 2

[Emily Toop (:fluffyemily)]

My plan for carthage is as follows:

1. For all third party code that can be carthagised, move to referencing carthage code, but otherwise leave as is.
2. For all non-carthage compliant carthage dependencies, remove from carthage into ThirdParty
3. Get Carthage building via `carthage update`
4. Remove all subprojects and third party carthage code from project and replace with linked frameworks
5. Look at which non-carthage third party code can be carthagised & migrate
6. Look at which non-carthage third party code can be replaced with carthage compliant code & migrate.

Currently I'm on step 1. As we're ultimately going to remove third party and subprojects, I don't want to start adding more sub projects at this time. If as we go along we want to change that, we can, but I want to get to step 4 as quickly as possible.

Comment 3

[Stephan Leroux [:sleroux]]

Ah okay that makes a lot more sense than the project file includes. I forgot that Carthage should be just linking frameworks and not have references like we do now. Really excited to see this!

Comment 4

[Emily Toop (:fluffyemily)]

Attachment: Pull request

All done (bar the merging)

Comment 5

[Emily Toop (:fluffyemily)]

(In reply to Emily Toop (:fluffyemily) from comment #4)
> Created attachment 8710550 [details] [review]
> Pull request
> 
> All done (bar the merging)

Instructions on how to test are in the PR description.

Patch for fixing SwiftKeychainWrapper to be compatible with extensions has been submitted back to jrendel. I'll update the Cartfile when this has been accepted.

Comment 6

[Stephan Leroux [:sleroux]]

Comment on attachment 8710550 [details] [review]
Pull request

Added a commit that resolves linking errors on device/simulator. Besides that, the changes look good +1

Comment 7

[James Hugman [:jhugman] [@jhugman]]

Comment on attachment 8710550 [details] [review]
Pull request

Tested ok.

Changes I needed to make on my machine:

 * upgrade carthage
 * upgrade git 

 % brew update
 % brew update carthage
 % brew update git
 % brew link --overwrite git

Thanks for this epic work. Would r+ again.

Comment 8

[Emily Toop (:fluffyemily)]

Attachment: Complete Pull request

Comment 9

[James Hugman [:jhugman] [@jhugman]]

LGTM, (the script changes).