Correctly link setup dependencies of linked packages

[grayjay]

When the solver links a package, it also merges the LinkGroups of the package's direct dependencies. This commit causes the solver to skip setup dependencies, which should remain independent of the package's other dependencies.

Here is the test's log without the fix. It shows a LinkGroup containing both setup and non-setup goals for package D, which causes the solver to reject D-2 for C's setup script.

targets: A, B
      setupDeps9:                                       constraints:
  stanzas A test (unknown source)
  stanzas B test (unknown source)
  stanzas C test (unknown source)
  stanzas D test (unknown source)
  stanzas D test (unknown source)
preferences:
strategy: PreferLatestForSelected
reorder goals: False
independent goals: True
avoid reinstalls: False
shadow packages: False
strong flags: False
max backjumps: infinite
[__0] trying: 0.A-1.0.0 (user goal)
[__1] trying: 0.C-1.0.0 (dependency of 0.A-1.0.0)
[__2] next goal: 0.D (dependency of 0.C-1.0.0)
[__2] rejecting: 0.D-2.0.0 (conflict: 0.C => 0.D==1.0.0)
[__2] trying: 0.D-1.0.0
[__3] trying: 1.B-1.0.0 (user goal)
[__4] trying: 1.C~>0.C-1.0.0 (dependency of 1.B-1.0.0)
[__5] trying: 1.D~>0.D-1.0.0 (dependency of 1.C-1.0.0)
[__6] next goal: C-setup.1.D (dependency of 1.C-1.0.0)
[__6] rejecting: C-setup.1.D~>1.D-1.0.0, C-setup.1.D~>0.D-1.0.0 (conflict: 1.C => C-setup.1.D==2.0.0)
[__6] rejecting: C-setup.1.D-2.0.0 (dependencies not linked: cannot make C-setup.1.D canonical member of {*0.D-1.0.0,1.D-1.0.0,C-setup.1.D-1.0.0}; conflict set: 1.C, C-setup.1.D)
[__6] rejecting: C-setup.1.D-1.0.0 (multiple instances)
[__5] fail (backjumping, conflict set: 0.D, 1.B, 1.C, C-setup.1.D)
[__4] rejecting: 1.C-1.0.0 (multiple instances)
[__3] fail (backjumping, conflict set: 0.C, 0.D, 1.B, 1.C, C-setup.1.D)
[__0] fail (backjumping, conflict set: 0.A, 0.C, 0.D, 1.B, 1.C, C-setup.1.D)
FAIL

[23Skidoo]

Looks fine to me.

/cc @kosmikus

[ezyang]

cc/ @edsko

[ezyang]

This commit fixes #3248.

[edsko]

I'm not sure I understand. You write in your test case docs:

When A and B are installed as independent goals, their dependencies on C must be linked, due to the single instance restriction. Since C depends on D, 0.D and 1.D must also be linked. However, C's setup dependency on D should remain independent. The solver should be able to choose D-1 for C's library and D-2 for C's setup script.

This all makes perfect sense. However, if we are linking 1.C to 0.C, then surely 1.C's setup dependencies and 0.C's setup dependencies must in fact be linked. After all, linking 1.C to 0.C means that 1.C and 0.C are the very same package; they must be completely identical.

What am I missing?

[edsko]

I mean: it is true that 1.C's direct dependency on D and it's setup dependency on D should not be linked together, nor should 0.C's direct dependency on D and it's setup dependency on D. But 1.C's setup dependency on D should be linked to 0.C's setup dependency on D, and similarly 1.C's direct dependency on D should be linked to 0.C's direct dependency on D.

[grayjay]

Oops! I didn't realize that 0.C-setup.D and 1.C-setup.D should be linked, but that makes sense. So the problem is that linkDeps adds 1.C-setup.D to the wrong link group, the one containing 0.D. My PR probably seemed to work because the setup dependencies were always linked together later, but we need to enforce the linking.

[grayjay]

@edsko I pushed another commit that links setup dependencies to other setup dependencies. I don't know if it's the best approach, since it treats direct setup dependencies as a special case.

I initially tried using only the qualified package name to determine how to link a dependency. For example, if A.B.package1 is linked to A'.B'.package1, and A.B.package1 has a dependency on A.B.C.package2, we can find the package to link with A.B.C.package2 by replacing A.B with A'.B' to yield A'.B'.C.package2. This would also handle indirect dependencies and independent tests, but it doesn't work as well with the new representation of package paths.

[edsko]

Wait... Maybe I'm starting to see what's going on. You are now manually qualifying the setup dependencies with the appropriate qualifier; maybe what you are saying here is that we should use qualityDeps when we call linkDeps? (Though i'm somewhat surprised we don't already, and that if we don't, that this isn't causing other bugs..)

[grayjay]

@edsko Yes, I see that I need to use some of the logic in qualifyDeps. I couldn't figure out how to reuse the function, though. It looks like it takes a QPN and a FlaggedDeps Component PN and qualifies the package names in the flagged deps to give a FlaggedDeps Component QPN. However, linkDeps takes a PP and a FlaggedDeps Component QPN and uses the PP to re-qualify each dependency in order to link the new and old QPNs. The bug is in re-qualifying the dependency, which could be simplified by making use of the existing package path.

I just tried fixing the bug by using qualifyDeps to calculate the new and old package paths at once. I gave qualifyDeps a more general type signature so that it could return a FlaggedDeps Component [QPN] in the case where we need to qualify dependencies in multiple ways. When the solver links two packages, it calls qualifyDeps with both package paths. It then traverses the resulting dependency tree, and each time it encounters a [QPN], it merges the link groups of those dependencies. My idea doesn't handle flags and stanzas, which shouldn't have multiple package paths. I'm not sure how to proceed.

(Though i'm somewhat surprised we don't already, and that if we don't, that this isn't causing other bugs..)

This is only a problem when there is more than one qualifier.

[edsko]

Hmmmm this is kind of subtle actually. Looking at the code, actually the dependencies that get passed as an argument to linkDeps have already been properly qualified using qualifyDeps (ultimately these dependencies are constructed in the call to pickPOption in validateLinking, correct?). So we might have something like 1.C with a setup dependency 1.C-setup.D, and we are linking 1.C to 0.C; the bug arises from the fact that we simply replace the package paths of the dependencies with the linked-to path, ending up linking 1.C-setup.D to 0.D, is that correct? Do I finally understand the bug correctly? 😊

[ezyang]

(Whatever the result is, this better be properly documented somewhere! Write a note!)

[23Skidoo]

(Whatever the result is, this better be properly documented somewhere! Write a note!)

+1.

[grayjay]

@edsko Yes, exactly. My first idea was to remove the prefix of 1.C-setup.D that matches the package path of 1.C, and then prepend the package path of 0.C, so that the result is 0.C-setup.D.

@ezyang What type of note? A comment in the code?

[ezyang]

@grayjay Yep.

[23Skidoo]

@edsko, if you think that this fix is correct, feel free to merge.

[edsko]

@23Skidoo No, the PR as currently pushed here is not completely correct I think. Let me give this some thought right now.

[edsko]

@grayjay What do yo think of https://github.com/edsko/cabal/tree/pr/FixLinkDeps ? Single additional commit on top of your PR. Basically, the idea is that we requalify all dependencies in linkDeps with the appropriate new top-level parent. It passes all unit tests, and I think it makes sense but I would appreciate your feedback.

[grayjay]

@edsko Thanks for working on this PR. linkDeps is called in two places: after a package is linked and after a flag is chosen. I think the linked package case is correct, but I'm not sure about the flag case.

Here is what I think happens after the solver chooses a flag for package-path.x, when it is linked to package-path'.x. The solver has to find the new dependencies of package-path.x (deps) and the new dependencies of package-path'.x (deps') and link them together. It calculates deps by calling qualifyDeps with package-path.x and then calling findNewDeps on the result. However, deps' is calculated by calling findNewDeps before qualifyDeps. I think this is only correct if the order of the two function calls doesn't matter. The order doesn't seem to matter now, but I wonder if that could change if a feature like independent tests causes dependencies under a flag or stanza to be qualified differently from top-level dependencies.

[edsko]

Hmmm, I see where you are coming from. I briefly experimented with having linkDeps take a set of unqualified dependencies (just to make this a little more obvious) but this does not work so well because in findNewDeps needs qualified flag names in order to look up the flag assignment.

But it seems reasonable to have (and document) the following invariant:

Package qualification should not depend on flag assignment

(It may depend on global solver flags of course.) That addresses your concern, doesn't it? And if we state it like that, it seems like a very reasonable thing to insist on. It's complicated enough as it is; if we have local differences in qualification, I foresee no end of headaches.

What do you think?

[grayjay]

@edsko I think that invariant makes sense. Do you want to open a new PR?

[grayjay]

https://github.com/edsko/cabal/tree/pr/FixLinkDeps fixed the issue and was included in #3357. The test from this PR was also included (2f4c440).