Save circular dependency errors til resolution finishes

[iangreenleaf]

This treats circular dependency errors as regular conflicts rather than aborting immediately. If no resolution is found, it will throw the relevant error at the end, thus still providing a useful error message in most cases.

This seems to work well in most cases, but unfortunately the spec uncovered an unrelated bug in the backjumping algorithm so it's not passing on all indexes yet.

[segiddins]

I think your integration specs submodule needs a rebase, as it seems to have lost a test case

[segiddins]

it'd be cool if we could avoid repeating this logic

[iangreenleaf]

Agreed. I wasn't sure of the best way to handle it. It seems like this logic might belong more inside unwind_for_conflict than here, but that method is used in a couple other places and I'm not quite familiar enough with the intricacies of its use to know if that was the right call or not.

[segiddins]

rubocop won't like this

[segiddins]

spaces around =

[segiddins]

Want to throw that other spec into the submodule? It looks like an easier one to debug on than the larger index

[segiddins]

The issue with the two failing specs appears to be that state_index_for_unwind would like the existing_requirement to be the fog-softlayer one that causes the circular dependency

[segiddins]

diff --git a/lib/molinillo/resolution.rb b/lib/molinillo/resolution.rb
index 4f4615f..4ef1d80 100644
--- a/lib/molinillo/resolution.rb
+++ b/lib/molinillo/resolution.rb
@@ -138,15 +138,18 @@ module Molinillo
           begin
             attempt_to_activate
           rescue CircularDependencyError => e
-            create_conflict(e)
-            unwind_for_conflict until possibility && state.is_a?(DependencyState)
+            create_conflict_and_unwind(e)
           end
         else
-          create_conflict if state.is_a? PossibilityState
-          unwind_for_conflict until possibility && state.is_a?(DependencyState)
+          create_conflict_and_unwind
         end
       end
 
+      def create_conflict_and_unwind(root_error = nil)
+        create_conflict(root_error) if state.is_a? PossibilityState
+        unwind_for_conflict until possibility && state.is_a?(DependencyState)
+      end
+
       # @return [Object] the current possibility that the resolution is trying
       #   to activate
       def possibility
@@ -188,7 +191,7 @@ module Molinillo
         conflicts.tap do |c|
           sliced_states = states.slice!((state_index_for_unwind + 1)..-1)
           unless state
-            error = c.values.map(&:root_error).detect {|e| ! e.nil? }
+            error = c.values.map(&:root_error).find(&:itself)
             raise error || VersionConflict.new(c)
           end
           activated.rewind_to(sliced_states.first || :initial_state) if sliced_states
@@ -216,6 +219,10 @@ module Molinillo
           end
         end
 
+        if current_requirement == existing_requirement && index == -1
+          index = states.length - 3
+        end
+
         index
       end
 
@@ -250,7 +257,7 @@ module Molinillo
 
       # @return [Conflict] a {Conflict} that reflects the failure to activate
       #   the {#possibility} in conjunction with the current {#state}
-      def create_conflict(root_error=nil)
+      def create_conflict(root_error = nil)
         vertex = activated.vertex_named(name)
         locked_requirement = locked_requirement_named(name)
 
diff --git a/spec/resolver_integration_specs b/spec/resolver_integration_specs
index a5315c3..4b05041 160000
--- a/spec/resolver_integration_specs
+++ b/spec/resolver_integration_specs
@@ -1 +1 @@
-Subproject commit a5315c384f91e51cb635cfcb2b6b0c271de76d65
+Subproject commit 4b050415cb747738c326aa32f5d089ea2bc1f1da

[iangreenleaf]

Ok, pushed a new rebased submodule that's up to date and has that other spec. I agree, that one exposes the bug a little more simply without the circular deps muddying the waters (and that one fails on master too).

I do have a branch where I've been experimenting with an alternative backjumping algorithm, but it's part of some more extensive changes. I'll take a look at that next and see if I can extract that one piece from the branch.

[segiddins]

Note that my diff fixes the issue for the circular case, just not the buckjumping case

[segiddins]

This might make failing resolutions take a tad longer, but it seems to fix the spec failures!
(It does seem to break 3 cocoapods specs that depend upon other info in the conflict, such as the possibility)

diff --git a/lib/molinillo/resolution.rb b/lib/molinillo/resolution.rb
index 4f4615f..b0e73bc 100644
--- a/lib/molinillo/resolution.rb
+++ b/lib/molinillo/resolution.rb
@@ -216,7 +216,7 @@ module Molinillo
           end
         end
 
-        index
+        index == -1 ? states.size - 2 : index
       end
 
       # @return [Object] the requirement that led to `requirement` being added

[iangreenleaf]

I'm not positive, but I'm guessing that solution would still leave open cases where the backtracking jumps over a culprit to try an unsuccessful branch higher on the stack, then runs out of options and fails to resolve.

FYI I'm working on an alternative backtracking choice on a branch, but haven't worked out all the kinks yet: https://github.com/iangreenleaf/Molinillo/tree/backjumping-bug

[segiddins]

Well it passed your test, so that's all we can ask 😝