Fix package promotion to snapshot

snoyberg · snoyberg · commit 4d9110fa1da3 · 2018-01-10T15:38:05.000-08:00
Fixes commercialhaskell/stackage#3185
diff --git a/ChangeLog.md b/ChangeLog.md
@@ -14,6 +14,12 @@ Bug fixes:
 * 1.6.1 introduced a change that made some precompiled cache files use
   longer paths, sometimes causing builds to fail on windows. This has been
   fixed. See [#3649](https://github.com/commercialhaskell/stack/issues/3649)
+* Correct the behavior of promoting a package from snapshot to local
+  package. This would get triggered when version bounds conflicted in
+  a snapshot, which could be triggered via Hackage revisions for old
+  packages. This also should allow custom snapshots to define
+  conflicting versions of packages without issue. See
+  [Stackage issue #3185](https://github.com/fpco/stackage/issues/3185).
 
 
 ## v1.6.3
diff --git a/src/Stack/Snapshot.hs b/src/Stack/Snapshot.hs
@@ -733,10 +733,35 @@ splitUnmetDeps extra =
 
     depsMet globals = all (depsMet' globals) . Map.toList . lpiPackageDeps
 
-    depsMet' globals (name, intervals) =
+    -- MSS 2018-01-10. Previously, we would actually perform a version
+    -- bounds check at this point. I believe this is a mistake: we
+    -- don't want to promote a package from a snapshot to a local just
+    -- because the version ranges aren't satisfied. In fact, we
+    -- intentionally allow snapshots to specify mismatched versions of
+    -- packages, and try building anyway.
+    --
+    -- With the old behavior: a number of packages would be converted
+    -- and treated as local packages. I specifically stumbled on this
+    -- while investigating Stackage issues #3185, where a revision to
+    -- semigroupoids's tagged dependency caused the builds to
+    -- break. Stack should have just ignored this and printed a
+    -- warning. Instead, Stack believed that semigroupoids was a local
+    -- package, not a snapshot package, and failed.
+    --
+    -- All that said: I'm pretty certain this is the right behavior,
+    -- but all of this is strongly indicating that we need some code
+    -- cleanup around this promotion business. I don't think I did a
+    -- particularly good job on this code during the extensible
+    -- snapshot rewrite.
+    depsMet' globals (name, _intervals) =
       case (lpiVersion <$> Map.lookup name globals) <|> Map.lookup name extra of
+        -- The dependency doesn't exist at all in the snapshot or
+        -- extra, therefore this package must be promoted to local as
+        -- well.
         Nothing -> False
-        Just version -> version `withinIntervals` intervals
+        -- It exists. As explained above, don't bother checking the
+        -- version bounds, we trust the snapshot.
+        Just _version -> True
 
 -- | Calculate a 'LoadedPackageInfo' from the given 'GenericPackageDescription'
 calculate :: GenericPackageDescription
diff --git a/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/Main.hs b/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/Main.hs
@@ -0,0 +1,6 @@
+import StackTest
+
+main :: IO ()
+main = do
+  stack ["build", "--dry-run"]
+  stackErr ["build", "--stack-yaml", "with-rev.yaml", "--dry-run"]
diff --git a/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/files/.gitignore b/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/files/.gitignore
@@ -0,0 +1 @@
+/unimportant.cabal
diff --git a/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/files/package.yaml b/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/files/package.yaml
@@ -0,0 +1,8 @@
+name: unimportant
+
+dependencies:
+- base
+- semigroupoids
+- tagged
+
+library: {}
diff --git a/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/files/stack.yaml b/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/files/stack.yaml
@@ -0,0 +1 @@
+resolver: lts-3.12
diff --git a/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/files/with-rev.yaml b/test/integration/tests/stackage-3185-ignore-bounds-in-snapshot/files/with-rev.yaml
@@ -0,0 +1,3 @@
+resolver: lts-3.12
+extra-deps:
+- semigroupoids-5.0.0.4@rev:1

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+resolver: lts-3.12`
	`2`	`+extra-deps:`
	`3`	`+- semigroupoids-5.0.0.4@rev:1`