Solver flag handling fixes #1878
Conversation
This was an innocent-looking but severe bug that was triggered for manual flags in certain circumstances: if both choices of a manual flag are already invalidated by other decisions, then the old code was removing the nodes underneath the flag completely. As a result, there's a node with no children rather than a node with two failure-children. The fail-nodes carry important information that is used to compute how far we can backtrack. If this information is removed, strange things happen.
Package flags generate (Boolean) goals. Package flags can be dependent
on one another, in situations such as this one:
if flag(a)
...
else
if flag(b)
...
else
...
In such a scenario, it's important to record that flag b depends on flag
a. This affects conflict set generation. If something fails due to the
choice of flag b, we should not backjump beyond flag a.
While the code handling the proper insertion of goals with their correct
dependencies was always there, it was accidentally overridden by another
piece of code that created flag goals (without dependencies) for all
flags defined in a package. The reason I add flag goals separately is
because not all paths in the decision tree may contain choices for all
flags of a package. For example, if a is chosen to be True in the
example above, b does not occur at all. But flag choices may still
affect other things that aren't visible to the solver (directory
choices, exposed modules, ...), so all flags declared should always be
chosen explicitly. So we want to keep adding all flags declared in a
package as dummies, but we have to make sure to do so *before* adding
the actual dependency tree. This way, while traversing the dependency
tree, the ones occurring in dependencies will be added again, overriding
the dummies, rather than the other way round (which we used to have
before, where the dummies were overwriting the more informative
versions).
|
I will merge this to both master and 1.20. Could you also please add some unit tests to https://github.com/haskell/cabal/blob/master/cabal-install/tests/UnitTests.hs? Just invoke the solver module directly (i.e. the test doesn't have to build a whole cabal package). |
|
@tibbe Adding any meaningful tests for the solver is currently quite tricky (because there are lots of inputs going in and outputs coming out, and there aren't any proper abstractions for making this any easier). I'm working on a proper testing framework for the solver which will make it easy to add lots of test cases. If it's ok with you, I'd prefer spending my time to try to make that one happen soon rather than adding some ad-hoc tests now that will probably not help a lot. |
|
@kosmikus that's fine by me, as long as we eventually end up with some tests. :) I naively assumed it was easy given that the |
|
@tibbe True in the sense that it's all pure. But one of the input values is the package index. Constructing one by hand, without either parsing it or having appropritate abstractions, isn't fun ;) |
These are two changes fixing a clear bug in the modular dependency solver (#1855).
Both changes could cause backjumping information to be calculated incorrectly, in turn leading to backjumps that are jumping too far, discarding unsearched parts of the search space that might still contain valid solutions. The problem was observed by having occurrences where
--reorder-goalswould lead to a solution, but--max-backjumps=-1would not.It's possible that the fix in the Builder affects other install plans, because it can theoretically affect the order in which goals are chosen.
If no unforeseen problems arise, we might want to merge this to the 1.20 branch and re-release. I've tested that these changes work on top of 1.20, too.