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

Carthage support #36

Merged
merged 4 commits into from Nov 9, 2018
Merged

Conversation

gobetti
Copy link
Contributor

@gobetti gobetti commented Nov 3, 2018

I'm particularly unhappy with the targets names (RxNimbleRxBlocking and RxNimbleRxTest) so please feel free to suggest any other names.

I've tried to add dashes to those names (e.g. RxNimble-RxBlocking) but then I had trouble importing those targets, even though I did replace the dash by an underscore and I did verify that the auto-generated product module name was RxNimble_RxBlocking, so I ran out of ideas on why this was failing.

Closes #9

- pod repo update
script: cd Demo ; set -o pipefail && xcodebuild -workspace 'Demo.xcworkspace' -scheme 'Demo' -configuration 'Debug' -sdk iphonesimulator -destination platform='iOS Simulator',OS='12.0',name='iPhone 8' build test | xcpretty -c --test
- carthageversion=$(carthage version)
- if [ $carthageversion != "0.31.2" ]; then brew uninstall carthage; HOMEBREW_NO_AUTO_UPDATE=1 brew install https://raw.githubusercontent.com/Homebrew/homebrew-core/a85feeb75bc9e9beb7f2e9dc6e2ccc996a6aeaf5/Formula/carthage.rb; fi
Copy link
Contributor Author

@gobetti gobetti Nov 3, 2018

Choose a reason for hiding this comment

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

to the best of my knowledge, a carthage version can't be specified in a Gemfile, so this is currently the fastest way I know to install a specific version. And actually I didn't mean to use 0.31.2 specifically, I'd like to always use the most up-to-date one but brew update is expensive

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is a bit backwards but I don't have a nicer idea to go around this for now :)

Copy link
Member

@ashfurrow ashfurrow left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! Let's give folks another day to give feedback then merge 👍 Thanks again!

Carthage requires a shared scheme, so now we have an actual project
replacing the old Demo one.

Initially this is going to be a single RxNimble target which will
compile all dependencies. Let's improve this later by having specific
RxBlocking vs. RxTest targets.

Had to disable bitcode in order to successfully import XCTest (which is
done inside Nimble).
Thanks so much Moya for this! A lot of manual changes were necessary:
- the SUPPORTED_PLATFORMS must include all of iOS, macOS and tvOS;
- the FRAMEWORK_SEARCH_PATHS need to be individually defined for each
SDK;
- in the tests target, the LD_RUNPATH_SEARCH_PATHS are different for
macOS;
- the TARGETED_DEVICE_FAMILY must include all of iOS, macOS and tvOS aka
1,2,3 (it can't be left out of the config because the default would also
include watchOS aka 4)
@gobetti
Copy link
Contributor Author

gobetti commented Nov 8, 2018

Rebased on top of master!

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

I noticed you entirely removed the Example project here. Could you explain the motivation?

Aside from that, this looks good to me IRT to the Carthage-related things.

@gobetti
Copy link
Contributor Author

gobetti commented Nov 9, 2018

Hi @freak4pc! That's because the example project didn't contain any example. All it had was unit tests that were testing RxNimble itself, so these tests are now part of an RxNimble project.

Please take a closer look at the removed files (like ViewController.swift) and notice that the tests files have been kept nearly untouched :)

@freak4pc
Copy link
Member

freak4pc commented Nov 9, 2018

OK, that makes sense. The only thing that bothers me here is that, the only way to develop this project now is using Carthage. the previous tests and sources were previously tied to CocoaPods which made it very easy to develop on a "Development Pod" directly inside the project.

Now, not only this project supports Carthage, but it became the only way to open RxNimble.xcodeproj (meaning having to do carthage bootstrap). I am personally not a fan of Carthage (like many others), so even though I support supporting it, I'm not a fan of it being a barrier to entry to people who want to participate with contributions.

I'm not sure I 100% love this but if @ashfurrow is OK with this, I am as well.

I have no issues with the rest of the code / migration. Great work!

@ashfurrow
Copy link
Member

This is a really common pattern I've seen on projects I have built with CocoaPods; people will add Carthage support, but their PR restructures the project around using Carthage to develop with. I don't like it, but I've given up trying to fight it. Carthage just wants to be in charge.

Thanks for the work @gobetti – going to merge!

@ashfurrow ashfurrow merged commit 3dc5048 into RxSwiftCommunity:master Nov 9, 2018
@ashfurrow
Copy link
Member

I've released this as 4.4.1 for Carthage, though I'm not going to bother publishing 4.4.1 for CocoaPods 🙃

@gobetti
Copy link
Contributor Author

gobetti commented Nov 9, 2018

I totally agree with both of you @freak4pc and @ashfurrow , and as pointed out, this is a common pattern I've been seeing in other projects as well, including some of the most up-to-date ones like SwiftLint and Moya. Just let me know whatever other pattern you guys have in mind where Carthage does not become a dependency for contributors, and I'll be more than happy to make the changes. Thanks again!

@ashfurrow
Copy link
Member

I wish I had an answer, but I haven't seen a great way for Carthage and CocoaPods to coexist in a project without Carthage taking over. It keeps happening to my projects, but like I said I've kind of given up looking for an answer. (This is just what happens when you try to combine a very flexible and a very inflexible dependency manager – the inflexible one wins. I think there's a lesson in there somewhere but I'm too tired to find it 😅)

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