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

Support Carthage #79

Merged
merged 3 commits into from Aug 16, 2015
Merged

Support Carthage #79

merged 3 commits into from Aug 16, 2015

Conversation

cybertk
Copy link
Contributor

@cybertk cybertk commented May 30, 2015

Before this PR is merged, add following line into your Cartfile to use with Carthage,

github "cybertk/FLAnimatedImage" "carthage"

@dcaunt
Copy link

dcaunt commented Aug 5, 2015

Can this be merged? I've checked out @cybertk's branch and it builds a framework successfully.

@raphaelschaad
Copy link
Collaborator

Yes, this looks good, thanks @cybertk. @dcaunt: Did you build & run Pinterest.app with PINRemoteImage including FLAnimatedImage built from this branch?

Sorry for the delayed response, I had accidentally routed my GH notifications to something like /dev/null.

@cybertk
Copy link
Contributor Author

cybertk commented Aug 12, 2015

Where can I find Pinterest.app? I will give it a shot.

@dcaunt
Copy link

dcaunt commented Aug 12, 2015

@raphaelschaad I'm not sure what you mean by Pinterest.app – I don't work at Pinterest – just trying to help out with Carthage support on PINRemoteImage.

@garrettmoon
Copy link

Whoops, I think I caused some confusion. I'm trying to help @dcaunt add Carthage support to https://github.com/pinterest/PINRemoteImage. But that depends on FLAnimatedImage getting Carthage support currently.

I'm happy to run any tests. Just to be sure, you'd like me to make sure FLAnimatedImage still builds and runs with this branch in the Pinterest app? I can definitely test that. We don't actually use Carthage (yet?), so I won't easily be able to test that this actually enables Carthage support :)

@dcaunt
Copy link

dcaunt commented Aug 12, 2015

So I tested with carthage build --no-skip-current and took a look at the framework headers which look correct.

You could additionally verify that the framework is correct by modifying the demo project and dragging in the resulting FLAnimatedImage.framework from Carthage/Build.

@raphaelschaad
Copy link
Collaborator

@dcaunt I was able to run the demo project successfully powered by the .framework. However, the .framework was built with DEBUG 0 and hence doesn't respond to calls the demo project makes use of. Is it possible to build a debug .framework with Carthage?

As a side note for myself, this helped resolve an initial runtime error I got: Alamofire/Alamofire#101

@cybertk
Copy link
Contributor Author

cybertk commented Aug 16, 2015

@raphaelschaad You mean that we need set DEBUG=1 for the distributed framework? Or just set it for demo project only?

@raphaelschaad
Copy link
Collaborator

Just for testing the demo project.

I was curious whether there is a way to build a DEBUG=1 framework through Carthage, without modifying the code/project itself.

PS: It would be great if you get the chance to quickly look at #79 (comment) – thanks!

@cybertk
Copy link
Contributor Author

cybertk commented Aug 16, 2015

Current FLAnimatedImage.xcodeproj already defined DEBUG=1 for Debug configuration, the debug framework can also built from carthage build --no-skip-current --configuration Debug

@cybertk
Copy link
Contributor Author

cybertk commented Aug 16, 2015

I see there is no testing target in FLAnimatedImageDemo.xcodeproj, so what do you mean testing the demo project?

@raphaelschaad
Copy link
Collaborator

Great, --configuration Debug was what I was looking for, thanks. By testing I mean verifying that the built framework works correctly by dragging it from Carthage/Build into the demo project and run it. It worked well.

Let's 🚢 this. Thanks!

One remaining post-merge thing I'd like to discuss is the issue with the framework version number: #79 (comment)

raphaelschaad added a commit that referenced this pull request Aug 16, 2015
@raphaelschaad raphaelschaad merged commit e5e940f into Flipboard:master Aug 16, 2015
@raphaelschaad raphaelschaad self-assigned this Aug 16, 2015
@raphaelschaad raphaelschaad added this to the 1.0.9 milestone Aug 16, 2015
@dcaunt
Copy link

dcaunt commented Aug 16, 2015

@raphaelschaad Great, thank you for taking the time to run through this and merge the PR.
I'm glad that you figured out the Debug issue – I think most libraries/frameworks don't have a conditional interface in the way that FLAnimatedImage does, so this isn't a common issue.

My understanding with the version number is that it is fine to leave the version as 1.0. While you can keep track of the version in the framework itself, not all frameworks actually do so. As tools like Carthage and CocoaPods can handle versioning and dependency resolution at a higher level, I think it is only useful in environments where shared frameworks are used rather than embedded frameworks (as is the case in iOS and Mac OS applications.)

@raphaelschaad
Copy link
Collaborator

@dcaunt Sure! Thank you. I read up on how Carthage versioning works and it seems tagging master as I use to will work well.

@cybertk I'll include your change in the change log for the next version.

  * Support Carthage (@cybertk)

Please let me know your full name if you'd like that to show up in the change log instead (so far it's all full names but I don't mind either way).

Could you also fill out Flipboard’s Apache-style Individual Contributor License Agreement (CLA)? It's standard and quick to fill out. Thanks!

https://docs.google.com/forms/d/1gh9y6_i8xFn6pA15PqFeye19VqasuI9-bGp_e0owy74/viewform

@cybertk
Copy link
Contributor Author

cybertk commented Aug 18, 2015

CLA is signed and my full name is Quanlong He

@raphaelschaad
Copy link
Collaborator

Awesome, thanks @cybertk, updated in bab99f6.

@dcaunt
Copy link

dcaunt commented Aug 18, 2015

@raphaelschaad When 1.0.9 hits it would be nice to provide a prebuilt framework for Carthage. Carthage can download prebuilt frameworks rather than checking out source and building, if an archive is attached to the release (example from Mantle). You can build the archive for attaching to a release like this:

carthage build --no-skip-current
carthage archive FLAnimatedImage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants