Allow "cycles" through test suite dependencies

[edsko]

NOTE: This PR consists of three commits, not five. The first two commits below are #3220 .

Cycles through test suites are not uncommon. For example:

• optparse-applicative might want to have a test suite that uses tasty, but tasty has a dependency on optparse-applicative.
• containers might want to have a test suite that uses tasty, but tasty has a dependency on containers

Issue #1575 suggests to do resolution on a component-by-component basis, and that would still be nice to have; but it would require more significant changes to the rest of cabal (in particular, to the execution of install plans). This PR is less ambitious: in the examples above, we would allow the dependency of tasty on optparse-applicative/containers to be resolved by using a different (probably, though not necessarily, older, and possibly, though not necessarily, already installed) version. That is, the resulting executable would be linked against two versions of the libraries: the test suite itself is of course testing this version of optparse-applicative/containers, but the dependency of tasty is satisfied by a different version of those libraries.

One might even argue that this is the right solution to this particular problem (rather than doing per-component resolution): do we really want to test a new version of containers (possibly containing bugs) by using a test framework linked against that very same, possibly broken, version of containers?

This PR consists of three orthogonal commits:

• The first simply introduces some unit tests (in the EDSL for testing the solver) for these cycles through test suites, testing both the situation where the older version of the library is already installed and where it is not, as well as testing both the situation where the test suite declares an explicit internal dependency on the library and where it does not.
• The next is the core of this PR: we add qualifiers to the dependencies of a test suite section, but only those that are not shared with the main library (or are internal). It would be terribly confusing if the test suite would get linked against an older version instead of the library that we are developing!
• The final one relaxes the internal consistency check for install plans. It used to check that all dependencies are consistent; but that turns out to be too strict now; we might, under certain circumstances (like the above) allow a test suite (or executable, or..) to be linked against multiple versions of a single package. In the relaxed version we just insist on consistency for libraries.

Pinging @dcoutts , @kosmikus ; @tibbe and @pcapriotti might also be interested.

[edsko]

If this is deemed an acceptable solution to #1575 then we can probably also address #2614.

[23Skidoo]

I guess we should also give benchmarks the same treatment. Would be nice to also have an integration test for this feature.

[ezyang]

This needs docs.

[edsko]

What kind of docs?

[ezyang]

"we add qualifiers to the dependencies of a test suite section, but only those that are not shared with the main library (or are internal)" --> From the user's perspective, what does this mean? What behavior should I expect Cabal to have? Under what circumstances will this not "do the right thing"?

[23Skidoo]

Yes, would be nice if the manual section on test suites said something about cycles. The relevant file is developing-packages.markdown.

[dcoutts]

I've merged these patches to the nix-local-build branch (along with all the other recent solver PRs).

[edsko]

@ezyang @23Skidoo pushed a commit that adds a subsection to the section on test suites, explaining both the rationale and what exactly is and isn't supported. Let me know if it's insufficient.

@dcoutts Not sure if you want to cherry-pick this extra commit as well, just in case we might merge this PR through the nix-local-branch build and might otherwise be forgotten?

[23Skidoo]

@edsko Can you look into the build bot failure?

[edsko]

@23Skidoo ComponentLib has an argument now? That's something that this commit predates. Related to convenience libraries perhaps? @dcoutts has already merged this into his branch, not sure if he fixed this issue. there were no build failures on this PR before I pushed the additional commit, which contains just the additional manual section.

[edsko]

@23Skidoo I briefly looked at the integration tests in cabal-install/test but it's not entirely clear to me what the right way to go about this is. Should I write two dummy packages that model the tasty-containers cycle, and then install them into a sandbox?

[ezyang]

@edsko I'm pretty sure I fixed this problem when I merged nix-local-build-1.25.x but it might be a little difficult to figure out what the extra patch is.

[ezyang]

Try this:

diff --git a/cabal-install/Distribution/Client/Dependency/Modular/Dependency.hs b/cabal-install/Distribution/Client/Dependency/Modular/Dependency.hs
index 35a1a56..0700fd4 100644
--- a/cabal-install/Distribution/Client/Dependency/Modular/Dependency.hs
+++ b/cabal-install/Distribution/Client/Dependency/Modular/Dependency.hs
@@ -315,8 +315,8 @@ qualifyDeps QO{..} (Q pp@(PP ns q) pn) allDeps = go allDeps
     isInternalDep dep = dep == pn

     maybeLibDep :: (Dep PN, Component) -> Maybe PN
-    maybeLibDep (Dep qpn _ci, ComponentLib) = Just qpn
-    maybeLibDep _otherwise                  = Nothing
+    maybeLibDep (Dep qpn _ci, ComponentLib _) = Just qpn
+    maybeLibDep _otherwise                    = Nothing

 {-------------------------------------------------------------------------------
   Setting/forgetting the Component

[ezyang]

To be clear: this PR adjusts the solver so that it can select different versions of the library in an executable, but it doesn't adjust Cabal the library so that it will accept this (or maybe it will accept it?)

EDIT: I now understand that this will work with the Cabal library unchanged, although for a non-obvious reason which I recorded in #3286.

[edsko]

@23Skidoo / @ezyang I've just added an integration test that tests whether this actually works (and it does), although I do have to admit the subtleties @ezyang reports in #3286 did not occur to me.

@23Skidoo I'm a little confused why this PR would result in compile errors; perhaps Cabal changed, but not in this branch.. I mean, when I check out this branch and make sure to use the version of Cabal from the same commit hash everything compiles and works just fine..?

Anyway, I think this is already integrated somewhere? If so, would just need to cherry-pick the two additional commits for the user manual and the integration test.

@23Skidoo Like I said before, not 100% sure what the correct approach to writing these integration tests is. Let me know if what I did is not correct.

[ezyang]

@edsko It's because it merged cleanly. When GitHub runs Travis, it first merges, and then runs it. But the clean merge is a lie! You need to apply my patch.

Re the integration tests, some minor style guidelines:

1. You should merge with HEAD; I made some updates to make the tests easier to write; there's no more need for a common.sh or making a should_run directory.
2. I like to delete the LICENSE and blank Changelog files because they don't add much.
3. You need to run ../Cabal/misc/gen-extra-source-files.sh cabal-install.cabal to update the extra-source-files at cabal-install.

Otherwise it looks fine. There are some infelicities with how to set up tests (e.g., finding executables) which someone should eventually fix, but this should be fine for now.

[edsko]

@ezyang I've tried rebasing the test but after doing that it's now failing with

      Resolving dependencies...
      Notice: installing into a sandbox located at
      /tmp/cabal-install-test-5422/.cabal-sandbox
      Configuring A-1...
      Building A-1...
      Installed A-1
      Configuring T-1...
      Failed to install T-1
      Build log ( /tmp/cabal-install-test-5422/.cabal-sandbox/logs/T-1.log ):
      cabal: Entering directory '/tmp/cabal-install-test-5422/deps/T'
      Configuring T-1...
      cabal: The following package dependencies were requested
      --dependency='A=.fake.A-1'
      however the given installed package instance does not exist.

does that ring any bells?

[ezyang]

Well, the message means that somehow the fake ID wasn't fixed up after A was installed. I... don't think I pushed any commits which should have broken this? But if that's what you say, it must be true...

[ezyang]

(Travis failed because you need to run ../Cabal/misc/gen-extra-source-files.sh cabal-install.cabal!)

[ezyang]

@edsko I'll take a look at your other failure and see if I can fix it.

[ezyang]

There appears to be an unrelated problem, which is that the sandbox code is checking for consistent dependencies which isn't going to work. The way to reproduce is to try to install in the sandbox twice:

ezyang@sabre:/tmp/cabal-install-test-20808$ ~/Dev/cabal-tmp/dist-newstyle/build/cabal-install-1.25.0.0/build/cabal/cabal install --enable-tests
Some add-source dependencies have been modified. They will be reinstalled...
Resolving dependencies...
cabal: Could not resolve dependencies:
next goal: A (user goal)
rejecting: A-2 (constraint from modified add-source dependency requires ==1)
rejecting: A-1 (constraint from user target requires ==2)
Dependency tree exhaustively searched.

Note: when using a sandbox, all packages are required to have consistent
dependencies. Try reinstalling/unregistering the offending packages or
recreating the sandbox.

[ezyang]

OK the bug is my problem; it's fixed in some in flight commits but I'll just fix it since those commits are under review.

[ezyang]

Test confirmed to work with #3283. I'll merge this and then rebase your set.

[23Skidoo]

@edsko Which ones of your solver PRs do you want included in 1.24? I'm a bit hesitant merging them so close to release given that there has been no review from @kosmikus and there are still some bugs according to @ezyang's comment in #3290. Can they wait for the 1.24.1 cabal-install release (which will include a technical preview of nix-local-build work)?

[ezyang]

Not this one, but the rest seem to be working reasonably well. I would like to see #3268 go in because it definitely fixes a bug.

[23Skidoo]

Current status: #3290 (comment).

[23Skidoo]

Closing in favour of #3402.