Allow the solver to toggle manual flags to match constraints that have any qualifier. #4342
Conversation
…e any qualifier. This fixes haskell#4299. The change gives the dependency solver the flexibility to link dependencies when the user has only set a manual flag on one of them. Previously, the solver would force the constrained dependency to have the flag value from the constraint and force the unconstrained dependency to have the default flag value. In cases where the single instance restriction required the dependencies to be linked, the solver couldn't find a solution. Qualified constraints can still be used to force different dependencies on a package to use different flag values. For example, "--constraint 'pkg +flag' --constraint 'pkg2:setup.pkg -flag'" turns the flag on for the top-level dependency and off for the setup dependency. I also stored flag default values in the search tree to simplify the code.
Tests can now declare flags before using them, in order to specify non-default values for the fields in 'D.Types.GenericPackageDescription.Flag'.
There was a problem hiding this comment.
I read through the key diff until it made sense. I will admit the approach taken is a little indirect; for example, if I have three instances of a flag, and one is explicitly set to True (via a constraint) and the other is explicitly set to False (ditto), then the third instance will be toggled by the solver however it wants (since there is "a constraint" for true and "a constraint" for false). In fact, IIUC, it can toggle the flag even if we don't end up linking?
OTOH, I don't really know you would directly say, "If you link, toggle the manual flag this way", so this might be OK for now.
| checkChild qpn x@(GoalChoice rdm _) = failIfCycle qpn rdm x | ||
| checkChild _ x@(Fail _ _) = x | ||
| checkChild qpn x@(Done rdm _) = failIfCycle qpn rdm x | ||
| checkChild qpn x@(PChoice _ rdm _ _) = failIfCycle qpn rdm x |
There was a problem hiding this comment.
Not something you have to do, but I wonder if we shouldn't have selectors here :)
There was a problem hiding this comment.
What type of selector? A function that returns a Maybe?
| -- | ||
| -- This function does not enforce any of the constraints, since that is done by | ||
| -- 'enforcePackageConstraints'. | ||
| enforceManualFlags :: M.Map PN [LabeledPackageConstraint] -> Tree d c -> Tree d c |
|
Sure, I can cherry-pick this into 2.0. |
| @@ -166,16 +169,23 @@ data ExampleDependency = | |||
| | ExPkg (ExamplePkgName, ExamplePkgVersion) | |||
| deriving Show | |||
|
|
|||
| data ExFlag = ExFlag { | |||
There was a problem hiding this comment.
A Haddock comment here would be nice.
There was a problem hiding this comment.
I made a PR: #4345
|
Also cherry-picked into 2.0. |
Yes, the third instance can take either value, even if it isn't linked. I thought that would be more consistent with how the solver behaves in other situations. It usually treats linking as just an optimization: any choices that are allowed when packages are linked are also allowed when the packages aren't linked. (The one exception is the single instance restriction, but I think we can remove that now.) I also wanted to avoid adding a dependency between manual flag choices and linking. In general, dependencies between different choices made by the solver lead to larger conflict sets and longer run times. If we need to control manual flag values more precisely, we could add it later. |
|
Aha! I guess I misunderstood how linking works; I thought it was semantically meaningful, probably due to its use for handling the single instance restriction. So then, would it be accurate to say that if we got rid of the single instance restriction, this patch wouldn't be necessary either? |
|
Yes, that was one of my suggestions in #4299. Maybe we can remove the single instance restriction on master and then revert this behavior change. I'm not sure how hard that would be. A few sections of code assume that there is only one instance of each package, and we might need to change how the solver decides when to build the inplace copies of packages. |
|
Oops, I missed that! Maybe it is worth a comment saying that this change can be reverted if single instance is dropped. If the semantics here cause us trouble in the future, we'll know what to change! |
|
Good idea. I made a PR: #4354 |
Three commits:
Allow the solver to toggle manual flags to match constraints that have any qualifier.
This fixes #4299. The change gives the dependency solver the flexibility to link
dependencies when the user has only set a manual flag on one of them.
Previously, the solver would force the constrained dependency to have the
flag value from the constraint and force the unconstrained dependency to have
the default flag value. In cases where the single instance restriction required
the dependencies to be linked, the solver couldn't find a solution.
Qualified constraints can still be used to force different dependencies on a
package to use different flag values. For example,
--constraint 'pkg +flag' --constraint 'pkg2:setup.pkg -flag'turns the flag onfor the top-level dependency and off for the setup dependency.
I also stored flag default values in the search tree to simplify the code.
Add manual flags to the solver DSL.
Tests can now declare flags before using them, in order to specify non-default
values for the fields in
D.Types.GenericPackageDescription.Flag.Add tests for manual flags.
@23Skidoo Do you think we could add this to the release branch? I wanted to fix #4299 because it's a regression.
@ezyang What do you think of this solution?
/cc @kosmikus