Commit d239a60f authored by kristenk's avatar kristenk
Browse files

Solver: Remove redundant flag choices from FlaggedDeps.

This commit simplifies the solver's internal representation of package
conditionals by removing branches that are unreachable.  For example, the solver
now simplifies

if flag(A)
  if flag(A)
    build-depends: package-a
  else
    build-depends: package-b

to

if flag(A)
  build-depends: package-a

because A must be true for the second conditional to be relevant to the
finalized PackageDescription.

This change probably won't improve performance by simplifying real packages.
However, it fixes a bug in the solver's handling of a certain edge case.  Before
this change, it was possible for the solver to create DependencyReasons with
conflicting flag values.  The conflicting values could cause a problem when
one was FlagBoth, the value used when the same dependency appears on both sides
of a conditional.  Here is an example:

if flag(A)
  if flag(A)
    build-depends: unknown-package
  else
    build-depends: unknown-package

Previously, the solver inserted both A=FlagTrue and A=FlagBoth into the
DependencyReason for the dependency on unknown-package. When the previous
commit changed the association list of flag values into a map, FlagBoth replaced
FlagTrue and prevented the solver from trying -A.

This commit simplifies the conditionals to:

if flag(A)
  build-depends: unknown-package

It also adds some unit tests for dependencies occurring on both sides of
conditionals. "testBackjumpingWithCommonDependency" tests the example above.
parent 6fe3c5f7
......@@ -172,7 +172,7 @@ convGPD os arch cinfo strfl solveExes pn
conv :: Mon.Monoid a => Component -> (a -> BuildInfo) -> DependencyReason PN ->
CondTree ConfVar [Dependency] a -> FlaggedDeps PN
conv comp getInfo dr =
convCondTree dr pkg os arch cinfo pn fds comp getInfo ipns solveExes .
convCondTree M.empty dr pkg os arch cinfo pn fds comp getInfo ipns solveExes .
PDC.addBuildableCondition getInfo
initDR = DependencyReason pn M.empty S.empty
......@@ -255,19 +255,19 @@ filterIPNs ipns (Dependency pn _) fd
-- | Convert condition trees to flagged dependencies. Mutually
-- recursive with 'convBranch'. See 'convBranch' for an explanation
-- of all arguments preceeding the input 'CondTree'.
convCondTree :: DependencyReason PN -> PackageDescription -> OS -> Arch -> CompilerInfo -> PN -> FlagInfo ->
convCondTree :: Map FlagName Bool -> DependencyReason PN -> PackageDescription -> OS -> Arch -> CompilerInfo -> PN -> FlagInfo ->
Component ->
(a -> BuildInfo) ->
IPNs ->
SolveExecutables ->
CondTree ConfVar [Dependency] a -> FlaggedDeps PN
convCondTree dr pkg os arch cinfo pn fds comp getInfo ipns solveExes@(SolveExecutables solveExes') (CondNode info ds branches) =
convCondTree flags dr pkg os arch cinfo pn fds comp getInfo ipns solveExes@(SolveExecutables solveExes') (CondNode info ds branches) =
concatMap
(\d -> filterIPNs ipns d (D.Simple (convLibDep dr d) comp)) ds -- unconditional package dependencies
++ L.map (\e -> D.Simple (LDep dr (Ext e)) comp) (PD.allExtensions bi) -- unconditional extension dependencies
++ L.map (\l -> D.Simple (LDep dr (Lang l)) comp) (PD.allLanguages bi) -- unconditional language dependencies
++ L.map (\(PkgconfigDependency pkn vr) -> D.Simple (LDep dr (Pkg pkn vr)) comp) (PD.pkgconfigDepends bi) -- unconditional pkg-config dependencies
++ concatMap (convBranch dr pkg os arch cinfo pn fds comp getInfo ipns solveExes) branches
++ concatMap (convBranch flags dr pkg os arch cinfo pn fds comp getInfo ipns solveExes) branches
-- build-tools dependencies
-- NB: Only include these dependencies if SolveExecutables
-- is True. It might be false in the legacy solver
......@@ -292,61 +292,81 @@ convCondTree dr pkg os arch cinfo pn fds comp getInfo ipns solveExes@(SolveExecu
--
-- This function takes a number of arguments:
--
-- 1. Some pre dependency-solving known information ('OS', 'Arch',
-- 1. A map of flag values that have already been chosen. It allows
-- convBranch to avoid creating nested FlaggedDeps that are
-- controlled by the same flag and avoid creating DependencyReasons with
-- conflicting values for the same flag.
--
-- 2. The DependencyReason calculated at this point in the tree of
-- conditionals. The flag values in the DependencyReason are similar to
-- the values in the map above, except for the use of FlagBoth.
--
-- 3. Some pre dependency-solving known information ('OS', 'Arch',
-- 'CompilerInfo') for @os()@, @arch()@ and @impl()@ variables,
--
-- 2. The package name @'PN'@ which this condition tree
-- 4. The package name @'PN'@ which this condition tree
-- came from, so that we can correctly associate @flag()@
-- variables with the correct package name qualifier,
--
-- 3. The flag defaults 'FlagInfo' so that we can populate
-- 5. The flag defaults 'FlagInfo' so that we can populate
-- 'Flagged' dependencies with 'FInfo',
--
-- 4. The name of the component 'Component' so we can record where
-- 6. The name of the component 'Component' so we can record where
-- the fine-grained information about where the component came
-- from (see 'convCondTree'), and
--
-- 5. A selector to extract the 'BuildInfo' from the leaves of
-- 7. A selector to extract the 'BuildInfo' from the leaves of
-- the 'CondTree' (which actually contains the needed
-- dependency information.)
--
-- 6. The set of package names which should be considered internal
-- 8. The set of package names which should be considered internal
-- dependencies, and thus not handled as dependencies.
convBranch :: DependencyReason PN -> PackageDescription -> OS -> Arch -> CompilerInfo ->
PN -> FlagInfo ->
Component ->
(a -> BuildInfo) ->
IPNs ->
SolveExecutables ->
CondBranch ConfVar [Dependency] a ->
FlaggedDeps PN
convBranch dr pkg os arch cinfo pn fds comp getInfo ipns solveExes (CondBranch c' t' mf') =
convBranch :: Map FlagName Bool
-> DependencyReason PN
-> PackageDescription
-> OS
-> Arch
-> CompilerInfo
-> PN
-> FlagInfo
-> Component
-> (a -> BuildInfo)
-> IPNs
-> SolveExecutables
-> CondBranch ConfVar [Dependency] a
-> FlaggedDeps PN
convBranch flags dr pkg os arch cinfo pn fds comp getInfo ipns solveExes (CondBranch c' t' mf') =
go c'
(\dr' -> convCondTree dr' pkg os arch cinfo pn fds comp getInfo ipns solveExes t')
(\dr' -> maybe [] (convCondTree dr' pkg os arch cinfo pn fds comp getInfo ipns solveExes) mf')
dr
(\flags' dr' -> convCondTree flags' dr' pkg os arch cinfo pn fds comp getInfo ipns solveExes t')
(\flags' dr' -> maybe [] (convCondTree flags' dr' pkg os arch cinfo pn fds comp getInfo ipns solveExes) mf')
flags dr
where
go :: Condition ConfVar
-> (DependencyReason PN -> FlaggedDeps PN)
-> (DependencyReason PN -> FlaggedDeps PN)
-> DependencyReason PN -> FlaggedDeps PN
-> (Map FlagName Bool -> DependencyReason PN -> FlaggedDeps PN)
-> (Map FlagName Bool -> DependencyReason PN -> FlaggedDeps PN)
-> Map FlagName Bool -> DependencyReason PN -> FlaggedDeps PN
go (Lit True) t _ = t
go (Lit False) _ f = f
go (CNot c) t f = go c f t
go (CAnd c d) t f = go c (go d t f) f
go (COr c d) t f = go c t (go d t f)
go (Var (Flag fn)) t f = \dr' ->
-- Add each flag to the DependencyReason for all dependencies below,
-- including any extracted dependencies. Extracted dependencies are
-- introduced by both flag values (FlagBoth). Note that we don't
-- actually need to add the flag to the extracted dependencies for
-- correct backjumping; the information only improves log messages by
-- giving the user the full reason for each dependency.
let addFlagVal v = addFlag fn v dr'
in extractCommon (t (addFlagVal FlagBoth))
(f (addFlagVal FlagBoth))
++ [ Flagged (FN pn fn) (fds ! fn) (t (addFlagVal FlagTrue))
(f (addFlagVal FlagFalse)) ]
go (Var (Flag fn)) t f = \flags' ->
case M.lookup fn flags' of
Just True -> t flags'
Just False -> f flags'
Nothing -> \dr' ->
-- Add each flag to the DependencyReason for all dependencies below,
-- including any extracted dependencies. Extracted dependencies are
-- introduced by both flag values (FlagBoth). Note that we don't
-- actually need to add the flag to the extracted dependencies for
-- correct backjumping; the information only improves log messages
-- by giving the user the full reason for each dependency.
let addFlagValue v = addFlagToDependencyReason fn v dr'
addFlag v = M.insert fn v flags'
in extractCommon (t (addFlag True) (addFlagValue FlagBoth))
(f (addFlag False) (addFlagValue FlagBoth))
++ [ Flagged (FN pn fn) (fds ! fn) (t (addFlag True) (addFlagValue FlagTrue))
(f (addFlag False) (addFlagValue FlagFalse)) ]
go (Var (OS os')) t f
| os == os' = t
| otherwise = f
......@@ -364,9 +384,9 @@ convBranch dr pkg os arch cinfo pn fds comp getInfo ipns solveExes (CondBranch c
where
matchImpl (CompilerId cf' cv) = cf == cf' && checkVR cvr cv
addFlag :: FlagName -> FlagValue -> DependencyReason pn -> DependencyReason pn
addFlag fn v (DependencyReason pn' flags stanzas) =
DependencyReason pn' (M.insert fn v flags) stanzas
addFlagToDependencyReason :: FlagName -> FlagValue -> DependencyReason pn -> DependencyReason pn
addFlagToDependencyReason fn v (DependencyReason pn' fs ss) =
DependencyReason pn' (M.insert fn v fs) ss
-- If both branches contain the same package as a simple dep, we lift it to
-- the next higher-level, but with the union of version ranges. This
......
......@@ -51,6 +51,11 @@ tests = [
, runTest $ indep $ mkTest db4 "linkFlags2" ["C", "D"] anySolverFailure
, runTest $ indep $ mkTest db18 "linkFlags3" ["A", "B"] (solverSuccess [("A", 1), ("B", 1), ("C", 1), ("D", 1), ("D", 2), ("F", 1)])
]
, testGroup "Lifting dependencies out of conditionals" [
runTest $ commonDependencyLogMessage "common dependency log message"
, runTest $ twoLevelDeepCommonDependencyLogMessage "two level deep common dependency log message"
, runTest $ testBackjumpingWithCommonDependency "backjumping with common dependency"
]
, testGroup "Manual flags" [
runTest $ mkTest dbManualFlags "Use default value for manual flag" ["pkg"] $
solverSuccess [("pkg", 1), ("true-dep", 1)]
......@@ -911,6 +916,61 @@ db18 = [
, Right $ exAv "G" 1 []
]
-- | When both values for flagA introduce package B, the solver should be able
-- to choose B before choosing a value for flagA. It should try to choose a
-- version for B that is in the union of the version ranges required by +flagA
-- and -flagA.
commonDependencyLogMessage :: String -> SolverTest
commonDependencyLogMessage name =
mkTest db name ["A"] $ solverFailure $ isInfixOf $
"trying: A-1.0.0 (user goal)\n"
++ "next goal: B (dependency of A +/-flagA)\n"
++ "rejecting: B-2.0.0 (conflict: A +/-flagA => B==1.0.0 || ==3.0.0)"
where
db :: ExampleDb
db = [
Right $ exAv "A" 1 [exFlagged "flagA"
[ExFix "B" 1]
[ExFix "B" 3]]
, Right $ exAv "B" 2 []
]
-- | Test lifting dependencies out of multiple levels of conditionals.
twoLevelDeepCommonDependencyLogMessage :: String -> SolverTest
twoLevelDeepCommonDependencyLogMessage name =
mkTest db name ["A"] $ solverFailure $ isInfixOf $
"unknown package: B (dependency of A +/-flagA +/-flagB)"
where
db :: ExampleDb
db = [
Right $ exAv "A" 1 [exFlagged "flagA"
[exFlagged "flagB"
[ExAny "B"]
[ExAny "B"]]
[exFlagged "flagB"
[ExAny "B"]
[ExAny "B"]]]
]
-- | Test handling nested conditionals that are controlled by the same flag.
-- The solver should treat flagA as introducing 'unknown' with value true, not
-- both true and false. That means that when +flagA causes a conflict, the
-- solver should try flipping flagA to false to resolve the conflict, rather
-- than backjumping past flagA.
testBackjumpingWithCommonDependency :: String -> SolverTest
testBackjumpingWithCommonDependency name =
mkTest db name ["A"] $ solverSuccess [("A", 1), ("B", 1)]
where
db :: ExampleDb
db = [
Right $ exAv "A" 1 [exFlagged "flagA"
[exFlagged "flagA"
[ExAny "unknown"]
[ExAny "unknown"]]
[ExAny "B"]]
, Right $ exAv "B" 1 []
]
-- | Tricky test case with independent goals (issue #2842)
--
-- Suppose we are installing D, E, and F as independent goals:
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment