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

Separate Carthage optional dependencies #635

Conversation

manicmaniac
Copy link
Contributor

@manicmaniac manicmaniac commented Jul 15, 2019

Problem

When I install apollo-ios from Carthage, it always builds unnecessary optional dependencies like SQLite.swift and Starscream.
Building unused dependencies is not only a waste of time, but also it might lead a conflict with the version of user's dependency.
In fact, the current version designation is too strict for those who want to use SQLite.swift or Starscream itself.

https://github.com/apollographql/apollo-ios/blob/3a91bee6be5c4c42e35a8e372d9645f06b439b93/Cartfile

github "stephencelis/SQLite.swift" == 0.12.2
github "daltoniam/Starscream" == 3.1.0

I think the most of users of apollo-ios don't always use both of websocket and SQLite features.
Thus it shouldn't depend on them by default and only users have the right to decide whether to use the feature or not or which version to use.

Currently apollo-ios's Podfile resolves this problem by using subspec feature.

Cause

Since Carthage resolves dependency graph by parsing Cartfile, including optional dependencies into Cartfile causes this kind of problem.
Fortunately, Carthage has the neat solution: Cartfile.private.

Solution

Renamed Cartfile to Cartfile.private to make dependencies optional.

@designatednerd
Copy link
Contributor

@manicmaniac What happens when you try to build on an empty Carthage project here - does it only build the Apollo-iOS framework and not the other two? I think if it tries to build the other two, it'll bomb out.

@designatednerd designatednerd added the dependency-management Issues with CocoaPods, Carthage, or SPM integration label Jul 15, 2019
@manicmaniac
Copy link
Contributor Author

manicmaniac commented Jul 15, 2019

@designatednerd In fact, frameworks can be built individually even without building their dependencies (they are only needed at the time they are linked into an executable).

I have created the example repository including an empty iOS project and Cartfile.
In this commit I specified apollographql/apollo-ios on master branch in Cartfile.
Then in the next commit, I specified manicmaniac/apollo-ios, which indicates this PR.
Both of them are integrated to Travis CI so that we can see what happens.

And the result is that both of them succeeded in building dependencies.
You can see build log here for the first one, and here for the second one.

Unfortunately I wrote the project filename incorrectly so the main unit test was broken... but I'm pretty sure it doesn't affect the conclusion.

@designatednerd
Copy link
Contributor

Welp, thanks for helping me figure out I needed to do #639. 🙃

Once that gets merged I'll try installing via Carthage again with this to see what I can figure out.

@designatednerd
Copy link
Contributor

@manicmaniac Can you please pull in the changes from master so I can try to get this to build locally? Thanks!

@manicmaniac manicmaniac force-pushed the separate-carthage-optional-dependencies branch from 68a464d to 944bb2a Compare July 15, 2019 23:35
Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

This works, I will update the Carthage installation docs in a separate PR.

@designatednerd designatednerd merged commit 3317a66 into apollographql:master Jul 16, 2019
@designatednerd designatednerd added this to the 0.11.2 milestone Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency-management Issues with CocoaPods, Carthage, or SPM integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants