Fix construction of conflict sets during check for single instance restriction (maybe) #3221
Conversation
|
So I thought I had an "in the wild" test case for this. My notes were in cabal-install.cabal: That is, adding a custom setup section to cabal-install and claiming it has a dep on that version of text. The solver log I saw made me think it was a problem with linking. It would consider some version of text with one set of flags, fail (legitimately) but then never try text with the different set of flags or with tests disabled. So in retrospect my suspicion is that it's not a linking bug but this bug about not having the right conflict sets so that backtracking does not let it consider the choice that would have worked. However I now cannot reproduce the solver failure. I may be able to with a much older revision. I may find time to try that. The stash I had mentions commit ad6eed5, so that may be worth trying. |
|
@grayjay BTW, well done on identifying this bug and isolating the test case. |
|
Thanks! |
|
If @kosmikus won't have time to review this before the 1.24 release, this PR will have to wait until the next point release of |
|
@23Skidoo What is my current deadline for the 1.24 release? |
|
@kosmikus I plan to make the 1.24 release simultaneously with GHC 8 final, which will be released around the end of March/beginning of April (I think). |
|
@23Skidoo Ok, then I'm still somewhat hopeful that I will make it. |
|
So I think this is a step in the right direction. The original code is clearly wrong. The new construction does still look strange, though. In an SIR-disabled choice, there are two variables that contribute to the conflict, which is reflected by the original code. They're called Now we cannot just add the two variables on their own, but we have to add their goal reason chains as well (because changing something that lead up to the introduction of a goal may mean the goal never gets introduced, and therefore may allow progress). The current PR adds Unfortunately, it's probably quite tricky (or even impossible) to come up with a test case where the omission of |
In the original implemented we recorded only the two conflicting packages; in the "fix" in #3221 we include the GRC for one of the two packages, but not the other. This is also wrong, although slightly less so and @kosmikus tells me that it's actually rather difficult to find an example that exposes the remaining inaccuracy. I'll have to take him at his word on that :) In this commit we use the GRC for both goals instead.
|
This has been merged into and subsumed by #3357 . |
In the original implemented we recorded only the two conflicting packages; in the "fix" in #3221 we include the GRC for one of the two packages, but not the other. This is also wrong, although slightly less so and @kosmikus tells me that it's actually rather difficult to find an example that exposes the remaining inaccuracy. I'll have to take him at his word on that :) In this commit we use the GRC for both goals instead. (cherry picked from commit 3bc157a)
In the original implemented we recorded only the two conflicting packages; in the "fix" in haskell#3221 we include the GRC for one of the two packages, but not the other. This is also wrong, although slightly less so and @kosmikus tells me that it's actually rather difficult to find an example that exposes the remaining inaccuracy. I'll have to take him at his word on that :) In this commit we use the GRC for both goals instead. (cherry picked from commit 3bc157a)
If correct, this fixes #2843; the first commit is basically @grayjay 's detailed test case (for which many thanks!). However, this needs review by @kosmikus ; have sent a detailed email.