Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow "cycles" through test suite dependencies (VERSION TWO) #3290

Closed
wants to merge 8 commits into from

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Apr 6, 2016

This is a version of #3232 updated for HEAD.

CC @edsko

edsko and others added 7 commits April 6, 2016 11:37
Of course, this test currently fails.
When a test suite has a dependency which is not shared with the main library,
we can consider it independent. This addresses haskell#1575 to some degree. Suppose

* optparse-applicative has a test suite that depends on tasty
* tasty has a (regular) dependency on optparse-applicative

We can resolve this by linking optparse-applicative's test suite against an
older version of itself.

This only works provided that optparse-applicative's test suite does not
declare optparse-applicative as a dependency (and instead just compiles in
the modules from the src/ directory or whatever). If the test suite did
declare the library as a dependency, then clearly the test suite needs to be
built against _this_ version of the library; it would be terribly confusing
if the test suite got built against an older version. But if the test suite
gets built against the library itself, then if the test suite also needs
tasty, we cannot pick two different versions in the same application (yet).

In this commit we add the appropriate qualifiers; however, the resulting
install plan will now be rejected by the internal validity check. That's next
up.
For test suites etc. we might want to allow for inconsistent dependencies. For
instance, in the optparse-applicative -> tasty -> optparse-applicative
dependency cycle we might want to allow two versions of optparse-applicative in
the same executable (one that is the library-under-test and one used internally
by tasty).
Details of the test in the README.
Signed-off-by: Edward Z. Yang <[email protected]>
@23Skidoo
Copy link
Member

23Skidoo commented Apr 6, 2016

If the tests pass, can you please squash fix-up commits with the original ones? Would be nice to avoid having broken commits in history.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 6, 2016

There's still a bug in this PR though, cabal install in the sandbox is not idempotent. In the integration test, if I run it the second time, here's what I get:

      In order, the following will be installed:
      A-2 *test (via: T-1) (reinstall) (changes: T-1 added)
      T-1 (via: A-2) (reinstall) (changes: A-1 -> 2)

UPDATE: and then the third time I run it, the dependency solver fails altogether:

      cabal: Could not resolve dependencies:
      next goal: A (user goal)
      rejecting: A-1/installed-3Pv..., A-1 (constraint from user target requires
      ==2)
      trying: A-2
      trying: A~>A-2 (user goal)
      trying: A~>A-2 (user goal)
      rejecting: A-2:!test (constraint from config file, command line flag, or user
      target requires opposite flag selection)
      trying: A-2:*test
      trying: A-test.T~>T-1 (dependency of A-2:*test)
      trying: A-test.A~>A-2 (dependency of A-test.T-1)
      rejecting: A-test.A-2:!test (constraint from config file, command line flag,
      or user target requires opposite flag selection)
      trying: A-test.A-2:*test
      fail (cyclic dependencies; conflict set: A-test.A, A-test.T, A-test.A-2:test)
      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.

@23Skidoo Of course.

@grayjay
Copy link
Collaborator

grayjay commented Apr 6, 2016

The log from the third run looks like #3203.

EDIT: I think there are multiple A targets, but I don't know if that explains the failure:

  trying: A-2
  trying: A~>A-2 (user goal)
  trying: A~>A-2 (user goal)

@ezyang
Copy link
Contributor Author

ezyang commented Apr 6, 2016

(I guess, at some point, we should move this to a branch on origin so @edsko can push to it.)

Signed-off-by: Edward Z. Yang <[email protected]>
@23Skidoo
Copy link
Member

So I think I won't delay the first 1.24 release for this. If @kosmikus thinks that this PR should go into 1.24, I think it can be made a part of a point release. Which means that 1.24 is now good to go.

@kosmikus
Copy link
Contributor

Independently of the first 1.24 release, I'd like to understand better what is going on here. There's quite a bit of history, and perhaps someone who has been following that more closely than I have can provide a good summary of what the status here is? @ezyang ?

@ezyang
Copy link
Contributor Author

ezyang commented Apr 26, 2016

I don't really know how the patch internally works, but I know that the obvious test for the feature (cabal installing a package with a cyclic dependency in a sandbox) doesn't work properly. So someone needs to dig in and figure out why it's failing and then fix it.

@23Skidoo
Copy link
Member

23Skidoo commented Apr 26, 2016

the obvious test for the feature (cabal installing a package with a cyclic dependency in a sandbox) doesn't work properly. So someone needs to dig in and figure out why it's failing and then fix it.

@edsko, can you please look into it?

@edsko
Copy link
Contributor

edsko commented May 2, 2016

@23Skidoo the solver part of this is working, as far as I can tell. I'm not sure what's going on with the sandbox, some additional check somewhere I think. I don't know when I'll get a chance to look at that; perhaps somebody who is more familiar with the sandboxing stuff would be able to identify the problem more quickly?

@23Skidoo
Copy link
Member

23Skidoo commented May 3, 2016

I don't know when I'll get a chance to look at that; perhaps somebody who is more familiar with the sandboxing stuff would be able to identify the problem more quickly?

I'll try to take a look at this next month.

@hvr
Copy link
Member

hvr commented May 5, 2016

@ezyang I wanted to to try this PR to see whether it fixes the issues I have with a couple of packages (time, bytestring, binary, cassava, ...) but sadly this PR has bitrotted and needs to be rebased with the merge-conflicts resolved...

@23Skidoo
Copy link
Member

23Skidoo commented May 5, 2016

Closing in favour of #3402.

@23Skidoo 23Skidoo closed this May 5, 2016
@23Skidoo
Copy link
Member

23Skidoo commented May 5, 2016

@hvr See #3402 for an updated version.

@hvr
Copy link
Member

hvr commented May 6, 2016

@23Skidoo thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants