Allow "cycles" through test suite dependencies

[23Skidoo]

A version of #3232/#3290/#3402 updated to work with master. Also moved the branch to the upstream repo so that others can push to it.

Current status:

• Needs to be reworked (see Allow "cycles" through test suite dependencies #3402 (comment) and Allow "cycles" through test suite dependencies #3422 (comment)).
• Needs a review from @kosmikus.
• There's a bug in interaction with sandboxes, see Allow "cycles" through test suite dependencies (VERSION TWO) #3290 (comment). I won't have time to look into this for a while; help appreciated.
• There's also a failing test (see Allow "cycles" through test suite dependencies #3422 (comment)):

      indepGoals2:                                      FAIL
        expected: Just [("A",1),("B",1),("C",1),("D",1)]
         but got: Just [("A",1),("B",1),("C",1),("D",1),("D",2)]

[grayjay]

(See #3402 (comment))

If you or anyone can spot cases where it's not as rosy as this, please do say so.

@dcoutts Thanks for the explanation. I thought about it some more, and I think that allowing inconsistent versions of the library is fine, since it only affects test suites that would otherwise have cyclic dependencies.

I think the problem is that the PR also allows inconsistent versions of the library's dependencies. Here's an example of a package without cyclic dependencies that might be broken by this change. I haven't actually tested it, so I could be wrong. (These aren't real versions)

name: example
version: 1.0
[...]

library
  build-depends: base,
                 containers == 2.*

test-suite tests
  build-depends: base,
                 containers,
                 example,
                 quickcheck-instances,
                 QuickCheck,
                 tasty,
                 tasty-quickcheck
  [...]

Say quickcheck-instances requires containers == 1.*, but only when an automatic flag has the default value.

AFAICT, this PR would qualify the test suite dependencies that aren't shared with the library, i.e., it would qualify quickcheck-instances, but not base, containers, or example. So the tests would be built with containers-1.* and containers-2.*. If a test used quickcheck-instances to generate random containers to pass to the library, it wouldn't type check. This example would work with master, because the solver would continue searching beyond the conflict with containers, and would flip the flag on quickcheck-instances.

Forcing both instances of the library (if there's a cycle) to have consistent dependencies would be safer, and I think it would still support the use cases.

[edsko]

@23Skidoo , @grayjay I took a look at the failing test case. The test in question is we're trying to solve for A and B as independent goals using the DB

db17 :: ExampleDb
db17 = [
    Right $ exAv "A" 1 [ExAny "C"] `withTest` ExTest "test" [ExFix "D" 1]
  , Right $ exAv "B" 1 [ExAny "C"] `withTest` ExTest "test" [ExFix "D" 1]
  , Right $ exAv "C" 1 [ExAny "D"]
  , Right $ exAv "D" 1 []
  , Right $ exAv "D" 2 []
  ]

The idea of the test is that the solver first picks version 2.0 of 0.D (i.e., A's dependency on D), then decides to link 1.D (B's dependency on A), and only then discovers that actually we have a constraint on D, and so we need to back-track sufficiently to pick a different version for 1.D.

This the modification in this PR, actually, the constraint on D doesn't apply to 0.D, but is instead applied to 0.A-test.D (or B.A-test.D), and so we will actually simply install two versions of D (one for the test suite and one for the main package). The backtracking behaviour that the tests intended to test for no longer happens:

The failures we discover when we explore this tree we find immediately when trying to link, so the test is no longer testing what it was intended for. I'm not entirely sure how to come up with a different test case though; that might not be so simple.

Moreover, if @grayjay 's objection above (#3422 (comment)) is implemented, then I think the test will be fixed again. I'm not entirely sure of the implications of @grayjay 's comments, but (/cc also @dcoutts ) it is somewhat strange that if, in this example, A had a direct dependency on D we would force the test-suite's dependency on D to be the same, but if this is an indirect dependency, we do not. Not sure if I can totally justify that.

[edsko]

@23Skidoo , regarding

1. Needs to be reworked (see Allow "cycles" through test suite dependencies #3402 (comment)).

what needs to happen is that as part of the set of FlaggedDeps we store for every package (the index), we additionally record if they are shared with the main library or not. That said, again, if we decide to change tactic here and implement @grayjay 's suggestion (#3422 (comment)) this will be a non-issue.

[grayjay]

The failures we discover when we explore this tree we find immediately when trying to link, so the test is no longer testing what it was intended for. I'm not entirely sure how to come up with a different test case though; that might not be so simple.

@edsko Is there any way to defer introducing the D == 1 constraint without adding a test suite? Maybe we should add a parameter to the solver to allow unit tests to specify the goal order. A few other backjumping tests are fragile because they try to indirectly force a certain order.

[edsko]

Is there any way to defer introducing the D == 1 constraint without adding a test suite? Maybe we should add a parameter to the solver to allow unit tests to specify the goal order. A few other backjumping tests are fragile because they try to indirectly force a certain order.

@grayjay I like this suggestion. Adding some infrastructure that would enable unit tests to reorder the tree would be very useful, I think. Not exactly sure what shape this should take though. But feel free to propose something :)

[edsko]

Discussion with @dcoutts : we should only consider the direct dependencies of the library, but all dependencies. That makes more sense to me, but it would be a lot more difficult to do (at the point in the solver where we qualify we might not even know all indirect dependencies yet?). For instance, if the library depends on A, which depends on B, and the test suite depends directly on B, then the two dependencies on B should probably match (but that indirect dependency on B might depend on flag choices..).

[grayjay]

@edsko I'm not sure I understand. Are you suggesting changing the part of the PR that adds the "test" qualifier (https://github.com/haskell/cabal/pull/3422/files#diff-684698b6af4c57398afaa2c1e4194338R283) so that it avoids qualifying all dependencies of the library, instead of just the direct dependencies?

[dcoutts]

@grayjay so if I understand your example and your point correctly, you're saying the property we want to achieve is that all dependencies (direct or indirect) that are shared between a lib/exe and associated test suite / benchmark should be consistent, and that we only allow picking versions of deps independently when they are used only by the test suite / benchmarks and not the main libs/exes in the package.

So if I've understood correctly then I think @edsko and I agree. Do you have any suggestion about how we might get the solver to pick such solutions?

[grayjay]

@dcoutts That sounds right. I forgot that this PR also supports choosing different versions of test frameworks for different packages' test suites, so it is important that all of the test-only dependencies are independent from the library.

I have an idea, but it's messy, and I don't know if it will work. I'm not very familiar with how cabal currently handles a test's dependency on the library.

For a package x, we qualify the test suite's dependency on x with x-test-lib. Then we qualify every other test dependency with x-test. x-test-lib is not inherited; its dependencies are instead qualified with x-test. The library component's dependencies have no new qualifiers. Then we traverse the tree again, and for each goal y-test-lib.y, we prune all choices except for the choice to link to y. The pruning step forces each test suite to use the correct instance of the library. (It also means that we have to solve for y before y-test-lib.y. We would probably have to remove the y-test-lib.y goal node when there is no choice to link to y.)

The idea is that the solver has to solve for the library twice (ignoring cyclic dependencies), once for x and once for x-test-lib.x. x depends on the library component's dependencies (unqualified), and x-test-lib.x depends on the test component's dependencies (qualified with x-test). When the two x goals are linked, the dependencies shared between the library and test suite are also linked. This should also support cyclic dependencies because x-test-lib.x and x-test.x can have different versions.

[gbaz]

I think this PR is superseded by other work to the solver. Does that sound right?

[grayjay]

The original issue, #1575, isn't fixed yet, though this PR would probably require a lot of work to be merged. There was also discussion of solving cyclic dependencies with a different approach towards the end of #1575 (require the library to have consistent versions, but allow cycles between components).

[Kleidukos]

Marking this PR as draft 🙂