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

Adds a podspec for CocoaPods #5

Merged
merged 14 commits into from Apr 17, 2015
Merged

Adds a podspec for CocoaPods #5

merged 14 commits into from Apr 17, 2015

Conversation

dasmer
Copy link
Member

@dasmer dasmer commented Apr 14, 2015

Adds support for CocoaPods, since many developers use CocoaPods to manage dependencies on iOS and OSX projects.

@dasmer dasmer changed the title Adds a podspec for Cocoapods Adds a podspec for CocoaPods Apr 14, 2015
@ohayon
Copy link

ohayon commented Apr 14, 2015

👍

@lazerwalker
Copy link

👍 Would love to see this merged in!

@cojoj
Copy link

cojoj commented Apr 14, 2015

👍

1 similar comment
@jgrandelli
Copy link

👍

@@ -0,0 +1,13 @@
Pod::Spec.new do |s|
s.name = 'ResearchKit'
s.version = '1.0.0'
Copy link

Choose a reason for hiding this comment

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

There's currently no 1.0.0 tag, that would need to happen, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! A maintainer would create a release (1.0.0 matches the project's build version), and pod trunk push after merging :octocat:

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we will do this to coincide.

Choose a reason for hiding this comment

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

👍

@brunokoga
Copy link

👍

2 similar comments
@bgilham
Copy link

bgilham commented Apr 14, 2015

👍

@paulomendes
Copy link

👍

@bryanbartow
Copy link

Yes, please.

@mergesort
Copy link

👍

@DanielKao
Copy link

I'd love to see this PR merged in!

@cezarywojcik
Copy link

👍

@pocketlim
Copy link

👍

1 similar comment
@tiembo
Copy link

tiembo commented Apr 14, 2015

👍

@esttorhe
Copy link

yes!!! 💥 👍

@brianmichel
Copy link

Yeah 👍

@CameronBanga
Copy link

👍 Awesome.

@codestergit
Copy link
Contributor

Yeah 👍

@dchohfi
Copy link

dchohfi commented Apr 14, 2015

👍

2 similar comments
@di0g0
Copy link

di0g0 commented Apr 14, 2015

👍

@samuele-mrapps
Copy link

👍

@gfontenot
Copy link

Dear Apple,

You can lock pull request comments to contributors with the little button to the right.

Also, I apologize on behalf of the internet.

  • gordon

@billyto
Copy link

billyto commented Apr 14, 2015

👍

@florianbachmann
Copy link

👍

s.author = { "Apple Inc." => "http://apple.com" }
s.source = { :git => 'https://github.com/ResearchKit/ResearchKit.git', :tag => "v#{s.version}"}
s.public_header_files = `./scripts/find_headers.rb --public`.split("\n")
s.private_header_files = `./scripts/find_headers.rb --private`.split("\n")
Copy link
Member

Choose a reason for hiding this comment

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

I've tested this and it doesn't do what I expected. In Xcode, frameworks can have public headers, private headers, and project headers. Here, private headers seems to be an exclusion glob against public headers, which excludes some headers from the public header directory.

I think we need to remove the private_header_files line. Sorry I mis-directed you earlier - the naming led me to expect it would behave like Xcode.

@jwe-apple
Copy link
Member

One thing I considered was whether to glob our "private" headers in with the public headers, since they're essentially "beta" API. This means users of CocoaPods won't be able to #import <ResearchKit/ResearchKit_Private.h>. My thinking was that these APIs aren't stable enough to be fully documented, so they are not stable enough for CocoaPods users and the automatically generated documentation that CocoaPods will produce.

I am open to counter-arguments!

Dasmer Singh added 4 commits April 16, 2015 22:56
[README] Drop fourth-level headers

Add Dependency Management file

[README.md] Link to dependency_management.md

Move /usr/bin/env ruby to top of script
@orta
Copy link

orta commented Apr 17, 2015

One thing I considered was whether to glob our "private" headers in with the public headers, since they're essentially "beta" API

I'd be in favour of this, you could add an array of beta-worthy files to the Podspec, add them to the public_headers and subtract from the private_headers.

Given that you're not using CocoaDocs, the only documentation we'll be generating (and showing on your behalf) is your README and metadata around size, if documented, if tested, etc. Example. As you control what goes into http://researchkit.github.io/docs/ you can expose things that are beta-like as public in the headers but not mention them in the docs, and we won't do anything to undermine that. Marking them in the header files themselves as "not quite there, usable, but liable to have changes" means you can get people testing without a full commitment.

@SlaunchaMan
Copy link

As this moves forward, the community will undoubtedly find things in ResearchKit that they want to use even outside of research applications. ORKVisualConsentTransitionAnimator is a great example; I just showed the process of applying a tint color to movies to a designer and she loved it. To that end, as someone wants to use a part of ResearchKit on its own, we could create a PR to pull it out into a subspec, so if you’re using ResearchKit as a whole, you’ll get everything, but if you just want one part you’ll just get that one part. Does anyone see a reason that wouldn’t work?

@jwe-apple
Copy link
Member

Ok, happy with Orta's suggestion. Let's drop the private_headers line and make public_headers include both the Private and Public headers from Xcode.

@SlaunchaMan We're open to this if you can make it work. In that specific case, though, it might be better to make a new project with just the bits you want (shaders, animator, etc). As long as you keep to the license terms you can then push it in a direction that's more general purpose.

Dependency Management
===========

The ResearchKit framework can also be added to you app using CocoaPods or Carthage.
Copy link
Member

Choose a reason for hiding this comment

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

to your app

…_files

[dependency_management.md] Fix typo

[Podspec] Add Xcode Public and Private files to public_header_files
@dasmer
Copy link
Member Author

dasmer commented Apr 17, 2015

@jwe-apple Dropped private_header_files and set public_header_files to include both the Public and Private headers from Xcode.

The following is an example of the JSON podspec generated after a pod install:
https://github.com/dasmer/ResearchKit/blob/daz/example_v2/samples/ORKCatalog/Pods/Local%20Podspecs/ResearchKit.podspec.json

@jwe-apple
Copy link
Member

Thanks @dasmer and everyone else who contributed! We're good to go!

jwe-apple added a commit that referenced this pull request Apr 17, 2015
Adds dependency management documentation (and CocoaPods support)
@jwe-apple jwe-apple merged commit 94a47a4 into ResearchKit:master Apr 17, 2015
Add the following line to your [Podfile](http://guides.cocoapods.org/syntax/podfile.html) and run `pod install`:

```ruby
pod 'ResearchKit', '~> 1.0'
Copy link

Choose a reason for hiding this comment

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

Just to point out, this won't actually work until the Pod is pushed to the master spec repository.

One of the owners of this project should run pod trunk push ResearchKit.podspec.

Copy link

Choose a reason for hiding this comment

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

Does having 1.0 instead of 1.0.0 be an issue? Or it should just work?

The current tag is 1.0.0.

Choose a reason for hiding this comment

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

@3lvis it's not an issue at all

@dasmer
Copy link
Member Author

dasmer commented Apr 17, 2015

@jwe-apple No problem! Now, how about you hookup Venmo iOS team with tickets to WWDC 😉

@karapigeon
Copy link

/me cries

@aenbar
Copy link

aenbar commented Apr 18, 2015

👍

2 similar comments
@aviflombaum
Copy link

👍

@davidbakertv
Copy link

👍

@DanKeen
Copy link
Member

DanKeen commented Apr 22, 2015

Thanks for all the feedback and contributions. As this has been merged, locking this pull request from further comment. If anyone has issues with the current implementation - please open a new issue :)

@ResearchKit ResearchKit locked and limited conversation to collaborators Apr 22, 2015
@jwe-apple
Copy link
Member

Thanks Dan!

httang12 pushed a commit to httang12/ResearchKit that referenced this pull request Jul 24, 2015
…ssionActiveTasks1 to develop

* commit '4a104e683280f7fdf9c4049b57280c92709d0b5e':
  updated retry logic
  Updated screen resolution for active task
  updated research kit active task 1 to collect results
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet