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

Find all CocoaPods with a circular dependency #6

Closed
kylef opened this issue Oct 14, 2014 · 31 comments
Closed

Find all CocoaPods with a circular dependency #6

kylef opened this issue Oct 14, 2014 · 31 comments
Assignees

Comments

@kylef
Copy link
Contributor

kylef commented Oct 14, 2014

Find all podspecs in CocoaPods master and ensure they are not circular dependencies. A quick search and I found two manually (KFData 1.0.1/ARAnalytics).

  • Try both platforms, iOS and OS X
  • Need to run on every recursive subspec, not just top-level specs

I know this is outside the scope of Resolver. But considering I found two pod specs by core contributors to CocoaPods which have circular dependencies which have been working before. I think it's important we check how many their are and potentially fix them before we break many CocoaPods projects for our users.

@segiddins segiddins self-assigned this Oct 14, 2014
@segiddins
Copy link
Member

@kylef
Copy link
Contributor Author

kylef commented Oct 15, 2014

Thanks for getting this list @segiddins, I'll see to it that all these pod specs are fixed.

@kylef kylef assigned kylef and unassigned segiddins Oct 15, 2014
@alloy
Copy link
Member

alloy commented Oct 24, 2014

What’s the plan of action here?

@orta
Copy link
Member

orta commented Oct 24, 2014

I think it's important we check how many their are and potentially fix them before we break many CocoaPods projects for our users.

Send them PRs if they will outright cause breakages?

@segiddins
Copy link
Member

We have to fix them in the Specs repo, since they will all outright cause breakages -- circular dependencies are not possible with this resolver, which is the proper thing. See the linked gist -- every subspec version that is broken is listed.

@pietbrauer
Copy link

I might have the same issue. I ran pod install in our current Podfile and it went in an infinite loop.

$ time bundle exec pod install
Analyzing dependencies
^C[!] Cancelled

real    6m34.152s
user    6m15.978s
sys 0m5.308s

@alloy
Copy link
Member

alloy commented Nov 3, 2014

This is the experience that @pietbrauer got:

Resolving dependencies of `Podfile`
................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
^C
[!] Cancelled

@segiddins Ideally we should immediately bail on a circular dependency.

@alloy
Copy link
Member

alloy commented Nov 3, 2014

@segiddins Also, seeing as we only output during verbose mode, what about logging the requirements that the resolver is trying to resolve so that we/users can easily pinpoint where the problem is?

@pietbrauer
Copy link

I shared our Podfile for further investigation.

@kylef
Copy link
Contributor Author

kylef commented Nov 3, 2014

We do immediately fail on circular dependency and the CircularDependencyError is raised. You two are discussing a different issue.

@alloy
Copy link
Member

alloy commented Nov 3, 2014

@kylef Ace 👍 I was indeed wondering if it wasn’t a different issue.

I guess then the question is if this resolve is going to a lot of incompatible permutations and does eventually finish. @pietbrauer Can you give it a try? (i.e. just let it run for an indeterminate time and also time the amount of time taken again)

@orta
Copy link
Member

orta commented Nov 3, 2014

I've made an issue on Analytics's repo showing how to fix it.

@rivera-ernesto
Copy link

Just got this problem with one of my libraries.

Is there a good explanation why circular dependencies are evil besides the challenge to properly implement the logic to support them? I think it can be perfectly valid to require two subspecs to be installed together.

#import and such support this and the podspec validated, installed and updated just fine with 0.34.x.

@lazerwalker
Copy link

I'm not 100% sure what the concrete plan of attack is for this issue, but SlackTextViewController can safely be removed from the list, as a fix is in trunk.

@segiddins
Copy link
Member

@lazerwalker do you mean all past versions have also been fixed?

@lazerwalker
Copy link

Ah, right. Negative, just the latest version.

This is a deep rabbithole.

@kylef
Copy link
Contributor Author

kylef commented Nov 5, 2014

Started over at CocoaPods/Specs#12783

@rivera-ernesto
Copy link

@kylef I don't see how older and even current podspecs can be "fixed" without modifying their code or functioning.
For instance NBUKit has two subspecs that have to be installed together. Sorry for repeating myself but I think circular dependencies make sense and should be handled by the new framework.

@alloy
Copy link
Member

alloy commented Nov 7, 2014

Am I correct in understand that the biggest pain is for podspecs that have subspecs? Possibly related to root specs depending on subspecs and subspecs on other specs?

Basically, I would like to see simple reduced examples of the situation that we can reason about better.

@rivera-ernesto @kylef If you can provide simple examples of when it currently breaks and which examples you think should be acceptable to still work.

@segiddins In case you know of the examples that people will run into, can you provide the same and explain why it’s good for everyone that that would no longer work? (so some details that describe the improvement to users that outweighs the pain to users for having to fix those situations)

@segiddins
Copy link
Member

It is impossible to write an iterative dependency resolver that supports conflict resolution and also allows for cyclic dependencies. Cycles make it impossible to reason about the 'sources' for each specification, since there are infinitely many. That means you can't backtrack to select conflicting states. Additionally, your program will never halt because each time you add a dependency on the given cyclic spec, you will be entering an infinitely recursive section of code.

@segiddins
Copy link
Member

From a more practical perspective, if two specs 'have to be installed together', then they really are one dependency.

@rivera-ernesto
Copy link

@segiddins Unless you want to keep them as separate podspecs for clarity/reusability/logic.
Saying that they any two depending subspecs should be merged is an imposition due to framework shortcomings. Which was my point exactly.

It is impossible to write an iterative dependency resolver that supports conflict resolution and also allows for cyclic dependencies. Cycles make it impossible to reason about the 'sources' for each specification, since there are infinitely many. That means you can't backtrack to select conflicting states.

I think this is far from impossible, just like visiting graph nodes, #import implementation, etc. do.
As for the implementation I think you only need to keep track of the already required subspecs with/without the required version and check if you already have an entry before going deeper. For instance 0.34.x implementation handled those same dependencies just fine.

I can understand that it may not be a priority, but at the very least it should be marked as not implemented, instead of saying circular dependencies are errors that have to be fixed.

@segiddins
Copy link
Member

Unless you want to keep them as separate podspecs for clarity/reusability/logic.

They're not truly reusable, since they are mutually dependent -- you can't have one without the other.

Saying that they any two depending subspecs should be merged is an imposition due to framework shortcomings

Please open CocoaPods issues for any such shortcomings.

For instance 0.34.x implementation handled those same dependencies just fine

0.34.x and before used a naïve dependency resolution algorithm that had absolutely no conflict resolution -- it greedily walked the list of dependencies, and if a version of that dependency was already 'activated', it would just continue onto the next dependency.

at the very least it should be marked as not implemented, instead of saying circular dependencies are errors that have to be fixed

I disagree. Circular dependencies will not be supported in this library, because they are errors.


For further reading, see rubygems/bundler#2652.

@kylef
Copy link
Contributor Author

kylef commented Nov 10, 2014

I can see two reasons for needing circular dependencies within subspecs of a given pod (as it stands in CocoaPods at the moment):

  • ARC / NO-ARC dilema (abusing subspecs to compile non-arc and arc code which depend on each other).
  • Needing to abuse header_mappings_dir across multiple subspecs to build weird C libraries.

Both of these are hacks and the only known way to solve a certain problem (without further changes to CocoaPods).

@alloy
Copy link
Member

alloy commented Nov 10, 2014

ARC / NO-ARC dilema (abusing subspecs to compile non-arc and arc code which depend on each other).

Ok, so for this we should implement requires_arc taking a file pattern, as discussed here: CocoaPods/CocoaPods#532. (Re-opened and added to the 0.35 milestone.)

Needing to abuse header_mappings_dir across multiple subspecs to build weird C libraries.

I think you said that you did not actually find an occurrence of this in the ‘master’ spec-repo?

@rivera-ernesto
Copy link

It can also be just two subspecs that require each other:

Pod::Spec.new do |s|

    s.name = 'ServerKit'
    #...
    s.default_subspecs = 'Server'

    s.subspec 'Server' do |sub|
        #...
        sub.dependency      'ClientCore'
    end

    s.subspec 'ClientCore' do |sub|
        #...
        sub.dependency      'Server'
    end

    s.subspec 'ClientA' do |sub|
        #...
        sub.dependency      'ClientCore'
    end

    s.subspec 'ClientB' do |sub|
        #...
        sub.dependency      'ClientCore'
    end

end

Server and ClientCore (abstract implementation of a client) require each other to compile. ClientA or ClientB are optional implementations.

I don't think that merging Server and ClientCore is more than a workaround for the dependency resolving framework. Sure we can hack the current circular dependencies but even more it limits what future libraries can and can't do.

@AliSoftware
Copy link

Your example just proves that allowing circular dependencies would be bad design, as well as your Server and ClientCore subspecs/modules depending on each other would be bad library conception and design in any OOP language, right?
At least Server and ClientCire depending on each other feels like bad conception to me.

Instead one would have a common base/API/subspec that defines all common stuff like interfaces/protocols needed by everything else (let's call it Core) and have Server and ClientCore depend on it.
Regardless of CocoaPods or any other dependency resolver, just considering basic OOP principles, that's how any reasonable OOP API would be done IMHO to avoid problems and keep consistency when consuming the library. That is not even a question about the resolver but about OOP library conception itself.

@rivera-ernesto
Copy link

I don't see how OOP has anything to do with it. Simply two subspecs that depend on each other while wanting to keep them separate. Server and ClientCore could just require each others' header files, but I wouldn't move the headers to Core just for that.

Off course anyone can come up with a different solution but that is not the point.

@AliSoftware
Copy link

Server and ClientCore could just require each others' header files

I don't get how that could be a good idea and good design.

@chrisballinger
Copy link

We are having an issue with this podspec on 0.35-rc2: https://github.com/ChatSecure/XMPPFramework/blob/release/XMPPFramework.podspec.json

When we edit it to remove the circular dependency, it seems to have cached the broken version somewhere and pod install never fetches the updated podspec. We are including it as submodule via :path in our Podfile. We even deleted it from the Local Podspecs folder and it still seems to use the old version. We verified this by adding invalid non-parsable JSON in the podspec.

edit: separate issue added here: CocoaPods/CocoaPods#2828
edit2: Solved by removing Podfile.lock

@segiddins
Copy link
Member

The bulk of this work was graciously done by @kylef in CocoaPods/Specs#12783, and the remainder will become possible after my work on CocoaPods/CocoaPods#2829 is merged, so this issue is going to be closed.

In the future, issues on the Molinillo repo will be exclusively for issues with the library itself, and CocoaPods-specific issues should, as always, be opened on the CocoaPods repo itself.

@CocoaPods CocoaPods locked and limited conversation to collaborators Nov 13, 2014
kaylarose pushed a commit to kaylarose/XMPPFramework that referenced this issue Dec 1, 2014
Attempt to resolve this error on ```pod install``` and ```pod update``` with Cocoapods 0.35.0
```[!] There is a circular dependency between XMPPFramework/Core and XMPPFramework/Authentication```

- Manually add the latest Podfile from the official specs repo
- Initial attempt to remove circular dependencies
	- The 3rd party Podfile in the spec repo is maintained by a 3rd party, and aborts ```pod update``` ```pod install``` for version 0.35.0 of the ```pod``` gem.
	- This is due to the newly added dependency conflict auto-resolution
- This is a workaround to address issues breaking all update/installs for our project dependent on XMPPFramework.

The permanent solution to this is being addressed as part of:
CocoaPods/Molinillo#6 - Initial ID of problem
CocoaPods/Specs#12783 - Fixes, including XMPPFramework
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants