Merge pull request #3503 from commercialhaskell/2781-shadow-bug

Force reconfigure when deps are rebuilt #2781

Author: snoyberg

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

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

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 =

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]

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
 

test/integration/tests/2781-shadow-bug/Main.hs

@@ -0,0 +1,10 @@
+import StackTest
+import System.Directory
+
+main :: IO ()
+main = do
+  createDirectoryIfMissing True "foo/src"
+  readFile "foo/v1/Foo.hs" >>= writeFile "foo/src/Foo.hs"
+  stack ["bench"]
+  readFile "foo/v2/Foo.hs" >>= writeFile "foo/src/Foo.hs"
+  stack ["bench"]

test/integration/tests/2781-shadow-bug/files/bar/bar.cabal

@@ -0,0 +1,18 @@
+name:                bar
+version:             0.1.0.0
+build-type:          Simple
+cabal-version:       >=1.10
+
+library
+  hs-source-dirs:      src
+  exposed-modules:     Bar
+  build-depends:       base, foo
+  default-language:    Haskell2010
+
+benchmark bench
+  type:                exitcode-stdio-1.0
+  hs-source-dirs:      bench
+  main-is:             bench.hs
+  build-depends:       base
+                     , bar
+  default-language:    Haskell2010

test/integration/tests/2781-shadow-bug/files/bar/bench/bench.hs

@@ -0,0 +1,2 @@
+main :: IO ()
+main = return ()

test/integration/tests/2781-shadow-bug/files/bar/src/Bar.hs

@@ -0,0 +1,8 @@
+module Bar
+    ( bar
+    ) where
+
+import Foo
+
+bar :: IO ()
+bar = foo

test/integration/tests/2781-shadow-bug/files/foo/foo.cabal

@@ -0,0 +1,10 @@
+name:                foo
+version:             0.1.0.0
+build-type:          Simple
+cabal-version:       >=1.10
+
+library
+  hs-source-dirs:      src
+  exposed-modules:     Foo
+  build-depends:       base
+  default-language:    Haskell2010