Drop version check when resolving package names.

In #4017, hvr reported that when he used --allow-older/--allow-newer,
there was an assert failure in toConfiguredComponent.  Indeed
the problem was that toConfiguredComponent was testing version
ranges of build-depends to determine which package to select, but
there was no satisfying one (since the build-depends field had
not been updated.)

After thinking about this for a bit, it seemed a bit bogus for
us to be doing another version check at this late phase; we
already picked dependencies earlier in the configuration process.
So I decided to drop it.

To drop it, however, I needed to remove support for a feature (discussed
in #4020), which uses version ranges to disambiguate whether or not a
dependency is on an external package or an internal package.  This
feature doesn't seem to be very useful.  If someone asks, I'll
check on Hackage to see if anyone is using it.

Also added some useful extra debug info.

Fixes #4020 and #4017

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

Author: ezyang

Cabal/Distribution/Backpack/ConfiguredComponent.hs

@@ -126,22 +126,16 @@ toConfiguredComponent pkg_descr this_cid
        lib_deps exe_deps component
   where
     bi = componentBuildInfo component
-    find_it :: PackageName -> VersionRange -> (ComponentId, PackageId)
-    find_it name reqVer =
-        fromMaybe (error ("toConfiguredComponent: " ++ display name)) $
-            lookup_name lib_map <|>
-            lookup_name external_lib_map
-      where
-        lookup_name m =
-            case Map.lookup name m of
-                Just (cid, pkgid)
-                    | packageVersion pkgid `withinRange` reqVer
-                    -> Just (cid, pkgid)
-                _ -> Nothing
+    find_it :: PackageName -> (ComponentId, PackageId)
+    find_it name =
+        fromMaybe (error ("toConfiguredComponent: " ++ display (packageName pkg_descr) ++
+                            " " ++ display name)) $
+            Map.lookup name lib_map <|>
+            Map.lookup name external_lib_map
     lib_deps
         | newPackageDepsBehaviour pkg_descr
-        = [ (name, find_it name reqVer)
-          | Dependency name reqVer <- targetBuildDepends bi ]
+        = [ (name, find_it name)
+          | Dependency name _ <- targetBuildDepends bi ]
         | otherwise
         = Map.toList external_lib_map
     exe_deps = [ cid

Cabal/Distribution/PackageDescription/Check.hs

@@ -520,6 +520,14 @@ checkFields pkg =
         ++ "different versions of the same compiler use multiple entries, "
         ++ "for example 'tested-with: GHC==6.10.4, GHC==6.12.3' and not "
         ++ "'tested-with: GHC==6.10.4 && ==6.12.3'."
+
+  , check (not (null buildDependsRangeOnInternalLibrary)) $
+      PackageBuildWarning $
+           "The package has a version range for a dependency on an "
+        ++ "internal library: "
+        ++ commaSep (map display buildDependsRangeOnInternalLibrary)
+        ++ ". This version range has no semantic meaning and can be "
+        ++ "removed."
   ]
   where
     unknownCompilers  = [ name | (OtherCompiler name, _) <- testedWith pkg ]
@@ -542,6 +550,17 @@ checkFields pkg =
       | (compiler, vr) <- testedWith pkg
       , isNoVersion vr ]
 
+    internalLibraries =
+        map (maybe (packageName pkg) mkPackageName . libName)
+            (allLibraries pkg)
+    buildDependsRangeOnInternalLibrary =
+      [ dep
+      | bi <- allBuildInfo pkg
+      , dep@(Dependency name versionRange) <- targetBuildDepends bi
+      , not (isAnyVersion versionRange)
+      , name `elem` internalLibraries
+      ]
+
 
 checkLicense :: PackageDescription -> [PackageCheck]
 checkLicense pkg =

Cabal/Distribution/Simple/Configure.hs

@@ -817,8 +817,8 @@ dependencySatisfiable
     -> Map PackageName InstalledPackageInfo -- ^ required dependencies
     -> (Dependency -> Bool)
 dependencySatisfiable
-    exact_config pkg_ver installedPackageSet internalPackageSet requiredDepsMap
-    d@(Dependency depName verRange)
+    exact_config _ installedPackageSet internalPackageSet requiredDepsMap
+    d@(Dependency depName _)
       | exact_config =
         -- When we're given '--exact-configuration', we assume that all
         -- dependencies and flags are exactly specified on the command
@@ -838,17 +838,12 @@ dependencySatisfiable
         -- when it actually isn't.
         (depName `Map.member` requiredDepsMap) || isInternalDep
 
-      | isInternalDep
-      , pkg_ver `withinRange` verRange =
-        -- If a 'PackageName' is defined by an internal component,
-        -- and the user didn't specify a version range which is
-        -- incompatible with the package version, the dep is
-        -- satisfiable (and we are going to use the internal
-        -- dependency.)  Note that this doesn't mean we are
-        -- actually going to SUCCEED when we configure the package,
-        -- if UseExternalInternalDeps is True.  NB: if
-        -- the version bound fails we want to fall through to the
-        -- next case.
+      | isInternalDep =
+        -- If a 'PackageName' is defined by an internal component, the
+        -- dep is satisfiable (and we are going to use the internal
+        -- dependency.)  Note that this doesn't mean we are actually
+        -- going to SUCCEED when we configure the package, if
+        -- UseExternalInternalDeps is True.
         True
 
       | otherwise =
@@ -1164,13 +1159,11 @@ selectDependency pkgid internalIndex installedIndex requiredDepsMap
   --
   -- We want "build-depends: MyLibrary" always to match the internal library
   -- even if there is a newer installed library "MyLibrary-0.2".
-  -- However, "build-depends: MyLibrary >= 0.2" should match the installed one.
   case Map.lookup dep_pkgname internalIndex of
-    Just cname | packageVersion pkgid `withinRange` vr
-           -> if use_external_internal_deps
-                then do_external (Just cname)
-                else do_internal
-    _      -> do_external Nothing
+    Just cname -> if use_external_internal_deps
+                    then do_external (Just cname)
+                    else do_internal
+    _          -> do_external Nothing
   where
     do_internal = Right (InternalDependency dep
                     (PackageIdentifier dep_pkgname (packageVersion pkgid)))

Cabal/changelog

@@ -98,6 +98,10 @@
 	  or greater, respectively).
 	* New `Distribution.Utils.ShortText.ShortText` type for representing
 	  short text strings compactly (#3898)
+	* Cabal no longer supports using a version bound to disambiguate
+	  between an internal and external package (#4020).  This should
+	  not affect many people, as this mode of use already did not
+	  work with the dependency solver.
 
 1.24.0.0 Ryan Thomas <[email protected]> March 2016
 	* Support GHC 8.

Cabal/tests/PackageTests/Tests.hs

@@ -120,9 +120,17 @@ tests config = do
   -- with the same name and LATER version
   tc "BuildDeps/InternalLibrary3" $ internal_lib_test "internal"
 
-  -- Test that an explicit dependency constraint which doesn't
-  -- match the internal library causes us to use external library
-  tc "BuildDeps/InternalLibrary4" $ internal_lib_test "installed"
+  -- On old versions of Cabal, an explicit dependency constraint which
+  -- doesn't match the internal library causes us to use external
+  -- library.  We got rid of this functionality in 1.25; now, we always
+  -- use internal, and just emit an error if you check.
+  tcs "BuildDeps/InternalLibrary4" "external" $ internal_lib_test "internal"
+
+  -- On a previous buggy version of my patch, the test above passed only
+  -- because the installed library caused Cabal to think that the
+  -- dependency was fulfilled.  Make sure we ignore the dependency
+  -- entirely.
+  tcs "BuildDeps/InternalLibrary4" "ignore" $ cabal_build []
 
   -- Test "old build-dep behavior", where we should get the
   -- same package dependencies on all targets if cabal-version

cabal-install/Distribution/Client/ProjectPlanning.hs

@@ -1096,6 +1096,8 @@ elaborateInstallPlan verbosity platform compiler compilerprogdb pkgConfigDB
         -> LogProgress [ElaboratedConfiguredPackage]
     elaborateSolverToComponents mapDep spkg@(SolverPackage _ _ _ deps0 exe_deps0)
         | Right g <- toComponentsGraph (elabEnabledSpec elab0) pd = do
+            infoProgress $ hang (text "Component graph for" <+> disp pkgid <<>> colon)
+                            4 (dispComponentsGraph g)
             (_, comps) <- mapAccumM buildComponent
                             ((Map.empty, Map.empty), Map.empty, Map.empty)
                             (map fst g)

cabal-install/cabal-install.cabal

@@ -139,6 +139,10 @@ Extra-Source-Files:
   tests/IntegrationTests/new-build/T3978/cabal.project
   tests/IntegrationTests/new-build/T3978/p/p.cabal
   tests/IntegrationTests/new-build/T3978/q/q.cabal
+  tests/IntegrationTests/new-build/T4017.sh
+  tests/IntegrationTests/new-build/T4017/cabal.project
+  tests/IntegrationTests/new-build/T4017/p/p.cabal
+  tests/IntegrationTests/new-build/T4017/q/q.cabal
   tests/IntegrationTests/new-build/executable/Main.hs
   tests/IntegrationTests/new-build/executable/Setup.hs
   tests/IntegrationTests/new-build/executable/Test.hs

cabal-install/tests/IntegrationTests/new-build/T4017.sh

@@ -0,0 +1,3 @@
+. ./common.sh
+cd T4017
+cabal new-build q

cabal-install/tests/IntegrationTests/new-build/T4017/cabal.project

@@ -0,0 +1,2 @@
+packages: p q
+allow-older: p:base

cabal-install/tests/IntegrationTests/new-build/T4017/p/p.cabal

@@ -0,0 +1,7 @@
+name: p
+version: 1.0
+build-type: Simple
+cabal-version: >= 1.10
+
+library
+    build-depends: base >= 99999

cabal-install/tests/IntegrationTests/new-build/T4017/q/q.cabal

@@ -0,0 +1,7 @@
+name: q
+version: 1.0
+build-type: Simple
+cabal-version: >= 1.10
+
+library
+    build-depends: p