Carthage support

[MarvinNazari]

Would be awesome to have Carthage support.

[soffes]

+1

[bcapps]

+1

[ciso]

+1

[cdzombak]

Please do not +1 issues; we generally do not use voting to determine this project’s roadmap. If you would like to see something happen sooner, a pull request is the best argument.

[livings124]

There was a pull request for this issue sitting around for months. What could be done to get it accepted?

[cdzombak]

I should expand on my stock (copy & pasted) reply; sorry about that.

There were a number of issues remaining with that initial, experimental PR, including how far out of date from develop it had fallen.

While we will eventually need Carthage support ourselves, it’s not currently the top priority for our team. But I can outline our current thinking on what we need to see in an acceptable Carthage-support PR:

• Restructure the Xcode workspace/project and directory structure, and share schemes as needed for Carthage, roughly following this excellent guide.

  We’re aiming for a directory structure like:

  .
  ├── Carthage
  │   ├── Build
  │   └── Checkouts
  ├── Documentation
  ├── Example
  │   ├── Example
  │   │   ├── *.h, *.m, *.swift
  │   │   ├── Assets.xcassets
  │   │   ├── Base.lproj
  │   │   └── Info.plist
  │   └── Example.xcodeproj
  ├── NYTPhotoViewer
  │   ├── *.h, *.m
  │   └── Info.plist
  ├── NYTPhotoViewer.podspec
  ├── NYTPhotoViewer.xcodeproj   # PhotoViewer library + shared schemes
  ├── NYTPhotoViewer.xcworkspace # Workspace with Example + PhotoViewer projects
  ├── NYTPhotoViewerTests
  │   ├── *.h, *.m, *.swift
  │   └── Info.plist
  └── scripts
  

• Add Cartfile including the optional FLAnimatedImage dependency.

• As a small detail that deviates from the linked guide, our setup script is named scripts/bootstrap for consistency with our internal libraries’ structures. We’ve split this responsibility into two scripts: bootstrap which runs carthage bootstrap --platform iOS --no-use-binaries --use-submodules, and carthage-update which runs carthage update --platform iOS --no-use-binaries --use-submodules.

• Verify building with Carthage works as expected.

• Figure out how to manage conditionally-compiled animated GIF support. Currently GIF support is compiled out if you don’t include the AnimatedGifSupport subspec, and ideally we wouldn't force clients to include this support when it’s not desired.

• Figure out how to manage the photo viewer’s resources; currently Cocoapods creates a resource bundle for us and the library uses that bundle to load its resources. I don’t think Carthage will build such a bundle automatically.

• Verify the Example project works 💯.

• Update the podspec to match the new directory structure and verify it still works as before (including the resource bundle, which has been a pain point for us in the past).

Those two “figure out” steps are a part of why we haven’t done this internally yet; we just haven’t spent the time to figure out the best plan of attack. They may well be issues others trivially know how to resolve with Carthage, but that I don’t offhand.

Hopefully this response is useful and can help guide a future attempt, whether that comes from someone else or from my own team at some other time.

Edit: oh, and IMO #62 belongs somewhere on that checklist.

[davbeck]

Figure out how to manage conditionally-compiled animated GIF support. Currently GIF support is compiled out if you don’t include the AnimatedGifSupport subspec, and ideally we wouldn't force clients to include this support when it’s not desired.

Unfortunately subspac like behavior is something the Carthage team explicitly does not want to support. My solution would be to include it by default, including adding it as a dependency to the frameworks Cartfile and making 2 framework targets, 1 called "NYTPhotoViewer" that links to and includes GIF support and "NYTPhotoViewerCore" which excludes it. User's of the framework that did not want to include GIF support would link against Core and ignore FLAnimatedImage.

The big downside here is that the carthage commands will still download and build FLAnimatedImage, as well as both frameworks, but the alternative would require separate repositories. The nice things about Carthage though is it only builds dependencies once and then reuses them across builds of the app, so it shouldn't add too much overhead like Cocoapods would.

[cdzombak]

Thank you very much for your comments above and your work on #164, @davbeck! I have merged your PR #164, adding Carthage support and closing this issue along with #62.

Before merging I checked the following:

• Reviewed the new project structure & targets & build scripts & etc. for overall sanity and a structure that makes sense, verifying everything seems to build and work as expected.
• Specifically, verified your solutions for the problems I mentioned in Carthage support #61 (comment) work as expected (they seem to! thank you!) and that the rest of those guidelines appear to be met.
• Verified both example targets work on-device, after a clean build.
• Tested integrating your branch into new projects via Cocoapods and Carthage.

However, I'm not quite ready to cut a release:

• This is a large change and before we cut an official release, I'd appreciate it if those who have expressed interest in this support could try this out (github "NYTimes/NYTPhotoViewer" "develop") and see how it works for them.
• While that happens, I have a few very tiny points I'll clean up in followup PRs as I find time over the next couple of days.

Finally, I want to thank everyone in this thread for their interest in the project and their patience with me, and especially @davbeck for his work in #164.