From b2be87e14857eef70598e56b1653bc8d023a7960 Mon Sep 17 00:00:00 2001 From: "Edward Z. Yang" Date: Thu, 31 Mar 2016 00:28:27 -0700 Subject: [PATCH] Fix build-tools ordering regression (#3257, #1541) When converting the component graph to operate in terms of UnitIds instead of CNames I accidentally introduced a regression where we stopped respecting build-tools when determining an ordering to build things. This commit fixes the regression (though perhaps not in the most clean/performant way you could manage it.) It also fixes a latent bug if internal libraries aren't processed in the correct order. Signed-off-by: Edward Z. Yang --- Cabal/Distribution/Simple/Configure.hs | 23 +++++++++--- .../tests/PackageTests/CustomPreProcess/A.pre | 4 +++ .../PackageTests/CustomPreProcess/Hello.hs | 6 ++++ .../CustomPreProcess/MyCustomPreprocessor.hs | 9 +++++ .../PackageTests/CustomPreProcess/Setup.hs | 36 +++++++++++++++++++ .../internal-preprocessor-test.cabal | 31 ++++++++++++++++ Cabal/tests/PackageTests/PackageTester.hs | 3 +- Cabal/tests/PackageTests/Tests.hs | 6 ++++ 8 files changed, 111 insertions(+), 7 deletions(-) create mode 100644 Cabal/tests/PackageTests/CustomPreProcess/A.pre create mode 100644 Cabal/tests/PackageTests/CustomPreProcess/Hello.hs create mode 100644 Cabal/tests/PackageTests/CustomPreProcess/MyCustomPreprocessor.hs create mode 100644 Cabal/tests/PackageTests/CustomPreProcess/Setup.hs create mode 100644 Cabal/tests/PackageTests/CustomPreProcess/internal-preprocessor-test.cabal diff --git a/Cabal/Distribution/Simple/Configure.hs b/Cabal/Distribution/Simple/Configure.hs index a0492aaa457..dc429993673 100644 --- a/Cabal/Distribution/Simple/Configure.hs +++ b/Cabal/Distribution/Simple/Configure.hs @@ -127,6 +127,8 @@ import Text.PrettyPrint import Distribution.Compat.Environment ( lookupEnv ) import Distribution.Compat.Exception ( catchExit, catchIO ) +import Data.Graph (graphFromEdges, topSort) + -- | The errors that can be thrown when reading the @setup-config@ file. data ConfigStateFileError = ConfigStateFileNoHeader -- ^ No header found. @@ -1393,7 +1395,7 @@ mkComponentsGraph pkg_descr internalPkgDeps = | c <- pkgEnabledComponents pkg_descr ] in case checkComponentsCyclic graph of Just ccycle -> Left [ cname | (_,cname,_) <- ccycle ] - Nothing -> Right [ (c, cdeps) | (c, _, cdeps) <- graph ] + Nothing -> Right [ (c, cdeps) | (c, _, cdeps) <- topSortFromEdges graph ] where -- The dependencies for the given component componentDeps component = @@ -1577,6 +1579,12 @@ computeCompatPackageKey comp pkg_name pkg_version (SimpleUnitId (ComponentId str | otherwise = str +topSortFromEdges :: Ord key => [(node, key, [key])] + -> [(node, key, [key])] +topSortFromEdges es = + let (graph, vertexToNode, _) = graphFromEdges es + in reverse (map vertexToNode (topSort graph)) + mkComponentsLocalBuildInfo :: ConfigFlags -> Compiler -> InstalledPackageIndex @@ -1592,11 +1600,16 @@ mkComponentsLocalBuildInfo cfg comp installedPackages pkg_descr graph flagAssignment = foldM (wrap componentLocalBuildInfo) [] graph where - wrap f z (component, _) = do + wrap f z (component, dep_cnames) = do clbi <- f z component - -- TODO: Maybe just store the internal deps in the clbi? - let dep_uids = map fst (filter (\(_,e) -> e `elem` internalPkgDeps) - (componentPackageDeps clbi)) + -- NB: We want to preserve cdeps because it contains extra + -- information like build-tools ordering + let dep_uids = [ componentUnitId dep_clbi + | cname <- dep_cnames + -- Being in z relies on topsort! + , (dep_clbi, _) <- z + , componentLocalName dep_clbi == cname ] + print (componentLocalName clbi, dep_cnames, dep_uids) return ((clbi, dep_uids):z) -- The allPkgDeps contains all the package deps for the whole package diff --git a/Cabal/tests/PackageTests/CustomPreProcess/A.pre b/Cabal/tests/PackageTests/CustomPreProcess/A.pre new file mode 100644 index 00000000000..f35c5e15530 --- /dev/null +++ b/Cabal/tests/PackageTests/CustomPreProcess/A.pre @@ -0,0 +1,4 @@ +module A where + +a :: String +a = "hello from A" diff --git a/Cabal/tests/PackageTests/CustomPreProcess/Hello.hs b/Cabal/tests/PackageTests/CustomPreProcess/Hello.hs new file mode 100644 index 00000000000..89a5e5a026d --- /dev/null +++ b/Cabal/tests/PackageTests/CustomPreProcess/Hello.hs @@ -0,0 +1,6 @@ +module Main where + +import A + +main :: IO () +main = putStrLn a diff --git a/Cabal/tests/PackageTests/CustomPreProcess/MyCustomPreprocessor.hs b/Cabal/tests/PackageTests/CustomPreProcess/MyCustomPreprocessor.hs new file mode 100644 index 00000000000..07a4ef33900 --- /dev/null +++ b/Cabal/tests/PackageTests/CustomPreProcess/MyCustomPreprocessor.hs @@ -0,0 +1,9 @@ +module Main where + +import System.Directory +import System.Environment + +main :: IO () +main = do + (source:target:_) <- getArgs + copyFile source target diff --git a/Cabal/tests/PackageTests/CustomPreProcess/Setup.hs b/Cabal/tests/PackageTests/CustomPreProcess/Setup.hs new file mode 100644 index 00000000000..18252a186d9 --- /dev/null +++ b/Cabal/tests/PackageTests/CustomPreProcess/Setup.hs @@ -0,0 +1,36 @@ +{-# OPTIONS_GHC -Wall #-} + +import Distribution.PackageDescription +import Distribution.Simple +import Distribution.Simple.LocalBuildInfo +import Distribution.Simple.PreProcess +import Distribution.Simple.Utils +import System.Exit +import System.FilePath +import System.Process (rawSystem) + +main :: IO () +main = defaultMainWithHooks + simpleUserHooks { hookedPreProcessors = [("pre", myCustomPreprocessor)] } + where + myCustomPreprocessor :: BuildInfo -> LocalBuildInfo -> ComponentLocalBuildInfo -> PreProcessor + myCustomPreprocessor _bi lbi _clbi = + PreProcessor { + platformIndependent = True, + runPreProcessor = mkSimplePreProcessor $ \inFile outFile verbosity -> + do info verbosity ("Preprocessing " ++ inFile ++ " to " ++ outFile) + callProcess progPath [inFile, outFile] + } + where + builddir = buildDir lbi + progName = "my-custom-preprocessor" + progPath = builddir progName progName + + -- Backwards compat with process < 1.2. + callProcess :: FilePath -> [String] -> IO () + callProcess path args = + do exitCode <- rawSystem path args + case exitCode of ExitSuccess -> return () + f@(ExitFailure _) -> fail $ "callProcess " ++ show path + ++ " " ++ show args ++ " failed: " + ++ show f diff --git a/Cabal/tests/PackageTests/CustomPreProcess/internal-preprocessor-test.cabal b/Cabal/tests/PackageTests/CustomPreProcess/internal-preprocessor-test.cabal new file mode 100644 index 00000000000..26735e39fa5 --- /dev/null +++ b/Cabal/tests/PackageTests/CustomPreProcess/internal-preprocessor-test.cabal @@ -0,0 +1,31 @@ +name: internal-preprocessor-test +version: 0.1.0.0 +synopsis: Internal custom preprocessor example. +description: See https://github.com/haskell/cabal/issues/1541#issuecomment-30155513 +license: GPL-3 +author: Mikhail Glushenkov +maintainer: mikhail.glushenkov@gmail.com +category: Testing +build-type: Custom +cabal-version: >=1.10 + +-- Note that exe comes before the library. +-- The reason is backwards compat: old versions of Cabal (< 1.18) +-- don't have a proper component build graph, so components are +-- built in declaration order. +executable my-custom-preprocessor + main-is: MyCustomPreprocessor.hs + build-depends: base, directory + default-language: Haskell2010 + +library + exposed-modules: A + build-depends: base + build-tools: my-custom-preprocessor + -- ^ Note the internal dependency. + default-language: Haskell2010 + +executable hello-world + main-is: Hello.hs + build-depends: base, internal-preprocessor-test + default-language: Haskell2010 diff --git a/Cabal/tests/PackageTests/PackageTester.hs b/Cabal/tests/PackageTests/PackageTester.hs index 395b8a2eb5a..c4e63086cb7 100644 --- a/Cabal/tests/PackageTests/PackageTester.hs +++ b/Cabal/tests/PackageTests/PackageTester.hs @@ -453,8 +453,7 @@ rawCompileSetup verbosity suite e path = do r <- rawRun verbosity (Just path) (ghcPath suite) e $ [ "--make"] ++ ghcPackageDBParams (ghcVersion suite) (packageDBStack suite) ++ - [ "-hide-all-packages" - , "-package base" + [ "-hide-package Cabal" #ifdef LOCAL_COMPONENT_ID -- This is best, but we don't necessarily have it -- if we're bootstrapping with old Cabal. diff --git a/Cabal/tests/PackageTests/Tests.hs b/Cabal/tests/PackageTests/Tests.hs index a511912bfc5..f0e79e0270f 100644 --- a/Cabal/tests/PackageTests/Tests.hs +++ b/Cabal/tests/PackageTests/Tests.hs @@ -338,6 +338,12 @@ tests config = do cabal "build" ["myprog"] cabal "copy" ["myprog"] + -- Test internal custom preprocessor + tc "CustomPreProcess" $ do + cabal_build [] + runExe' "hello-world" [] + >>= assertOutputContains "hello from A" + where ghc_pkg_guess bin_name = do cwd <- packageDir