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

Add Carthage Support #21

Merged
merged 1 commit into from
Feb 16, 2016
Merged

Add Carthage Support #21

merged 1 commit into from
Feb 16, 2016

Conversation

tternes
Copy link
Contributor

@tternes tternes commented Jan 27, 2016

This change set provides Carthage support for the library, which would resolve #17.

I've attempted to follow the example of AlamofireImage for how to best satisfy dependencies while not requiring integrators to use a particular dependency management system. To accomplish this goal, I've removed internal use of Pods within JSONWebToken, and instead satisfy the CryptoSwift dependency with a submodule. I've added a Cartfile that handles the dependency chain for Carthage users, and attempted to verify CocoaPods support with a local spec. I've also updated the Travis tests and run them against my fork to make sure those continue working.

As a Carthage user, I'm most concerned about having possibly broken CocoaPods, so feedback in that area would be greatly appreciated.

As a look ahead, I'm also planning to submit changes to add support for an iOS framework target, as well as code changes to support custom header values and signing key formats. I'll also submit a follow-up PR with changes for the README.

@tternes
Copy link
Contributor Author

tternes commented Feb 1, 2016

Any chance you'll be able to review this in the next few days, @kylef? I have a couple other changes queued up that I've held until this merges.

Thanks!

- xcodebuild -workspace JWT.xcworkspace -scheme JWT test -sdk macosx | xcpretty -c
- pod lib lint
Copy link
Owner

Choose a reason for hiding this comment

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

Can you restore this line, it's verifying if the pod spec is valid for CocoaPods.

@tternes
Copy link
Contributor Author

tternes commented Feb 7, 2016

@kylef I get the following error when I run pod lib lint --no-clean --verbose locally:

** BUILD SUCCEEDED **

 -> JSONWebToken (1.4.2)
    - WARN  | url: The URL (http://twitter.com/kylefuller) is not reachable.

I've taken care of the other comments.

# https://github.com/fastlane/fastlane/blob/master/docs/Gitignore.md

fastlane/report.xml
fastlane/screenshots
Copy link
Owner

Choose a reason for hiding this comment

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

We don't use fast lane and a bunch of things in this gitignore, can you strip them out of this gitignore please?

@kylef
Copy link
Owner

kylef commented Feb 8, 2016

Okay, just two remaining comments. I've fixed the issue with the podspec in 6df50e2, so rebasing on-top of master should solve this. Would you be able to address them and then squash these commits?

I have one concern, that the Xcode project only has an OS X Target. From what I understand, that means Carthage will only be able to integrate this library into OS X and it won't work for iOS, tvOS and watchOS users. Is this a problem for you?

@tternes
Copy link
Contributor Author

tternes commented Feb 8, 2016

I have one concern, that the Xcode project only has an OS X Target.

I have another PR ready for iOS / tvOS targets that should take care of that.

@tternes
Copy link
Contributor Author

tternes commented Feb 9, 2016

@kylef The pod lint step passes for me locally, but it looks like Travis has an issue with it. Any chance you can give me some guidance on that, since I'm not as familiar with what pods might be upset about?

Thanks so much for your help with this.

kylef added a commit that referenced this pull request Feb 16, 2016
@kylef kylef merged commit 9564b83 into kylef:master Feb 16, 2016
@ikesyo ikesyo mentioned this pull request Feb 18, 2016
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.

Carthage support?
2 participants