Use standard graph construction in PlanIndex #2504
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It turns out not to be the right solution for general private dependencies and is just complicated. However we keep qualified goals, just much simpler. Now dependencies simply inherit the qualification of their parent goal. This gets us closer to the intended behaviour for the --independent-goals feature, and for the simpler case of private dependencies for setup scripts. When not using --independent-goals, the solver behaves exactly as before (tested by comparing solver logs for a hard hackage goal). When using --independent-goals, now every dep of each independent goal is qualified, so the dependencies are solved completely independently (which is actually too much still).
POption annotates a package choice with a "linked to" field. This commit just introduces the datatype and deals with the immediate fallout, it doesn't actually use the field for anything.
This is implemented as a separate pass so that it can be understood independently of the rest of the solver.
In particular, in the definition of dependencyInconsistencies. One slightly annoying thing is that in order to validate an install plan, we need to know if the goals are to be considered independent. This means we need to pass an additional Bool to a few functions; to limit the number of functions where this is necessary, also recorded whether or not goals are independent as part of the InstallPlan itself.
Since we didn't really have a unit test setup for the solver yet, this introduces some basic tests for solver, as well as tests for independent goals specifically.
This address @23Skidoo's comment #2500 (comment)
I don't know why we we constructed this graph manually here rather than calling `graphFromEdges`; it doesn't really matter except that we will want to change the structure of this graph somewhat once we have more fine-grained dependencies, and then the manual construction becomes a bit more painful; easier to use the standard construction.
|
OK, approve of using the library function. Haven't looked at the patches yet. |
|
It looks like I got confused by the PR. If you just want to submit one patch it would be better to interactively rebase it to the top |
|
Sorry for the confusion @ezyang , I did try to make that clear in the description. That doesn't mean that reviewing the other patches wasn't useful, they make up #2500 :) As for why the solved goals plan doesn't work, I have no idea -- that patch is @dcoutts 's, and it was my starting point for implementing linking. FWIW, the design of this is all @dcoutts ' and @kosmikus '. |
This was referenced Apr 1, 2015
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the definition of
dependencyGraphwe constructed the graph manually, rather than using the standard library funcitongraphFromEdges. I don't know why we did this, perhaps it predates that function; but it makes the function a bit hard to modify. In particular, once we start introducing fine-grained dependencies (next PR), we will want an edge-labeled graph and having manual graph construction here makes that awkward to do.This commit lives in a branch forked from #2500, so it looks like a big PR, but actually it's just one commit ac47cbc on top of that branch, which is really quite independent of the changes in that branch; this commit can be reviewed completely in isolation. Submitting as a separate PR so that perhaps @ezyang and/or @23Skidoo can take a quick look at this before I start to take advantage of this new definition in the next PR.
As a sanity check, I ran cabal-install 1.22.0.0 (built with Cabal 1.22.0.0) against a sizeable portion of Hackage (the latest versions of all packages alphebatically up to NXT-DST), and compared against HEAD with this patch in, and found that the install plans were identical in both cases (with similar performance).
Also /cc @kosmikus .
(PS. We could also make the corresponding change in Cabal; I haven't done that yet but we could. It's less important because that graph will not change.)