From ba919898ce2552e87725937b640930e0e9d65796 Mon Sep 17 00:00:00 2001 From: Duncan Coutts Date: Mon, 30 May 2016 22:33:51 +0100 Subject: [PATCH 1/2] Fix issue 3428, update the install plan with the right info When building packages we update the install plan with the completed packages and for libraries we include the InstalledPackageInfo. We now support packages that register multiple libraries but only one of them is the representative public library, and only that one is stashed in the install plan. Out of the list of library registraions we select the primary one by looking for the one with the expected unit it. As part of collecting the registration info we do some processing (to account for older Cabal and ghc versions) and the mistake was that while did that post-processing ok and registered the right libraries we ended up returing the un-processed registraion info and so then failed to find the primary lib, and thus failed to stash any library info in the install plan, causing internal errors later on. So this patch fixes it to return the same post-processed registration info as actually gets registered. Subsequent patches improve the internal error checking for this issue. --- cabal-install/Distribution/Client/ProjectBuilding.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cabal-install/Distribution/Client/ProjectBuilding.hs b/cabal-install/Distribution/Client/ProjectBuilding.hs index 1e4e7b04a71..d0b991f65d0 100644 --- a/cabal-install/Distribution/Client/ProjectBuilding.hs +++ b/cabal-install/Distribution/Client/ProjectBuilding.hs @@ -1031,7 +1031,7 @@ buildAndInstallUnpackedPackage verbosity Cabal.registerPackage verbosity compiler progdb HcPkg.MultiInstance (pkgRegisterPackageDBStack pkg) ipkg' - return ipkgs + return ipkgs' else return [] --TODO: [required feature] docs and test phases From 3d1f5ba92e4108e5203a50812fba337963d8f7e9 Mon Sep 17 00:00:00 2001 From: Duncan Coutts Date: Mon, 30 May 2016 22:41:26 +0100 Subject: [PATCH 2/2] Improve internal error checking to avoid issues like #3428 Check that we do get the registration info we expect in a couple places, and add detail to the error message originally reported in #3428. Also build the integration tests with assertions on, which might have caught this error earlier (via the invariant for the install plan). --- .../Distribution/Client/InstallPlan.hs | 7 ++++-- .../Distribution/Client/ProjectBuilding.hs | 22 ++++++++++++++++--- cabal-install/cabal-install.cabal | 2 +- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/cabal-install/Distribution/Client/InstallPlan.hs b/cabal-install/Distribution/Client/InstallPlan.hs index 8df69417888..749357ce86b 100644 --- a/cabal-install/Distribution/Client/InstallPlan.hs +++ b/cabal-install/Distribution/Client/InstallPlan.hs @@ -427,12 +427,15 @@ lookupReadyPackage plan pkg = do Just (Configured _) -> Nothing Just (Processing _) -> Nothing Just (Installed _ (Just ipkg) _) -> Just ipkg - Just (Installed _ Nothing _) -> internalError depOnNonLib + Just (Installed _ Nothing _) -> internalError (depOnNonLib pkgid) Just (Failed _ _) -> internalError depOnFailed Nothing -> internalError incomplete incomplete = "install plan is not closed" depOnFailed = "configured package depends on failed package" - depOnNonLib = "configured package depends on a non-library package" + depOnNonLib dep = "the configured package " + ++ display (packageId pkg) + ++ " depends on a non-library package " + ++ display dep -- | Marks packages in the graph as currently processing (e.g. building). -- diff --git a/cabal-install/Distribution/Client/ProjectBuilding.hs b/cabal-install/Distribution/Client/ProjectBuilding.hs index d0b991f65d0..75cdccc8143 100644 --- a/cabal-install/Distribution/Client/ProjectBuilding.hs +++ b/cabal-install/Distribution/Client/ProjectBuilding.hs @@ -823,8 +823,19 @@ executeInstallPlan verbosity jobCtl plan0 installPkg = -> GenericInstallPlan ipkg srcpkg iresult BuildFailure updatePlan pkg (BuildSuccess ipkgs buildSuccess) = InstallPlan.completed (installedPackageId pkg) - (find (\ipkg -> installedPackageId ipkg == installedPackageId pkg) ipkgs) + mipkg buildSuccess + where + mipkg = case (ipkgs, find (\ipkg -> installedPackageId ipkg + == installedPackageId pkg) ipkgs) of + ([], _) -> Nothing + ((_:_), Just ipkg) -> Just ipkg + ((_:_), Nothing) -> + error $ "executeInstallPlan: package " ++ display (packageId pkg) + ++ " was expected to register the unit " + ++ display (installedPackageId pkg) + ++ " but is actually registering the unit(s) " + ++ intercalate ", " (map (display . installedPackageId) ipkgs) updatePlan pkg (BuildFailure buildFailure) = InstallPlan.failed (installedPackageId pkg) buildFailure depsFailure @@ -1025,8 +1036,13 @@ buildAndInstallUnpackedPackage verbosity -- Case A and B [ipkg] -> [ipkg { Installed.installedUnitId = ipkgid }] -- Case C - _ -> assert (any ((== ipkgid) . Installed.installedUnitId) - ipkgs) ipkgs + _ -> ipkgs + unless (any ((== ipkgid) . Installed.installedUnitId) ipkgs') $ + die $ "the package " ++ display (packageId pkg) ++ " was expected " + ++ " to produce registeration info for the unit Id " + ++ display ipkgid ++ " but it actually produced info for " + ++ intercalate ", " + (map (display . Installed.installedUnitId) ipkgs') forM_ ipkgs' $ \ipkg' -> Cabal.registerPackage verbosity compiler progdb HcPkg.MultiInstance diff --git a/cabal-install/cabal-install.cabal b/cabal-install/cabal-install.cabal index b63f4bade40..280f9fc13b0 100644 --- a/cabal-install/cabal-install.cabal +++ b/cabal-install/cabal-install.cabal @@ -528,7 +528,7 @@ test-suite integration-tests2 type: exitcode-stdio-1.0 main-is: IntegrationTests2.hs hs-source-dirs: tests, . - ghc-options: -Wall -fwarn-tabs + ghc-options: -Wall -fwarn-tabs -fno-ignore-asserts other-modules: build-depends: async,