Improve algorithm for choosing flags with ./Setup configure

[grayjay]

This pull request fixes the space leak in #2777 and should also improve the run time in many cases. The new algorithm assigns one flag at a time and backtracks when a flag introduces a dependency that is unavailable, instead of always trying every flag combination until one succeeds.

The first commit changes the search algorithm, and the second commit updates error messages. Previously, Cabal printed the shortest list of missing dependencies from a single complete flag assignment. Now it is easier to take the union of all dependencies that caused Cabal to backtrack. Is this change okay? Here is an example:

library
  if flag(flag1)
    build-depends: unknown-package1
    if flag(flag2)
      build-depends: unknown-package2
    else
      build-depends: unknown-package3
  else
    build-depends: unknown-package4
    if flag(flag3)
      build-depends: unknown-package5
    else
      build-depends: unknown-package6

./Setup configure on master prints:

Configuring example-0.1.0.0...
Setup: At least the following dependencies are missing:
unknown-package1 -any, unknown-package2 -any

This branch only lists the dependencies introduced by flag1, because there is no need to search farther:

Configuring example-0.1.0.0...
Setup: At least one of the following dependencies is missing:
unknown-package1 -any, unknown-package4 -any

EDIT: I think that the new error message is a little confusing, especially when a package has no flags, but I'm not sure how to reword it.

[23Skidoo]

Travis failure seems to be due to Data.Map.Strict module missing on 7.4.2.

[23Skidoo]

@grayjay Our policy is that Cabal should be buildable out of the box on all supported versions of GHC. Can you please make your patch compatible with containers-0.4?

[grayjay]

@23Skidoo I fixed it. Would you like me to combine the last two commits?

[grayjay]

I looked into printing a different error message when the set of missing dependencies doesn't depend on flag choices, but it would be a large change relative to the improvement in message quality. I reworded the existing message instead.

[grayjay]

I rebased and cleaned up the history. If that's a problem, I can restore it.

[23Skidoo]

@grayjay OK, thanks, will take another look at this soon. Sorry for the delay.

[phadej]

this is now exactly the same as http://hackage.haskell.org/package/containers-0.5.6.3/docs/Data-Tree.html

[grayjay]

Thanks! I just updated the PR.

[dcoutts]

@kosmikus could you have a look at this one too.

[joeyh]

As the one hit hard by this bug, I really appreciate the work here to fix it. So, I kind of hate to raise this possibility, but..

It's possible for setting flag A xor B to yield an unavailable dependency, while setting flag A and B yields a dependency that is available.

 if flag(A) && flag(B)
   Build-Depends: preferred-thing
 else
   if flag(A) || flag(B)
     Build-Depends: unavailable-thing

Sounds like the backtracking introduced in this patch would cause the resolver to see A alone is bad, and never try A and B, so never find the preferred dependency.

I have not verified this is the case, and I don't know if such a thing appears in real-world cabal files.

[ezyang]

@joeyh I think this actually should not matter; the ./Setup resolver is not actually used for any serious flag selection unless you manually call ./Setup configure. If you use "cabal configure", cabal-install's solver will be use which can actually do a proper job of it.

[grayjay]

@joeyh I just tried the test case, and it seems to work. ./Setup configure either chooses true for both flags or false for both flags, depending on their defaults. The behavior is the same as on master.

[23Skidoo]

the ./Setup resolver is not actually used for any serious flag selection unless you manually call ./Setup configure.

There is one case in which it's actually useful. When cabal configure can't find a solution using the cabal-install solver, it calls ./Setup configure. And in the case of the containers test suite (which itself depends on containers) it turns out that the dumb solver can find a solution, while the smart one can't. See #1806 for details.

[grayjay]

I discovered a problem when rebasing this onto master. The backtracking optimization is incompatible with #2731. For example, the unavailable dependency outside of a conditional causes configure to fail immediately on this branch, without finding that a flag choice can make it unnecessary:

executable my-executable
  build-depends: unavailable-package
  if !flag(build-exe)
    buildable: False

I haven't tried anything yet, but I see a few options:

• I could make a PR that only fixes the space leak. That would be easy, but it wouldn't allow Setup to configure git-annex in a reasonable amount of time.
• We could apply the cabal-install solver changes from Ignore dependencies that are not Buildable #2731 to Cabal, i.e., add an extra conditional at the top level of each component that represents the condition for which the component is Buildable. I don't know how difficult that would be. It could easily be several times harder than my original change, so I probably wouldn't have time to fix it before the next release. I think it would work for git-annex, though. git-annex doesn't use Buildable, and Ignore dependencies that are not Buildable #2731 has an optimization that allows it to make no additional consistency checks in the case where Buildable is always true.
• We could only apply this PR's backtracking optimization when a package has no Buildable fields. It would be relatively easy to support both types of backtracking, since the backtracking part of this change is only four lines of code. That would work for git-annex, but it's not a complete solution.

[joeyh]

Kristen Kozak wrote:

• I could make a PR that only fixes the space leak. That would be easy, but
it wouldn't allow Setup to configure git-annex in a reasonable amount of
time.

If you mean it would need to exhastively check 2^21 combinations of flags
in the case of git-annex until it found a solution, I think that would
probably be ok.

Even if it turned out to be too slow in such a case, that would be much
less annoying then OOMing.

see shy jo

[grayjay]

@joeyh Thanks for the feedback. Yes, it would have to try all combinations until it found a solution. I'll make a PR with just the space leak fix when I have time.

[23Skidoo]

Closing in favour of #3076.

@grayjay If you come up with further improvements, please open a separate PR.

[grayjay]

@23Skidoo I tried updating this to work with master, and it actually wasn't too hard. I'll make another pull request.