Merge pull request #4058 from dcoutts/issue3932

Refine the fix for requiring Cabal > 1.20 for Setup.hs

Fixes #4196

Author: hvr

cabal-install/Distribution/Client/Dependency.hs

@@ -56,6 +56,7 @@ module Distribution.Client.Dependency (
     removeLowerBounds,
     removeUpperBounds,
     addDefaultSetupDependencies,
+    addSetupCabalMinVersionConstraint,
   ) where
 
 import Distribution.Solver.Modular
@@ -84,9 +85,9 @@ import Distribution.PackageDescription.Configuration
 import Distribution.Client.PackageUtils
          ( externalBuildDepends )
 import Distribution.Version
-         ( mkVersion, VersionRange, anyVersion, thisVersion, orLaterVersion
-         , withinRange, simplifyVersionRange
-         , removeLowerBound, removeUpperBound )
+         ( Version, mkVersion
+         , VersionRange, anyVersion, thisVersion, orLaterVersion, withinRange
+         , simplifyVersionRange, removeLowerBound, removeUpperBound )
 import Distribution.Compiler
          ( CompilerInfo(..) )
 import Distribution.System
@@ -466,6 +467,22 @@ addDefaultSetupDependencies defaultSetupDeps params =
         gpkgdesc = packageDescription srcpkg
         pkgdesc  = PD.packageDescription gpkgdesc
 
+-- | There If a package has a custom setup then we need to add a setup-depends
+-- on Cabal. For now it's easier to add this unconditionally.  Once
+-- qualified constraints land we can turn this into a custom setup
+-- only constraint.
+--
+addSetupCabalMinVersionConstraint :: Version
+                                  -> DepResolverParams -> DepResolverParams
+addSetupCabalMinVersionConstraint minVersion =
+    addConstraints
+      [ LabeledPackageConstraint
+          (PackageConstraintVersion cabalPkgname (orLaterVersion minVersion))
+          ConstraintSetupCabalMinVersion
+      ]
+  where
+    cabalPkgname = mkPackageName "Cabal"
+
 
 upgradeDependencies :: DepResolverParams -> DepResolverParams
 upgradeDependencies = setPreferenceDefault PreferAllLatest

cabal-install/Distribution/Client/ProjectPlanning.hs

@@ -952,23 +952,25 @@ planPackages comp platform solver SolverSettings{..}
                                    . PD.packageDescription
                                    . packageDescription)
 
+      . addSetupCabalMinVersionConstraint (mkVersion [1,20])
+          -- While we can talk to older Cabal versions (we need to be able to
+          -- do so for custom Setup scripts that require older Cabal lib
+          -- versions), we have problems talking to some older versions that
+          -- don't support certain features.
+          --
+          -- For example, Cabal-1.16 and older do not know about build targets.
+          -- Even worse, 1.18 and older only supported the --constraint flag
+          -- with source package ids, not --dependency with installed package
+          -- ids. That is bad because we cannot reliably select the right
+          -- dependencies in the presence of multiple instances (i.e. the
+          -- store). See issue #3932. So we require Cabal 1.20 as a minimum.
+
       . addPreferences
           -- preferences from the config file or command line
           [ PackageVersionPreference name ver
           | Dependency name ver <- solverSettingPreferences ]
 
       . addConstraints
-
-          -- If a package has a custom setup then we need to add a setup-depends
-          -- on Cabal. For now it's easier to add this unconditionally.  Once
-          -- qualified constraints land we can turn this into a custom setup
-          -- only constraint.
-          --
-          -- TODO: use a qualified constraint
-            [ LabeledPackageConstraint (PackageConstraintVersion cabalPkgname
-                                       (orLaterVersion (mkVersion [1,20])))
-                                       ConstraintNewBuildCustomSetupLowerBoundCabal
-                                       ] . addConstraints
           -- version constraints from the config file or command line
             [ LabeledPackageConstraint (userToPackageConstraint pc) src
             | (pc, src) <- solverSettingConstraints ]
@@ -2397,6 +2399,10 @@ packageSetupScriptStyle pkg
 -- we still need to distinguish the case of explicit and implict setup deps.
 -- See 'rememberImplicitSetupDeps'.
 --
+-- Note in addition to adding default setup deps, we also use
+-- 'addSetupCabalMinVersionConstraint' (in 'planPackages') to require
+-- @Cabal >= 1.20@ for Setup scripts.
+--
 defaultSetupDeps :: Compiler -> Platform
                  -> PD.PackageDescription
                  -> Maybe [Dependency]
@@ -2415,35 +2421,19 @@ defaultSetupDeps compiler platform pkg =
         where
           -- The Cabal dep is slightly special:
           -- * We omit the dep for the Cabal lib itself, since it bootstraps.
-          -- * We constrain it to be >= 1.18 < 2
+          -- * We constrain it to be < 1.25
           --
-          -- Note: cabalCompatMinVer only gets applied WHEN WE ARE ADDING a
-          -- default setup build info, i.e., when there is no custom-setup
-          -- stanza. If there is a custom-setup stanza, this codepath never gets
-          -- invoked (that's why there's an error case for
-          -- SetupCustomExplicitDeps).
+          -- Note: we also add a global constraint to require Cabal >= 1.20
+          -- for Setup scripts (see use addSetupCabalMinVersionConstraint).
           --
-          -- One way we could solve this problem is by also modifying
-          -- custom-setup stanzas when they exist, but we're going to take a
-          -- different approach: add an extra constraint on Cabal globally to
-          -- make sure the solver respects it regardless of whether or not there
-          -- is an explicit setup build info or not. See planPackages.
-          cabalConstraint   = orLaterVersion cabalCompatMinVer
-                                `intersectVersionRanges`
-                              orLaterVersion (PD.specVersion pkg)
+          cabalConstraint   = orLaterVersion (PD.specVersion pkg)
                                 `intersectVersionRanges`
                               earlierVersion cabalCompatMaxVer
           -- The idea here is that at some point we will make significant
           -- breaking changes to the Cabal API that Setup.hs scripts use.
           -- So for old custom Setup scripts that do not specify explicit
           -- constraints, we constrain them to use a compatible Cabal version.
           cabalCompatMaxVer = mkVersion [1,25]
-          -- In principle we can talk to any old Cabal version, and we need to
-          -- be able to do that for custom Setup scripts that require older
-          -- Cabal lib versions. However in practice we have currently have
-          -- problems with Cabal-1.16. (1.16 does not know about build targets)
-          -- If this is fixed we can relax this constraint.
-          cabalCompatMinVer = mkVersion [1,18]
 
       -- For other build types (like Simple) if we still need to compile an
       -- external Setup.hs, it'll be one of the simple ones that only depends
@@ -2458,7 +2448,7 @@ defaultSetupDeps compiler platform pkg =
       SetupNonCustomInternalLib -> Just []
 
       -- This case gets ruled out by the caller, planPackages, see the note
-      -- above in the SetupCustomIplicitDeps case.
+      -- above in the SetupCustomImplicitDeps case.
       SetupCustomExplicitDeps ->
         error $ "defaultSetupDeps: called for a package with explicit "
              ++ "setup deps: " ++ display (packageId pkg)

cabal-install/Distribution/Solver/Modular/Package.hs

@@ -12,6 +12,7 @@ module Distribution.Solver.Modular.Package
   , instI
   , makeIndependent
   , primaryPP
+  , setupPP
   , showI
   , showPI
   , unPN
@@ -92,6 +93,14 @@ primaryPP (PackagePath _ns q) = go q
     go (Setup _)   = False
     go (Exe _ _)   = False
 
+-- | Is the package a dependency of a setup script. This is used establish
+-- whether or not certain constraints should apply to this dependency
+-- (grep 'setupPP' to see the use sites).
+--
+setupPP :: PackagePath -> Bool
+setupPP (PackagePath _ns (Setup _)) = True
+setupPP (PackagePath _ns _)         = False
+
 -- | Create artificial parents for each of the package names, making
 -- them all independent.
 makeIndependent :: [PN] -> [QPN]

cabal-install/Distribution/Solver/Modular/Preference.hs

@@ -156,6 +156,9 @@ processPackageConstraintP pp _ _ (LabeledPackageConstraint _ src) r
   | src == ConstraintSourceUserTarget && not (primaryPP pp)         = r
     -- the constraints arising from targets, like "foo-1.0" only apply to
     -- the main packages in the solution, they don't constrain setup deps
+  | src == ConstraintSetupCabalMinVersion && not (setupPP pp)       = r
+    -- the internal constraints on the Setup.hs CLI version don't apply to
+    -- the main packages in the solution, they only constrain setup deps
 
 processPackageConstraintP _ c i (LabeledPackageConstraint pc src) r = go i pc
   where

cabal-install/Distribution/Solver/Types/ConstraintSource.hs

@@ -46,8 +46,9 @@ data ConstraintSource =
   -- | The source of the constraint is not specified.
   | ConstraintSourceUnknown
 
-  -- | Custom setup requires a minimum lower bound on Cabal
-  | ConstraintNewBuildCustomSetupLowerBoundCabal
+  -- | An internal constraint due to compatability issues with the Setup.hs
+  -- command line interface requires a minimum lower bound on Cabal
+  | ConstraintSetupCabalMinVersion
   deriving (Eq, Show, Generic)
 
 instance Binary ConstraintSource
@@ -71,4 +72,5 @@ showConstraintSource ConstraintSourceFreeze = "cabal freeze"
 showConstraintSource ConstraintSourceConfigFlagOrTarget =
     "config file, command line flag, or user target"
 showConstraintSource ConstraintSourceUnknown = "unknown source"
-showConstraintSource ConstraintNewBuildCustomSetupLowerBoundCabal = "new-build's support of Custom Setup (issue #3932)"
+showConstraintSource ConstraintSetupCabalMinVersion =
+    "minimum version of Cabal used by Setup.hs"

cabal-testsuite/PackageTests/Regression/T3932/cabal.test.hs

@@ -7,4 +7,4 @@ main = cabalTest $
     -- won't be reported.
     withRepo "repo" $ do
         fails (cabal' "new-build" []) >>=
-            assertOutputContains "(issue #3932) requires >=1.20"
+            assertOutputContains "Setup.hs requires >=1.20"