From f381972cc30090c13856721cab74272dd159109f Mon Sep 17 00:00:00 2001
From: Michael Snoyman <michael@snoyman.com>
Date: Sun, 22 Oct 2017 05:27:40 +0300
Subject: [PATCH] Force reconfigure when deps rebuild #2781

---
 ChangeLog.md                     |  5 +++++
 src/Stack/Build/ConstructPlan.hs |  2 ++
 src/Stack/Build/Execute.hs       |  7 ++++---
 src/Stack/SDist.hs               |  1 +
 src/Stack/Types/Build.hs         | 11 +++++++++++
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/ChangeLog.md b/ChangeLog.md
index 6edfb923da..271ec55d04 100644
--- a/ChangeLog.md
+++ b/ChangeLog.md
@@ -150,6 +150,11 @@ Bug fixes:
   [#3476](https://github.com/commercialhaskell/stack/issues/3476).
 * Properly handle relative paths stored in the precompiled cache files. See
   [#3431](https://github.com/commercialhaskell/stack/issues/3431).
+* In some cases, Cabal does not realize that it needs to reconfigure, and must
+  be told to do so automatically. This would manifest as a "shadowed
+  dependency" error message. We now force a reconfigure whenever a dependency is
+  built, even if the package ID remained the same. See
+  [#2781](https://github.com/commercialhaskell/stack/issues/2781).
 
 
 ## 1.5.1
diff --git a/src/Stack/Build/ConstructPlan.hs b/src/Stack/Build/ConstructPlan.hs
index 21f2339370..37dbd5aa52 100644
--- a/src/Stack/Build/ConstructPlan.hs
+++ b/src/Stack/Build/ConstructPlan.hs
@@ -354,6 +354,7 @@ addFinal lp package isAllInOne = do
                 , taskType = TTFiles lp Local -- FIXME we can rely on this being Local, right?
                 , taskAllInOne = isAllInOne
                 , taskCachePkgSrc = CacheSrcLocal (toFilePath (lpDir lp))
+                , taskAnyMissing = not $ Set.null missing
                 }
     tell mempty { wFinals = Map.singleton (packageName package) res }
 
@@ -562,6 +563,7 @@ installPackageGivenDeps isAllInOne ps package minstalled (missing, present, minL
                     PSIndex loc _ _ pkgLoc -> TTIndex package (loc <> minLoc) pkgLoc
             , taskAllInOne = isAllInOne
             , taskCachePkgSrc = toCachePkgSrc ps
+            , taskAnyMissing = not $ Set.null missing
             }
 
 -- Update response in the lib map. If it is an error, and there's
diff --git a/src/Stack/Build/Execute.hs b/src/Stack/Build/Execute.hs
index d2984422c5..f12417ba5d 100644
--- a/src/Stack/Build/Execute.hs
+++ b/src/Stack/Build/Execute.hs
@@ -787,11 +787,12 @@ ensureConfig :: HasEnvConfig env
              -> RIO env () -- ^ announce
              -> (ExcludeTHLoading -> [String] -> RIO env ()) -- ^ cabal
              -> Path Abs File -- ^ .cabal file
+             -> Task
              -> RIO env Bool
-ensureConfig newConfigCache pkgDir ExecuteEnv {..} announce cabal cabalfp = do
+ensureConfig newConfigCache pkgDir ExecuteEnv {..} announce cabal cabalfp task = do
     newCabalMod <- liftIO (fmap modTime (D.getModificationTime (toFilePath cabalfp)))
     needConfig <-
-        if boptsReconfigure eeBuildOpts
+        if boptsReconfigure eeBuildOpts || taskAnyMissing task
             then return True
             else do
                 -- We can ignore the components portion of the config
@@ -1305,7 +1306,7 @@ singleBuild runInBase ac@ActionContext {..} ee@ExecuteEnv {..} task@Task {..} in
                       ("Building all executables for `" <> packageNameText (packageName package) <>
                        "' once. After a successful build of all of them, only specified executables will be rebuilt."))
 
-            _neededConfig <- ensureConfig cache pkgDir ee (announce ("configure" <> annSuffix executableBuildStatuses)) cabal cabalfp
+            _neededConfig <- ensureConfig cache pkgDir ee (announce ("configure" <> annSuffix executableBuildStatuses)) cabal cabalfp task
 
             let installedMapHasThisPkg :: Bool
                 installedMapHasThisPkg =
diff --git a/src/Stack/SDist.hs b/src/Stack/SDist.hs
index f542e7b283..5f5e9d9908 100644
--- a/src/Stack/SDist.hs
+++ b/src/Stack/SDist.hs
@@ -291,6 +291,7 @@ getSDistFileList lp =
         , taskPresent = Map.empty
         , taskAllInOne = True
         , taskCachePkgSrc = CacheSrcLocal (toFilePath (lpDir lp))
+        , taskAnyMissing = True
         }
 
 normalizeTarballPaths :: HasRunner env => [FilePath] -> RIO env [FilePath]
diff --git a/src/Stack/Types/Build.hs b/src/Stack/Types/Build.hs
index 6cb4cd6171..8e08789d5c 100644
--- a/src/Stack/Types/Build.hs
+++ b/src/Stack/Types/Build.hs
@@ -404,6 +404,17 @@ data Task = Task
     , taskAllInOne        :: !Bool
     -- ^ indicates that the package can be built in one step
     , taskCachePkgSrc     :: !CachePkgSrc
+    , taskAnyMissing      :: !Bool
+    -- ^ Were any of the dependencies missing? The reason this is
+    -- necessary is... hairy. And as you may expect, a bug in
+    -- Cabal. See:
+    -- <https://github.com/haskell/cabal/issues/4728#issuecomment-337937673>. The
+    -- problem is that Cabal may end up generating the same package ID
+    -- for a dependency, even if the ABI has changed. As a result,
+    -- without this field, Stack would think that a reconfigure is
+    -- unnecessary, when in fact we _do_ need to reconfigure. The
+    -- details here suck. We really need proper hashes for package
+    -- identifiers.
     }
     deriving Show