Document unused graph traversal functions
Both cabal-install and `Cabal` define a notion of a package index. `Cabal` defines data PackageIndex a = PackageIndex !(Map InstalledPackageId a) !(Map PackageName (Map Version [a])) whereas `cabal-install` defines newtype PackageIndex pkg = PackageIndex (Map PackageName [pkg]) Note that Cabal.PackageIndex is indexed by installed package IDs, whereas CabalInstall.PackageIndex is indexed by package names. There are a bunch of "graph traversal" functions that similarly duplicated between `Cabal` and `cabal-install`; in `Cabal` we have brokenPackages :: PackageInstalled a => PackageIndex a -> [(a, [InstalledPackageId])] dependencyClosure :: PackageInstalled a => PackageIndex a -> [InstalledPackageId] -> Either (PackageIndex a) [(a, [InstalledPackageId])] dependencyCycles :: PackageInstalled a => PackageIndex a -> [[a]] dependencyGraph :: PackageInstalled a => PackageIndex a -> (Graph.Graph, Graph.Vertex -> a, InstalledPackageId -> Maybe Graph.Vertex) dependencyInconsistencies :: PackageInstalled a => PackageIndex a -> [(PackageName, [(PackageId, Version)])] reverseDependencyClosure :: PackageInstalled a => PackageIndex a -> [InstalledPackageId] -> [a] reverseTopologicalOrder :: PackageInstalled a => PackageIndex a -> [a] topologicalOrder :: PackageInstalled a => PackageIndex a -> [a] which are mirrored in `cabal-install` as brokenPackages :: PackageFixedDeps pkg => PackageIndex pkg -> [(pkg, [PackageIdentifier])] dependencyClosure :: PackageFixedDeps pkg => PackageIndex pkg -> [PackageIdentifier] -> Either (PackageIndex pkg) [(pkg, [PackageIdentifier])] dependencyCycles :: PackageFixedDeps pkg => PackageIndex pkg -> [[pkg]] dependencyGraph :: PackageFixedDeps pkg => PackageIndex pkg -> (Graph.Graph, Graph.Vertex -> pkg, PackageIdentifier -> Maybe Graph.Vertex) dependencyInconsistencies :: PackageFixedDeps pkg => PackageIndex pkg -> [(PackageName, [(PackageIdentifier, Version)])] reverseDependencyClosure :: PackageFixedDeps pkg => PackageIndex pkg -> [PackageIdentifier] -> [pkg] reverseTopologicalOrder :: PackageFixedDeps pkg => PackageIndex pkg -> [pkg] topologicalOrder :: PackageFixedDeps pkg => PackageIndex pkg -> [pkg] This by itself makes a certain amount of sense, but here's where the situation gets confusing. `cabal-install` defines a `PlanIndex` as type PlanIndex = Cabal.PackageIndex PlanPackage Note that is using `Cabal`'s notion of a PackageIndex, not `cabal-install`'s; it makes sense that a PlanIndex is indexed by installed package IDs rather than package names (even if currently we have to fake installed package IDs. Almost all of the functions listed above, however, are only called on `PlanIndex`s. This means that we invoke the functions from `Cabal`, not the functions from `cabal-install`; in fact, almost all these functions in `cabal-install` are completely unused right now. In `cabal-install` but calls from `Cabal` ---------------------------------------------------------- closed brokenPackages acyclic dependencyCycles consistent dependencyInconsistencies problems brokenPackages', dependencyCycles', dependencyInconsistencies' This is more than just a code clean-up issue. As mentioned in the previous PR, the fundamental difference between Cabal and cabal-install is their view of dependencies: Cabal knows only about installed libraries and their library dependencies, whereas cabal knows about packages and the dependencies of their setup scripts, executables, test-suites, benchmarks, as well as their library dependencies. By calling the graph-traversal functions from `Cabal` rather than from `cabal-install`, any of these additional dependencies are either completely ignored, or else the distinction is lost (depending on how we implemented installedDepends for plan packages); and neither option is correct. For example, in `new` from Distribution.Client.InstallPlan (in `cabal-install`) we call `dependendyGraph` on the plan index; since the plan index is defined in terms of Cabal's plan index, we call Cabal's `dependencyGraph` here, but that means that this graph will completely lack any setup dependencies. The reverse graph is used in (only one place): `packagedThatDependOn`, which in turn is (only) used in `failed`. But this is wrong: if a package fails to install, if another package depends on it through a setup dependency, then that second package should also be marked as impossible to install. What needs to happen is that we modify the graph traversal functions from `cabal-install` to take a PackageIndex from `Cabal` (so that we can apply them to a PlanIndex), but use the dependencies from `FixedPackageDeps` rather than the flat or incomplete dependencies we get from `PackageInstalled`. In fact, the whole `PackageInstalled` instance for `ConfiguredPackage`, `ReadyPackage` and `PlanPackage` should go: returning only part of the dependencies, or else all dependencies flattened, is just too error prone. This first commit only documents the problem (this commit message) and moves the above functions to a new module called Distribution.Client.PlanIndex. Cleaning this up is complicated by the fact that we _do_ still call two of the above functions on a `CabalInstall.PackageIndex`: * `pruneInstallPlan` from `Distribution.Client.Freeze` calls `dependencyClosure` * The top-down solver calls `dependencyGraph` If we change the above functions to work on a `Cabal.PackageIndex` instead these two exceptions will break, so we need to look at that first.
Showing
- cabal-install/Distribution/Client/Dependency/TopDown.hs 3 additions, 2 deletionscabal-install/Distribution/Client/Dependency/TopDown.hs
- cabal-install/Distribution/Client/Freeze.hs 2 additions, 1 deletioncabal-install/Distribution/Client/Freeze.hs
- cabal-install/Distribution/Client/PackageIndex.hs 4 additions, 187 deletionscabal-install/Distribution/Client/PackageIndex.hs
- cabal-install/Distribution/Client/PlanIndex.hs 205 additions, 0 deletionscabal-install/Distribution/Client/PlanIndex.hs
Loading
Please register or sign in to comment