Allow "cycles" through test suite dependencies

[23Skidoo]

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

/cc @ezyang @edsko

[23Skidoo]

Current status:

1. Needs to be reworked (see Allow "cycles" through test suite dependencies #3402 (comment)).
2. Needs a review from @kosmikus.
3. 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.
4. There's also a failing test:

      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)]

[23Skidoo]

Hmm, apparently we have a pre-receive hook that doesn't allow force-pushing to all branches (not just protected ones).

[hvr]

I just tried this against time-1.6.0.1 from Git w/ GHC 7.10... but I'm not sure this works as expected: the in-place lib:time-1.6.0.1 seems to be only used by time's testsuite, but not by its dependencies such as random/tf-random etc...

if I interpret the nm output right, tests is in fact linked against two versions of time:

$ nm -g ./dist-newstyle/build/time-1.6.0.1/build/tests/tests | grep DataziTimeziCalendarziWeekDate_fromWeekDate2
0000000000a5d0c8 D timezuFTheb6LSxyX1UABIbBXRfn_DataziTimeziCalendarziWeekDate_fromWeekDate2_closure
0000000000a30028 D z34UAg7ZZnVmmgR6Tt8tOJ3oLE_DataziTimeziCalendarziWeekDate_fromWeekDate2_closure

/cc @ezyang

EDIT: where the first symbol comes from /opt/ghc/7.10.3/lib/ghc-7.10.3/time_FTheb6LSxyX1UABIbBXRfn/libHStime-1.5.0.1-FTheb6LSxyX1UABIbBXRfn.a i.e. the GHC-bundled time, and the 2nd symbols comes from ./dist-newstyle/build/time-1.6.0.1/build/libHStime-1.6.0.1-inplace.a

[kosmikus]

I'm not completely sure why this is perceived to be blocked by me. There seem to be more non-solver changes than solver changes here, and there was some discussion that the patch is still buggy, which at least according to @edsko is unlikely to be due to the solver part.

For me to contribute further on this, I'd need someone who knows the history of this patch and understands the changes that are made to the non-solver part to explain to me what the issue is and what we are supposed to achieve here.

[23Skidoo]

@kosmikus Can you take a look at the solver parts of this PR at least? IIUC, the solver side is considered complete.

[23Skidoo]

@kosmikus

For me to contribute further on this, I'd need someone who knows the history of this patch and understands the changes that are made to the non-solver part to explain to me what the issue is and what we are supposed to achieve here.

The rationale for this change is given in #3232. There's also some documentation included in the PR itself. @edsko is the one to talk to about the implementation details.

[kosmikus]

Ok, perhaps I was confused then. I thought others had worked on this as well, and that it had something to do with sandboxes.

[hvr]

Now I see that in #3232 it's mentioned that linking against multiple library versions is by design

[grayjay]

I think this is inconsistent with #3357. It added requalifyDeps, which can call qualifyDeps on only a subset of dependencies:

cabal/cabal-install/Distribution/Solver/Modular/Linking.hs

Line 290 in 37325ca

rdeps <- requalify deps

[grayjay]

Should this feature be controlled by a flag, since most test suites don't have cyclic dependencies, and inconsistent dependencies can lead to compile errors?

[edsko]

@grayjay's comment that

I think this is inconsistent with #3357. It added requalifyDeps, which can call qualifyDeps on only a subset of dependencies:

cabal/cabal-install/Distribution/Solver/Modular/Linking.hs

Line 290 in 37325ca

rdeps <- requalify deps

is (as per usual.. 😓 ) an excellent observation and something that needs fixing before this can be merged. Sadly the fix won't be entirely trivial.

[dcoutts]

Should this feature be controlled by a flag, since most test suites don't have cyclic dependencies, and inconsistent dependencies can lead to compile errors?

@grayjay the hope is that this can actually be fully automatic. What kind of cases can lead to compile errors?

Here's an example: suppose optparse-appicative has a testsuite that uses tasty and suppose that this testsuite wants to makes use of tasty's feature to add new command line arguments which itself makes use of optparse-appicative. Does this testsuite really expect that the tasty lib is built with the instance of optparse-appicative that is under test? If it does, then yes it could do that, but I would say that this is an unreasonable expectation.

Or another example might be a testsuite or benchmark for bytestring or binary. If these testsuites/benchmarks in their support code need to call functions from their testing/benchmarking libs (QC, criterion, tasty etc) that themselves use e.g. the ByteString type or the Binary class in their interface then yes that becomes tricky (but not impossible) since they cannot use any function from the bytestring lib directly since the visible one is the one under test.

But again I don't think this is a common problem or a reasonable expectation that the support libs for your testsuite or benchmark code are themselves compiled against the code under test. Indeed I think the reverse expectation is much more reasonable. If I'm testing bytestring or binary or something, then I would much prefer it if criterion/quickcheck/tasty etc were built against existing stable versions of bytestring, binary etc than to try and build them against the code I'm currently testing. Also, this avoids having to rebuild all those support libs every time I make a minor change to the code under test.

Another argument is that while yes in some cases there would be limitations on what you can do in the testsuite/benchmark since it's hard to make use of two versions of the same package (though we may be able to do something about this with package-qualified imports or the new backpack-style module renaming), these are not cases that work at all right now. So we're allowing more.

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

[23Skidoo]

This branch needed rebasing, so I deleted and recreated it (couldn't force-push due to the pre-receive hook complaining). But apparently I can't reopen this PR now, so will open another.

[23Skidoo]

Opened #3422.

@hvr, is there some way to get rid of the pre-receive hook? I think that only master and release branches should be protected (and they already are), for other branches we should allow force-pushing.

[23Skidoo]

Asked support to re-enable force-pushes.

[23Skidoo]

Force-pushes now work again on feature branches.

[hvr]

@23Skidoo for the record; the global force-push-protection predates the "protected branches" feature; in the past you had to ask the github admins to enable it repo-wide. Luckily now there's more granular protected branches user's can control via the web-UI (even though I'm still not satisfied by those, but it's better than nothing :-) )

[23Skidoo]

for the record; the global force-push-protection predates the "protected branches" feature;

Yeah, I figured as much.