Add ability to specify source with each dependency #4486

efirestone · 2015-11-03T01:22:34Z

This adds the ability to add a :source flag to pod dependencies in a Podfile. This behavior mirrors in Gemfiles, allowing a dependency to be specified as such:

pod 'JSONKit', '1.4', :source => 'http://github.com/MyOrg/Specs.git'

Depends on CocoaPods/Core#278

efirestone · 2015-11-03T01:27:55Z

I didn't see a specific existing issue regarding this enhancement. There's some discussion here, and I can file a new issue if you want one.

segiddins · 2015-11-03T01:37:31Z

No need to file an issue

segiddins · 2015-11-03T01:39:11Z

How does this handle transitive dependencies?

jafara · 2015-11-03T03:48:12Z

👍 this is a great improvement!

efirestone · 2015-11-03T10:03:35Z

@segiddins Transitive dependencies are handled like so:

If a dependency is explicitly specified in the Podfile, then the :source repo will be used for that dependency if it comes up as a transitive dependency
If a dependency is not explicitly specified anywhere in the Podfile, then the normal file source rules apply (unchanged from before)

The simplest way to summarize is that it has the exact same behavior as a podspec given using :podspec or :path. Podfile dependencies get resolved first and are cached and reused for all subsequent requirements.

One small thing I noticed: as mentioned, the resolver relies on caching to make sure dependency resolutions are consistent. This works fine when calling resolve, but it means that if you call search_for directly before calling resolve then the wrong thing might be cached. This is true with the existing code for external sources (:path) as well.

To illustrate:

Have a Podfile with the dependency pod 'MyPod', :path => '/path/to/1.0/MyPod.podspec'
Use that podfile as a constructor argument in creating a Resolver
Before calling resolve, call search_for(Dependency.new('MyPod', '1.0'))

I would expect the resolver to give me back the Spec from the local path, but instead it will actually use the sources passed to the resolver constructor instead. If you call resolve first then the dependencies from the podfile will get passed to search_for first and will be the ones that are cached. The new :source flag behaves the same way.

Again, I think this is all fine, but I figured I'd call it out. Root cause here seems to be that search_for should be private so that you can't abuse it in this way but in reality has to be public to conform to Molinillo::SpecificationProvider.

efirestone · 2015-11-03T10:06:15Z

Also, the code has a dependency on CocoaPods/Core#278, so the tests should go green once we can point them at that. It looks like everything is pegged to HEAD, so is the standard development flow to merge the other PR first?

segiddins · 2015-11-03T15:44:27Z

You'll need to point the Gemfile in this repo to your Core PR so the tests here can pass.
And I'm still not seeing tests for what will happen to transitive dependencies of dependencies with a custom source set -- one could argue that they ought to use the source for their parent dependency, though you then get into conflict situations... Either way, that scenario needs to be sufficiently tested.

segiddins · 2015-11-03T16:38:37Z

lib/cocoapods/sources_manager.rb

+        @aggregates_by_repos ||= {}
+
+        # Use the cached version if it exists.
+        if result = @aggregates_by_repos[repos]


all this can be replaced with @aggregates_by_repos[repos] ||= Source::Aggregate.new(repos)

efirestone · 2015-11-03T21:19:23Z

spec/unit/installer/analyzer_spec.rb

@@ -62,6 +62,19 @@ module Pod
        @analyzer.update_repositories
      end

+      it 'does not update sources if there are no dependencies' do


This is a small optimization that came along with my changes. If a Podfile has no dependencies specified, then it won't bother updating any of the sources. This seems safe to me, but I'd like confirmation from the veterans on this.

efirestone · 2015-11-03T21:27:03Z

Added some transitive dependency tests. See other comments for discussion on the behavior. Also updated the Gemfile and addressed your comments. Thanks for looking it over @segiddins!

efirestone · 2015-11-09T23:31:31Z

This is ready for a review. Much appreciated!

segiddins · 2015-11-11T03:16:40Z

CHANGELOG.md

+  [Eric Firestone](https://github.com/efirestone)
+  [#4486](https://github.com/CocoaPods/CocoaPods/pull/4486)
+
+* Improve `pod search` performance while using _`--full`_ flag  


this line shouldn't be there

segiddins · 2015-11-11T03:21:20Z

Generally looks good to me, but I'd like someone else on the team to also have a look.

efirestone · 2015-11-11T05:03:53Z

Comments addressed. Thanks for looking it over.

efirestone · 2015-11-11T23:21:56Z

Who are good candidates to take a second look at this? @orta? @neonichu?

manuyavuz · 2015-11-12T18:26:11Z

CHANGELOG.md

@@ -20,6 +20,10 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
  [Marcelo Fabri](https://github.com/marcelofabri)
  [#4487](https://github.com/CocoaPods/CocoaPods/pull/4487)

+* Add support for specifying :source with a pod dependency.  


I think your changelog entry should be at the top in order to preserve historical flow.

Done and done.

neonichu · 2015-11-13T10:15:14Z

Looks good to me as well — thanks for the comprehensive comments, @efirestone 👍

manuyavuz · 2015-11-13T10:29:25Z

@efirestone, it just needs a rebase to be 🚀. Thank you for your effort !

manuyavuz · 2015-11-13T10:40:31Z

And you should revert the Gemfile because changes in Core repo has been merged.

Enables specifying a :source flag with each pod dependency

efirestone · 2015-11-13T19:07:02Z

Should be good to go! Thanks all!

neonichu · 2015-11-13T19:09:06Z

Awesome, thank you 👍

Add ability to specify source with each dependency

efirestone force-pushed the firestone/source-flag branch from f172105 to 6469b10 Compare November 3, 2015 01:24

efirestone mentioned this pull request Nov 3, 2015

Add ability to specify an explicit source for a given dependency CocoaPods/Core#278

Merged

efirestone force-pushed the firestone/source-flag branch from 6469b10 to 9959bb4 Compare November 3, 2015 10:23

segiddins reviewed Nov 3, 2015
View reviewed changes

efirestone force-pushed the firestone/source-flag branch 2 times, most recently from fa9358e to f2ee737 Compare November 3, 2015 21:17

efirestone reviewed Nov 3, 2015
View reviewed changes

efirestone force-pushed the firestone/source-flag branch from f2ee737 to 0124950 Compare November 9, 2015 18:38

efirestone closed this Nov 9, 2015

efirestone reopened this Nov 9, 2015

efirestone force-pushed the firestone/source-flag branch from 0124950 to 1d8c703 Compare November 9, 2015 22:58

efirestone force-pushed the firestone/source-flag branch from 1d8c703 to 41d4864 Compare November 11, 2015 00:32

segiddins reviewed Nov 11, 2015
View reviewed changes

efirestone force-pushed the firestone/source-flag branch from 41d4864 to 7334c0d Compare November 11, 2015 04:45

efirestone force-pushed the firestone/source-flag branch from 88b4147 to 5ee82c3 Compare November 12, 2015 18:16

manuyavuz reviewed Nov 12, 2015
View reviewed changes

efirestone force-pushed the firestone/source-flag branch from 5ee82c3 to 7d490de Compare November 12, 2015 18:31

Add ability to specify source with each dependency

ccfde78

Enables specifying a :source flag with each pod dependency

efirestone force-pushed the firestone/source-flag branch from 7d490de to ccfde78 Compare November 13, 2015 18:30

neonichu added a commit that referenced this pull request Nov 13, 2015

Merge pull request #4486 from efirestone/firestone/source-flag

2e89214

Add ability to specify source with each dependency

neonichu merged commit 2e89214 into CocoaPods:master Nov 13, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to specify source with each dependency #4486

Add ability to specify source with each dependency #4486

efirestone commented Nov 3, 2015

efirestone commented Nov 3, 2015

segiddins commented Nov 3, 2015

segiddins commented Nov 3, 2015

jafara commented Nov 3, 2015

efirestone commented Nov 3, 2015

efirestone commented Nov 3, 2015

segiddins commented Nov 3, 2015

segiddins Nov 3, 2015

efirestone Nov 3, 2015

efirestone Nov 3, 2015

efirestone commented Nov 3, 2015

efirestone commented Nov 9, 2015

segiddins Nov 11, 2015

segiddins commented Nov 11, 2015

efirestone commented Nov 11, 2015

efirestone commented Nov 11, 2015

manuyavuz Nov 12, 2015

efirestone Nov 12, 2015

neonichu commented Nov 13, 2015

manuyavuz commented Nov 13, 2015

manuyavuz commented Nov 13, 2015

efirestone commented Nov 13, 2015

neonichu commented Nov 13, 2015

Add ability to specify source with each dependency #4486

Add ability to specify source with each dependency #4486

Conversation

efirestone commented Nov 3, 2015

efirestone commented Nov 3, 2015

segiddins commented Nov 3, 2015

segiddins commented Nov 3, 2015

jafara commented Nov 3, 2015

efirestone commented Nov 3, 2015

efirestone commented Nov 3, 2015

segiddins commented Nov 3, 2015

segiddins Nov 3, 2015

Choose a reason for hiding this comment

efirestone Nov 3, 2015

Choose a reason for hiding this comment

efirestone Nov 3, 2015

Choose a reason for hiding this comment

efirestone commented Nov 3, 2015

efirestone commented Nov 9, 2015

segiddins Nov 11, 2015

Choose a reason for hiding this comment

segiddins commented Nov 11, 2015

efirestone commented Nov 11, 2015

efirestone commented Nov 11, 2015

manuyavuz Nov 12, 2015

Choose a reason for hiding this comment

efirestone Nov 12, 2015

Choose a reason for hiding this comment

neonichu commented Nov 13, 2015

manuyavuz commented Nov 13, 2015

manuyavuz commented Nov 13, 2015

efirestone commented Nov 13, 2015

neonichu commented Nov 13, 2015