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.

Sign up for GitHub

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

Jump to bottom

Add Carthage support #226

Merged
merged 2 commits into from
Jun 12, 2015
Merged

Add Carthage support #226

merged 2 commits into from
Jun 12, 2015

Conversation

jakecraige
Copy link
Contributor

Added a --carthage option that's disabled by default to provide backwards
compatibility.

What this option does:

  • Adds an empty Cartfile
  • Runs carthage update
  • Adds the copy-frameworks.sh build phase to the project
  • Adds carthage update to bin/setup script

@jakecraige
Copy link
Contributor Author

Right now the Cartfile is empty, I'm curious what y'alls thoughts are on providing defaults? If we want to, it's probably best in another PR since it increases the complexity a bit to do all the setup necessary.

@jakecraige
Copy link
Contributor Author

@thoughtbot/ios Any thoughts on this?

@gfontenot
Copy link
Member

Oh, yeah, sorry. I do have thoughts.

I really want Carthage support. It bothers me that we don't have it already. But I'm also thinking that the better long term solution would be to remove explicit CocoaPods support, and instead make dependency management completely generic. So for example, we could provide a dependency_installation_command or something to the .liftoffrc and then have it default to pod install. That could be overridden to be carthage bootstrap or bin/setup or git submodule update --init --recursive or whatever you want. Maybe we also include a default dependency_template or something that defaults to Podfile, then we can override that with Cartfile or false if there isn't one.

This might (will) cause issues with some current implementations that explicitly look for cocoapods support. For example, the Settings bundle stuff is fairly tightly coupled to the current implementation. But I do think this is a better long-term solution.

@jakecraige
Copy link
Contributor Author

@gfontenot I was thinking it would be nice to not duplicate all this logic as I was writing it but didn't want to change everything at once.

As a middle ground between the current state of this and what you're thinking, what do you think about for now extracting the dependency stuff into a single class so we remove this duplication, and come back later and couple it less?

@gfontenot
Copy link
Member

Sounds like a reasonable plan. I realize that what I'm asking for is a much larger undertaking.

jakecraige added a commit that referenced this pull request Jun 11, 2015
@jakecraige
Extract dependency managers into an object that can delegate to many
ca6582b 
This is the first step in removing specific dependency managers
from liftoff.

Based on [this conversation](#226 (comment))
@jakecraige jakecraige force-pushed the jc-carthage branch 2 times, most recently from f1242f5 to 0d0ac8c Compare June 11, 2015 16:20
jakecraige added a commit that referenced this pull request Jun 11, 2015
@jakecraige
Extract dependency managers into an object that can delegate to many
a48f886 
This is the first step in removing specific dependency managers
from liftoff.

Based on [this conversation](#226 (comment))
@jakecraige
Extract dependency managers into an object that can delegate to many
8625e6e 
This is the first step in removing specific dependency managers
from liftoff.

Based on [this conversation](#226 (comment))
jakecraige
jakecraige reviewed Jun 11, 2015
View reviewed changes
lib/liftoff/cli.rb
opts.on('--dependency-managers [NAME(s)]', 'Comma separated list of dependency managers to enable. Available options: cocoapods,carthage') do |list|
@options[:dependency_managers] = (list || "").split(",")
end

opts.on('--[no-]cocoapods', 'Enable/Disable Cocoapods') do |use_cocoapods|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if I should remove this option or not. It wouldn't break people's liftoffrc but if there were a script that passes in options this needs to be here. Thoughts?

@jakecraige
Copy link
Contributor Author

@gfontenot Did the extraction 8625e6e, let me know what you think. It should still maintain compatibility with anyone using the --use-cocoapods option or that has it in their liftoffrc

gfontenot
gfontenot reviewed Jun 11, 2015
View reviewed changes
lib/liftoff/launchpad.rb
generate_templates
generate_settings
install_cocoapods
install_dependency_managers
Copy link
Member

Choose a reason for hiding this comment

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

maybe install_dependencies?

@gfontenot
Copy link
Member

This is really really great. I think for the Cartfile, we can at least add Quick and Nimble as default dependencies. That's in line with what we're doing with CocoaPods, and we'll want them in all of our projects.

@jakecraige
Copy link
Contributor Author

@gfontenot Unfortunately that adds a lot of extra complexity to this PR. With that in mind I'd prefer to keep away from that for now.

@gfontenot
Copy link
Member

👯

@jakecraige
Add Carthage support
aca9304 
Added carthage as a dependency manager.

What it does:

- Adds an empty `Cartfile`
- Runs `carthage update`
- Adds the `copy-frameworks.sh` build phase to the project
- Adds `carthage update` to `bin/setup` script
@jakecraige jakecraige merged commit aca9304 into master Jun 12, 2015
@jakecraige jakecraige deleted the jc-carthage branch June 12, 2015 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jakecraige @gfontenot