Consistently update 'localPkgDescr' with 'HookedBuildInfo' + docs

I got sucked into this rathole while puzzling over why
we pass both 'PackageDescription' and 'LocalBuildInfo' (which
has a 'PackageDescription' in 'localPkgDescr').  The conclusion
is that they were different, but not for a good reason. Now they're
the same.

Added docs fixes #1757.

Signed-off-by: Edward Z. Yang <[email protected]>

Cabal/Distribution/Simple.hs

@@ -187,17 +187,14 @@ configureAction :: UserHooks -> ConfigFlags -> Args -> IO LocalBuildInfo
 configureAction hooks flags args = do
     distPref <- findDistPrefOrDefault (configDistPref flags)
     let flags' = flags { configDistPref = toFlag distPref }
+
+    -- See docs for 'HookedBuildInfo'
     pbi <- preConf hooks args flags'
 
     (mb_pd_file, pkg_descr0) <- confPkgDescr hooks verbosity
 
-    --get_pkg_descr (configVerbosity flags')
-    --let pkg_descr = updatePackageDescription pbi pkg_descr0
     let epkg_descr = (pkg_descr0, pbi)
 
-    --(warns, ers) <- sanityCheckPackage pkg_descr
-    --errorOut (configVerbosity flags') warns ers
-
     localbuildinfo0 <- confHook hooks epkg_descr flags'
 
     -- remember the .cabal filename if we know it
@@ -252,10 +249,15 @@ replAction hooks flags args = do
              (replProgramArgs flags')
              (withPrograms lbi)
 
+  -- As far as I can tell, the only reason this doesn't use
+  -- 'hookedActionWithArgs' is because the arguments of 'replHook'
+  -- takes the args explicitly.  UGH.   -- ezyang
   pbi <- preRepl hooks args flags'
-  let lbi' = lbi { withPrograms = progs }
-      pkg_descr0 = localPkgDescr lbi'
-      pkg_descr = updatePackageDescription pbi pkg_descr0
+  let pkg_descr0 = localPkgDescr lbi
+  sanityCheckHookedBuildInfo pkg_descr0 pbi
+  let pkg_descr = updatePackageDescription pbi pkg_descr0
+      lbi' = lbi { withPrograms = progs
+                 , localPkgDescr = pkg_descr }
   replHook hooks pkg_descr lbi' hooks flags' args
   postRepl hooks args flags' pkg_descr lbi'
 
@@ -292,6 +294,12 @@ cleanAction hooks flags args = do
     pbi <- preClean hooks args flags'
 
     (_, ppd) <- confPkgDescr hooks verbosity
+    -- It might seem like we are doing something clever here
+    -- but we're really not: if you look at the implementation
+    -- of 'clean' in the end all the package description is
+    -- used for is to clear out @extra-tmp-files@.  IMO,
+    -- the configure script goo should go into @dist@ too!
+    --          -- ezyang
     let pkg_descr0 = flattenPackageDescription ppd
     -- We don't sanity check for clean as an error
     -- here would prevent cleaning:
@@ -333,9 +341,10 @@ sdistAction hooks flags args = do
     let pkg_descr0 = flattenPackageDescription ppd
     sanityCheckHookedBuildInfo pkg_descr0 pbi
     let pkg_descr = updatePackageDescription pbi pkg_descr0
+        mlbi' = fmap (\lbi -> lbi { localPkgDescr = pkg_descr }) mlbi
 
-    sDistHook hooks pkg_descr mlbi hooks flags'
-    postSDist hooks args flags' pkg_descr mlbi
+    sDistHook hooks pkg_descr mlbi' hooks flags'
+    postSDist hooks args flags' pkg_descr mlbi'
   where
     verbosity = fromFlag (sDistVerbosity flags)
 
@@ -401,15 +410,13 @@ hookedActionWithArgs :: (UserHooks -> Args -> flags -> IO HookedBuildInfo)
         -> UserHooks -> flags -> Args -> IO ()
 hookedActionWithArgs pre_hook cmd_hook post_hook get_build_config hooks flags args = do
    pbi <- pre_hook hooks args flags
-   localbuildinfo <- get_build_config
-   let pkg_descr0 = localPkgDescr localbuildinfo
-   --pkg_descr0 <- get_pkg_descr (get_verbose flags)
+   lbi0 <- get_build_config
+   let pkg_descr0 = localPkgDescr lbi0
    sanityCheckHookedBuildInfo pkg_descr0 pbi
    let pkg_descr = updatePackageDescription pbi pkg_descr0
-   -- TODO: should we write the modified package descr back to the
-   -- localbuildinfo?
-   cmd_hook hooks args pkg_descr localbuildinfo hooks flags
-   post_hook hooks args flags pkg_descr localbuildinfo
+       lbi = lbi0 { localPkgDescr = pkg_descr }
+   cmd_hook hooks args pkg_descr lbi hooks flags
+   post_hook hooks args flags pkg_descr lbi
 
 sanityCheckHookedBuildInfo :: PackageDescription -> HookedBuildInfo -> IO ()
 sanityCheckHookedBuildInfo pkg_descr hooked_bis
@@ -562,7 +569,8 @@ defaultUserHooks = autoconfUserHooks {
                    pbi <- getHookedBuildInfo verbosity
                    sanityCheckHookedBuildInfo pkg_descr pbi
                    let pkg_descr' = updatePackageDescription pbi pkg_descr
-                   postConf simpleUserHooks args flags pkg_descr' lbi
+                       lbi' = lbi { localPkgDescr = pkg_descr' }
+                   postConf simpleUserHooks args flags pkg_descr' lbi'
 
           backwardsCompatHack = True
 
@@ -593,7 +601,8 @@ autoconfUserHooks
                    pbi <- getHookedBuildInfo verbosity
                    sanityCheckHookedBuildInfo pkg_descr pbi
                    let pkg_descr' = updatePackageDescription pbi pkg_descr
-                   postConf simpleUserHooks args flags pkg_descr' lbi
+                       lbi' = lbi { localPkgDescr = pkg_descr' }
+                   postConf simpleUserHooks args flags pkg_descr' lbi'
 
           backwardsCompatHack = False
 

Cabal/Distribution/Types/HookedBuildInfo.hs

@@ -9,6 +9,55 @@ module Distribution.Types.HookedBuildInfo (
 import Distribution.Types.ComponentName
 import Distribution.Types.BuildInfo
 
+-- | 'HookedBuildInfo' is mechanism that hooks can use to
+-- override the 'BuildInfo's inside packages.  One example
+-- use-case (which is used in core libraries today) is as
+-- a way of passing flags which are computed by a configure
+-- script into Cabal.  In this case, the autoconf build type adds
+-- hooks to read in a textual 'HookedBuildInfo' format prior
+-- to doing any operations.
+--
+-- Quite honestly, this mechanism is a massive hack since we shouldn't
+-- be editing the 'PackageDescription' data structure (it's easy
+-- to assume that this data structure shouldn't change and
+-- run into bugs, see for example 1c20a6328579af9e37677d507e2e9836ef70ab9d).
+-- But it's a bit convenient, because there isn't another data
+-- structure that allows adding extra 'BuildInfo' style things.
+--
+-- In any case, a lot of care has to be taken to make sure the
+-- 'HookedBuildInfo' is applied to the 'PackageDescription'.  In
+-- general this process occurs in "Distribution.Simple", which is
+-- responsible for orchestrating the hooks mechanism.  The
+-- general strategy:
+--
+--      1. We run the pre-hook, which produces a 'HookedBuildInfo'
+--         (e.g., in the Autoconf case, it reads it out from a file).
+--      2. We sanity-check the hooked build info with
+--         'sanityCheckHookedBuildInfo'.
+--      3. We update our 'PackageDescription' (either freshly read
+--         or cached from 'LocalBuildInfo') with 'updatePackageDescription'.
+--         In doing so, we must be careful to also update it in
+--         'localPkgDescr' in 'LocalBuildInfo', where a user can
+--         also get to it.  In practice the code also passes around
+--         the updated 'PackageDescription' around explicitly (redundantly)
+--         which is what everyone used to get the actual 'Component'.
+--         And that's a good thing because up until recently
+--         'localPkgDescr' was NOT updated.  Although, it doesn't
+--         look like anyone saw a bug because of this, thanks to
+--         1c20a6328579af9e37677d507e2e9836ef70ab9d.
+--
+-- It is not well-specified whether or not a 'HookedBuildInfo' applied
+-- at configure time is persistent to the 'LocalBuildInfo'.  The
+-- fact that 'HookedBuildInfo' is passed to 'confHook' MIGHT SUGGEST
+-- that the 'HookedBuildInfo' is applied at this time, but actually
+-- since 9317b67e6122ab14e53f81b573bd0ecb388eca5a it has been ONLY used
+-- to create a modified package description that we check for problems:
+-- it is never actually saved to the LBI.  Since 'HookedBuildInfo' is
+-- applied monoidally to the existing build infos (and it is not an
+-- idempotent monoid), it could break things to save it, since we
+-- are obligated to apply any new 'HookedBuildInfo' and then we'd
+-- get the effect twice.  But this does mean we have to re-apply
+-- it every time. Hey, it's more flexibility.
 type HookedBuildInfo = [(ComponentName, BuildInfo)]
 
 emptyHookedBuildInfo :: HookedBuildInfo