ConfiguredCabal -> PackageData, more comments, more feedback addressed

hadrian.cabal

@@ -36,7 +36,7 @@ executable hadrian
                        , Hadrian.Builder.Tar
                        , Hadrian.Expression
                        , Hadrian.Haskell.Cabal
-                       , Hadrian.Haskell.Cabal.Configured
+                       , Hadrian.Haskell.Cabal.PackageData
                        , Hadrian.Haskell.Cabal.Parse
                        , Hadrian.Haskell.Cabal.Type
                        , Hadrian.Oracles.ArgsHash

src/Builder.hs

@@ -4,8 +4,6 @@ module Builder (
     ArMode (..), CcMode (..), GhcCabalMode (..), GhcMode (..), GhcPkgMode (..), HaddockMode (..),
     SphinxMode (..), TarMode (..), Builder (..),
 
-    builderPath',
-
     -- * Builder properties
     builderProvenance, systemBuilderPath, builderPath, isSpecified, needBuilder,
     runBuilder, runBuilderWith, runBuilderWithCmdOptions, getBuilderPath,
@@ -55,7 +53,7 @@ instance Binary   GhcMode
 instance Hashable GhcMode
 instance NFData   GhcMode
 
--- | GHC cabal mode. Can configure, copy and register pacakges.
+-- | GHC cabal mode. Can configure, copy and register packages.
 data GhcCabalMode = Conf | HsColour | Check | Sdist
     deriving (Eq, Generic, Show)
 
@@ -167,9 +165,6 @@ builderProvenance = \case
   where
     context s p = Just $ vanillaContext s p
 
-builderPath' :: Builder -> Action FilePath
-builderPath' = builderPath
-
 instance H.Builder Builder where
     builderPath :: Builder -> Action FilePath
     builderPath builder = case builderProvenance builder of
@@ -199,7 +194,7 @@ instance H.Builder Builder where
     -- query the builder for some information.
     -- contrast this with runBuilderWith, which returns @Action ()@
     -- this returns the @stdout@ from running the builder.
-    -- For now this only implements asking @ghc-pkg@ about pacakge
+    -- For now this only implements asking @ghc-pkg@ about package
     -- dependencies.
     askBuilderWith :: Builder -> BuildInfo -> Action String
     askBuilderWith builder BuildInfo {..} = case builder of
@@ -262,8 +257,12 @@ instance H.Builder Builder where
                     unit $ cmd [Cwd output] [path]        buildArgs
 
                 GhcPkg Clone _ -> do
-                    -- input is "virtual" here. it's essentially a package name
-                    Stdout pkgDesc <- cmd [path] ["--expand-pkgroot", "--no-user-package-db", "describe", input ]
+                    Stdout pkgDesc <- cmd [path]
+                      [ "--expand-pkgroot"
+                      , "--no-user-package-db"
+                      , "describe"
+                      , input -- the package name
+                      ]
                     cmd (Stdin pkgDesc) [path] (buildArgs ++ ["-"])
 
                 _  -> cmd echo [path] buildArgs

src/Builder.hs-boot

@@ -4,7 +4,6 @@ import Stage
 import Hadrian.Builder.Ar
 import Hadrian.Builder.Sphinx
 import Hadrian.Builder.Tar
-import Development.Shake
 
 data CcMode = CompileC | FindCDependencies
 data GhcMode =  CompileHs | CompileCWithGhc | FindHsDependencies | LinkHs
@@ -45,5 +44,3 @@ data Builder = Alex
 
 instance Eq Builder
 instance Show Builder
-
-builderPath' :: Builder -> Action FilePath

src/Context/Paths.hs

@@ -32,7 +32,7 @@ buildDir context = contextDir context -/- "build"
 
 -- | Path to the directory containing build artifacts of a given 'Context'.
 buildPath :: Context -> Action FilePath
-buildPath context = buildRoot <&> (-/- (buildDir context))
+buildPath context = buildRoot <&> (-/- buildDir context)
 
 -- | Get the build path of the current 'Context'.
 getBuildPath :: Expr Context b FilePath

src/Expression.hs

@@ -14,7 +14,7 @@ module Expression (
 
     -- * Convenient accessors
     getBuildRoot, getContext, getOutputs, getInputs,
-    getInput, getOutput, getConfiguredCabalData,
+    getInput, getOutput, getPackageData,
 
     -- * Re-exports
     module Base,
@@ -27,14 +27,14 @@ import {-# SOURCE #-} Builder
 import Context hiding (stage, package, way)
 import Expression.Type
 import Hadrian.Expression hiding (Expr, Predicate, Args)
-import Hadrian.Haskell.Cabal.Configured (ConfiguredCabal)
-import Hadrian.Oracles.TextFile (readConfiguredCabalFile)
+import Hadrian.Haskell.Cabal.PackageData (PackageData)
+import Hadrian.Oracles.TextFile (readPackageDataFile)
 
 -- | Get values from a configured cabal stage.
-getConfiguredCabalData :: (ConfiguredCabal -> a) -> Expr a
-getConfiguredCabalData key = do
+getPackageData :: (PackageData -> a) -> Expr a
+getPackageData key = do
   ctx   <- getContext
-  Just cabal <- expr (readConfiguredCabalFile ctx)
+  Just cabal <- expr (readPackageDataFile ctx)
   return $ key cabal
 
 -- | Is the build currently in the provided stage?

src/Hadrian/Haskell/Cabal.hs

@@ -13,11 +13,12 @@ module Hadrian.Haskell.Cabal (
     pkgVersion, pkgIdentifier, pkgDependencies, pkgSynopsis
     ) where
 
+import Data.Maybe
 import Development.Shake
 
 import Context.Type
-import Hadrian.Haskell.Cabal.Type       as C
-import Hadrian.Haskell.Cabal.Configured as CC
+import Hadrian.Haskell.Cabal.Type        as C
+import Hadrian.Haskell.Cabal.PackageData as PD
 import Hadrian.Package
 import Hadrian.Oracles.TextFile
 
@@ -29,7 +30,7 @@ pkgVersion = fmap (fmap C.version) . readCabalFile
 -- The Cabal file is tracked.
 pkgIdentifier :: Context -> Action String
 pkgIdentifier ctx = do
-    Just cabal <- readCabalFile ctx
+    cabal <- fromMaybe (error "Cabal file could not be read") <$> readCabalFile ctx
     return $ if null (C.version cabal)
         then C.name cabal
         else C.name cabal ++ "-" ++ C.version cabal
@@ -39,7 +40,7 @@ pkgIdentifier ctx = do
 -- returns a crude overapproximation of actual dependencies. The Cabal file is
 -- tracked.
 pkgDependencies :: Context -> Action (Maybe [PackageName])
-pkgDependencies = fmap (fmap CC.dependencies) . readConfiguredCabalFile
+pkgDependencies = fmap (fmap PD.dependencies) . readPackageDataFile
 
 -- | Read a Cabal file and return the package synopsis. The Cabal file is tracked.
 pkgSynopsis :: Context -> Action (Maybe String)

src/Hadrian/Haskell/Cabal/Configured.hs → src/Hadrian/Haskell/Cabal/PackageData.hs

@@ -1,14 +1,13 @@
-module Hadrian.Haskell.Cabal.Configured where
+module Hadrian.Haskell.Cabal.PackageData where
 
 import Development.Shake.Classes
 import Hadrian.Package.Type
 import GHC.Generics
 
-data ConfiguredCabal = ConfiguredCabal
+data PackageData = PackageData
     { dependencies :: [PackageName]
     , name         :: PackageName
     , version      :: String
-    -- , packageDesc  :: C.PackageDescription
     -- * used to be pkg Data
     , componentId  :: String
     , modules      :: [String]
@@ -22,7 +21,7 @@ data ConfiguredCabal = ConfiguredCabal
     , depCompIds   :: [String]
     , includeDirs  :: [String]
     , includes     :: [String]
-    , installIncludes :: [String] -- TODO: do we need this one?
+    , installIncludes :: [String]
     , extraLibs    :: [String]
     , extraLibDirs :: [String]
     , asmSrcs      :: [String]
@@ -41,7 +40,7 @@ data ConfiguredCabal = ConfiguredCabal
     , buildGhciLib :: Bool
     } deriving (Eq, Read, Show, Typeable, Generic)
 
-instance Binary ConfiguredCabal
+instance Binary PackageData
 
-instance Hashable ConfiguredCabal
-instance NFData ConfiguredCabal
+instance Hashable PackageData
+instance NFData PackageData

src/Hadrian/Haskell/Cabal/Parse.hs

@@ -9,7 +9,7 @@
 -- Extracting Haskell package metadata stored in Cabal files.
 -----------------------------------------------------------------------------
 module Hadrian.Haskell.Cabal.Parse
-  ( ConfiguredCabal (..), parseCabal, parseConfiguredCabal
+  ( PackageData (..), parseCabal, parsePackageData
   , parseCabalPkgId
   , configurePackage, copyPackage, registerPackage
   ) where
@@ -42,17 +42,19 @@ import qualified Distribution.Verbosity                 as C
 
 import Base
 import Builder hiding (Builder)
-import Context -- .Type
+import Context
 import Flavour (args)
 import GHC.Packages (rts)
 import Hadrian.Expression
+import Hadrian.Haskell.Cabal.PackageData
 import Hadrian.Haskell.Cabal.Type ( Cabal( Cabal ) )
-import Hadrian.Haskell.Cabal.Configured
 import Hadrian.Oracles.TextFile
 import Hadrian.Target
 import Settings
 import Oracles.Setting
 
+-- | Parse the Cabal package identifier from the .cabal file at the given
+--   filepath.
 parseCabalPkgId :: FilePath -> IO String
 parseCabalPkgId file = C.display . C.package . C.packageDescription <$> C.readGenericPackageDescription C.silent file
 
@@ -75,6 +77,12 @@ biModules pd = go [ comp | comp@(bi,_) <- (map libBiModules . maybeToList $ C.li
         go _  = error "can not handle more than one buildinfo yet!"
         isHaskell fp = takeExtension fp `elem` [".hs", ".lhs"]
 
+-- | Parse the cabal file of the package from the given 'Context'.
+--
+--   This function reads the cabal file, gets some information about the compiler
+--   to be used corresponding to the stage it gets from the 'Context', and finalizes
+--   the package description it got from the cabal file with the additional information
+--   it got (e.g platform, compiler version conditionals, package flags).
 parseCabal :: Context -> Action Cabal
 parseCabal context@Context {..} = do
     let (Just file) = pkgCabalFile package
@@ -83,7 +91,7 @@ parseCabal context@Context {..} = do
     gpd <- liftIO $ C.readGenericPackageDescription C.verbose file
 
     -- configure the package with the ghc compiler for this stage.
-    hcPath <- builderPath' (Ghc CompileHs stage)
+    hcPath <- builderPath (Ghc CompileHs stage)
     (compiler, Just platform, _pgdb) <- liftIO $ GHC.configure C.silent (Just hcPath) Nothing Db.emptyProgramDb
 
 
@@ -109,6 +117,12 @@ parseCabal context@Context {..} = do
                    pd
                    depPkgs
 
+-- | This function runs the equivalent of @cabal configure@ using the Cabal library
+--   directly, collecting all the configuration options and flags to be passed to Cabal
+--   before invoking it.
+--
+--   It of course also 'need's package database entries for the dependencies of
+--   the package the 'Context' points to.
 configurePackage :: Context -> Action ()
 configurePackage context@Context {..} = do
     Just (Cabal _ _ _ gpd _pd depPkgs) <- readCabalFile context
@@ -148,7 +162,9 @@ configurePackage context@Context {..} = do
         liftIO $ do
           Hooks.defaultMainWithHooksNoReadArgs hooks gpd (argList ++ ["--flags=" ++ unwords flagList])
 
--- XXX: move this somewhere else. This is logic from ghc-cabal
+-- | Copies a built package (that the 'Context' points to) into a package
+--   database (the one for the ghc corresponding to the stage the 'Context'
+--   points to).
 copyPackage :: Context -> Action ()
 copyPackage context@Context {..} = do
   -- original invocation
@@ -164,6 +180,8 @@ copyPackage context@Context {..} = do
 
     liftIO $ Hooks.defaultMainWithHooksNoReadArgs hooks gpd ["copy", "--builddir", ctxPath, "--target-package-db", pkgDbPath]
 
+-- | Registers a built package (the one the 'Context' points to)
+--   into the package database.
 registerPackage :: Context -> Action ()
 registerPackage context@Context {..} = do
     top     <- topDirectory
@@ -175,9 +193,9 @@ registerPackage context@Context {..} = do
     liftIO $
       Hooks.defaultMainWithHooksNoReadArgs regHooks gpd ["register", "--builddir", ctxPath]
 
--- | Parse a ConfiguredCabal file.
-parseConfiguredCabal :: Context -> Action ConfiguredCabal
-parseConfiguredCabal context@Context {..} = do
+-- | Parses the 'PackageData' for a package (the one in the 'Context').
+parsePackageData :: Context -> Action PackageData
+parsePackageData context@Context {..} = do
     -- XXX: This is conceptually wrong!
     --      We should use the gpd, and
     --      the flagAssignment and compiler, hostPlatform, ... information
@@ -240,11 +258,10 @@ parseConfiguredCabal context@Context {..} = do
                     -- the RTS's library-dirs here.
             _ -> error "No (or multiple) ghc rts package is registered!!"
 
-      in return $ ConfiguredCabal
+      in return $ PackageData
       { dependencies = deps
       , name     = C.unPackageName . C.pkgName . C.package $ pd'
       , version  = C.display . C.pkgVersion . C.package $ pd'
-      -- , packageDesc = pd
       , componentId = C.localCompatPackageKey lbi'
       , modules  = map C.display . snd . biModules $ pd'
       , otherModules = map C.display . C.otherModules . fst . biModules $ pd'

src/Hadrian/Haskell/Cabal/Parse.hs-boot

@@ -2,8 +2,8 @@ module Hadrian.Haskell.Cabal.Parse where
 
 import Context.Type (Context)
 import Development.Shake (Action)
+import Hadrian.Haskell.Cabal.PackageData (PackageData)
 import Hadrian.Haskell.Cabal.Type (Cabal)
-import Hadrian.Haskell.Cabal.Configured (ConfiguredCabal)
 
 parseCabal :: Context -> Action Cabal
-parseConfiguredCabal :: Context -> Action ConfiguredCabal
+parsePackageData :: Context -> Action PackageData

src/Hadrian/Oracles/TextFile.hs

@@ -13,7 +13,7 @@
 module Hadrian.Oracles.TextFile (
     readTextFile, lookupValue, lookupValueOrEmpty, lookupValueOrError,
     lookupValues, lookupValuesOrEmpty, lookupValuesOrError, lookupDependencies,
-    readCabalFile, readConfiguredCabalFile, textFileOracle
+    readCabalFile, readPackageDataFile, textFileOracle
     ) where
 
 import Control.Monad
@@ -24,8 +24,8 @@ import Development.Shake.Classes
 import Development.Shake.Config
 
 import Context.Type
+import Hadrian.Haskell.Cabal.PackageData
 import Hadrian.Haskell.Cabal.Type
-import Hadrian.Haskell.Cabal.Configured
 import {-# SOURCE #-} Hadrian.Haskell.Cabal.Parse
 import Hadrian.Package
 import Hadrian.Utilities
@@ -39,9 +39,9 @@ newtype CabalFile = CabalFile Context
     deriving (Binary, Eq, Hashable, NFData, Show, Typeable)
 type instance RuleResult CabalFile = Maybe Cabal
 
-newtype ConfiguredCabalFile = ConfiguredCabalFile Context
+newtype PackageDataFile = PackageDataFile Context
     deriving (Binary, Eq, Hashable, NFData, Show, Typeable)
-type instance RuleResult ConfiguredCabalFile = Maybe ConfiguredCabal
+type instance RuleResult PackageDataFile = Maybe PackageData
 
 newtype KeyValue = KeyValue (FilePath, String)
     deriving (Binary, Eq, Hashable, NFData, Show, Typeable)
@@ -102,8 +102,8 @@ lookupDependencies depFile file = do
 readCabalFile :: Context -> Action (Maybe Cabal)
 readCabalFile = askOracle . CabalFile
 
-readConfiguredCabalFile :: Context -> Action (Maybe ConfiguredCabal)
-readConfiguredCabalFile = askOracle . ConfiguredCabalFile
+readPackageDataFile :: Context -> Action (Maybe PackageData)
+readPackageDataFile = askOracle . PackageDataFile
 
 -- | This oracle reads and parses text files to answer 'readTextFile' and
 -- 'lookupValue' queries, as well as their derivatives, tracking the results.
@@ -142,8 +142,8 @@ textFileOracle = do
         case pkgCabalFile package of
           Just file -> do
             need [file]
-            putLoud $ "| ConfiguredCabalFile oracle: reading " ++ quote file ++ " (Stage: " ++ stageString stage ++ ")..."
-            Just <$> parseConfiguredCabal ctx
+            putLoud $ "| PackageDataFile oracle: reading " ++ quote file ++ " (Stage: " ++ stageString stage ++ ")..."
+            Just <$> parsePackageData ctx
           Nothing -> return Nothing
 
-    void $ addOracle $ \(ConfiguredCabalFile ctx) -> confCabal ctx
+    void $ addOracle $ \(PackageDataFile ctx) -> confCabal ctx

src/Hadrian/Package.hs

@@ -63,6 +63,10 @@ isCPackage _ = False
 -- | Is this a Haskell package?
 isHsPackage :: Package -> Bool
 isHsPackage (Package Haskell _ _ _) = True
+-- we consider the RTS as a haskell package because we
+-- use information from its Cabal file to build it,
+-- and we e.g want 'pkgCabalFile' to point us to
+-- 'rts/rts.cabal' when passed the rts package as argument.
 isHsPackage (Package _ _ "rts" _)   = True
 isHsPackage _ = False