Added test demonstrating issue with find_in_unresolved_tree

[mfazekas]

This is just a test demonstrating the require can result in using a spec that causes a conflict.

[tenderlove]

Am I reading this correctly that resolution is basically just broken?

[tenderlove]

From what I can tell, this isn't a bug in find_in_unresolved_tree (find_in_unresolved_tree is never called in this test). It looks like it finds the two specs for b, then hits this else block that activates the latest spec. Seems like it may be an issue in the conflicts method. I'm guessing that this line is supposed to be returning nil, and that we're supposed to get an error? But I really have no idea what the correct behavior is (though the behavior outlined in your test seems more reasonable than an exception).

@evanphx your name is on this code. Do you know how to fix?

[mfazekas]

Yes i think so. This is the output.

Expected: ["a-1", "b-1", "c-2"]
  Actual: ["a-1", "b-2", "c-1"]

But in the test i set it up so that a-1 requires c>1 so the actual output is not correct.

a1 = new_spec "a", "1", "c" => "> 1", "b" => "> 0"
b1 = new_spec "b", "1", {"c" => "= 2"}, "lib/d.rb"
b2 = new_spec "b", "2", {"c" => "= 1"}, "lib/d.rb"
c1 = new_spec "c", "1"
c2 = new_spec "c", "2"
c3 = new_spec "c", "3"

I'd say there is 2 errors:
1.) find_in_unresolved_tree returns one possible solution, but does not check if that solution could result in a conflict or not. This is probably non trivial to solve.
2.) raise_if_conflicts does not check that the spec itself(c-2) is violating any dependency of an already loaded spec (a-1's c>1)

[mfazekas]

Oh yes you're right it's plain find_in_unresolved what's called here from require and not the tree. I'll need other testcase for find_in_unresolved_tree but i should be able to get similiar error there.

[tenderlove]

I'd say there is 2 errors:
1.) find_in_unresolved_tree returns one possible solution, but does not check if that solution could result in a conflict or not. This is probably non trivial to solve.
2.) raise_if_conflicts does not check that the spec itself(c-2) is violating any dependency of an already loaded spec (a-1's c>1)

Ya, that sounds about right to me. Just out of curiosity, are you hitting this error in real life? I wonder if this is a case that we could possibly punt on (maybe raise an exception or something). My gut says we might be able to solve the issue, but it will really slow down resolution (it seems like we'd essentially have to backtrack on dependencies). I wonder if Bundler gets this case right.

[tenderlove]

(IMO passing your test is the best solution, but raising an exception seems better than just activating the wrong gem)

[mfazekas]

No i'm not hitting it in realword. The optimization in #1168 changes the behaviour of find_in_unresolved_tree, and it's doing less checking. But while original tree code is potentially exponential it still does not always selects a good solution that what i wanted to prove but i need different testcase for that..

[tenderlove]

But while original tree code is potentially exponential it still does not always selects a good solution that what i wanted to prove but i need different testcase for that

Makes sense. If we know the resolution isn't correct, and we're willing to live with that, then we could probably make some nice speed improvements.

[tenderlove]

Hi, I added b19051c that drops your test to about 10ms (on my machine), then cherry picked in 33d463f. I'm not sure what the complexity of the current algorithm is yet (because I don't fully understand the code), but I was able to speed this up without any caching and without changing the (admittedly broken per #1169) behavior.

If the speed now is acceptable to you, I'll close this, then start thinking about how to fix #1169.

I commented on the wrong ticket!!!

[mfazekas]

Added a new testcase called test_self_activate_conflicting_in_indirect that should invoke find_in_unresolved_tree.

[copiousfreetime]

This appears to have been incorporated / resolved with #1188