Detect cycles in solver

[edsko]

(Only one commit in this PR; the first commit is really #3167.)

The solver currently does not attempt to detect cyclic solutions; for instance, if we have a package DB where A depends on B and B depends on A, the solver will happily return a "solution" where A and B are both installed; this will then subsequently lead to an internal error when we verify the derived install plan.

This wasn't a major deal until now because the package DB should not really contain cycles in the first place. However, as part of the work on qualified goals, this is becoming increasingly important. Even if we limit the number of qualifiers (#3160), if we want to allow a solution where, say (#1575)

• optparse-applicative's test suite gets built against tasty
• tasty gets built against an older version of optparse-applicative

it is important that the solver rejects a truly cyclic solution; but for that to work, it needs to be able to spot that it found a solution and try and come up with a different solution.

This PR adds another pass to the solver to detect and reject cycles (and construct the appropriate conflict set to make sure that when the solver backjumps it is able to make different choices that could potentially break the cycle). Since the cycle check is relatively expensive (linear in the size of the solution), we do this only once we have found a solution. This means that for most use cases the performance should not really be affected; after all, right now if the solver would have found a solution we would then result in internal error.

The PR also adds a few tests:

• The first test has a very simple DB with a direct cycle between packages A and B, and asks the solver to solve for A, and expects the solver to say that there is no solution.
• The next test does the same thing, but now asks the solver to solve for both A and B; this might matter, because now the goal reason chains are different. Again, we expect the solver to find no solution.
• The final test has package C depending on either D or E, depending on flag assignment, but defaulting to depending on D. Package D in turn depends on C, but E does not. Hence, the solver must detect the cycle, reject the solution, and return C-flag, E as the solution. This is effectively what must happen in the optparse-applicative/tasty cycle above, except that for that to work we additionally need to make sure to qualify (private) dependencies of test suites, which this PR does not yet address.

This PR is an important step towards solving #3160.

[dcoutts]

I reviewed the design of this with @edsko and looked at the core bit of the cycle detection code.

We believe, but have not tested, that there is minimal performance impact since we only do the cycle detection when we would otherwise return an invalid solution, so in typical current cases this should happen at most once. Currently every place where the cycle check fails (which are the cases where it could go on to do more than one) the current solver just fails outright, which is not something we see often in practice.

[23Skidoo]

Build bot failures are due to missing import of <$>.

[edsko]

Imported <$> for base < 4.8.

[23Skidoo]

Patches look good to me, but would be nice if @kosmikus could comment.

[edsko]

Added Haddock comments as requested.

[cstrahan]

Find any reject any solutions that are cyclic

The first "any" should probably read "and".

[23Skidoo]

@edsko Do you want this to go into 1.24 or do you think it can wait?

[hvr]

@23Skidoo if I understand the issue correctly, this is needed to have setup-depends work properly (which will be interpreted by Cabal 1.24+ even if they're present although just says cabal-version: >=1.10)

[edsko]

I think it's quite important that this goes in, although it only addresses part of the problem. The other component is limiting the number of qualifiers. I won't have time to look at that until next week though, unfortunately. Maybe have a chat with @dcoutts, he's aware of the issues.

On 1 Mar 2016, at 16:39, Mikhail Glushenkov [email protected] wrote:

@edsko Do you want this to go into 1.24 or do you think it can wait?

—
Reply to this email directly or view it on GitHub.

[23Skidoo]

OK. Still would like to see a review from @kosmikus before merging, but it's probably not forthcoming.

[kosmikus]

@23Skidoo Sorry. Will see what I can do. If there's no further comment from me until the end of the week, then it's unfortunately unlikely to happen the week after either ... But I'll try to have a look tomorrow.

[23Skidoo]

Given that the GHC 8 release has been delayed, 1.24 final won't be out until the end of this month most likely, so we have time.

[kosmikus]

@23Skidoo I am going to merge this with minor modifications. Changes look good to me. I've also run a superficial test trying to install each package from Hackage on a clean package DB using ghc-7.10.3, and there seem to be no deviations in the results, and no significant deviations in the runtime or memory consumption. Note that in this test, there's most likely not a single case where cycles are actually detected, but at least this gives some amount of trust that we do not pay much for this in the common case.

@dcoutts, @edsko In general, I think this PR could possibly be improved ever so slightly by not collecting what feels like redundant information just for the cycle check, but I'm not going to block this PR based on it. It's documented clearly enough, and seems minimally invasive to the rest, so I think it should hopefully be a clear relative improvement.

[23Skidoo]

@kosmikus Great, thanks!

[23Skidoo]

Also cherry-picked into 1.24.