Fix precedence for PATH for build-tools-depends (#8972)

* Fix precedence for PATH for build-tools-depends

* changelog

---------

Co-authored-by: Hécate Moonlight <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Author: 3 people

Cabal/src/Distribution/Simple/Configure.hs

@@ -1028,10 +1028,17 @@ mkProgramDb cfg initialProgramDb = programDb
         . setProgramSearchPath searchpath
         $ initialProgramDb
     searchpath =
-      map
-        ProgramSearchPathDir
-        (fromNubList $ configProgramPathExtra cfg)
-        ++ getProgramSearchPath initialProgramDb
+      getProgramSearchPath initialProgramDb
+        ++ map
+          ProgramSearchPathDir
+          (fromNubList $ configProgramPathExtra cfg)
+
+-- Note. We try as much as possible to _prepend_ rather than postpend the extra-prog-path
+-- so that we can override the system path. However, in a v2-build, at this point, the "system" path
+-- has already been extended by both the built-tools-depends paths, as well as the program-path-extra
+-- so for v2 builds adding it again is entirely unnecessary. However, it needs to get added again _anyway_
+-- so as to take effect for v1 builds or standalone calls to Setup.hs
+-- In this instance, the lesser evil is to not allow it to override the system path.
 
 -- -----------------------------------------------------------------------------
 -- Helper functions for configure

cabal-install/src/Distribution/Client/Check.hs

@@ -99,34 +99,35 @@ check verbosity = do
 
 -- Poor man’s “group checks by constructor”.
 groupChecks :: [PackageCheck] -> [NE.NonEmpty PackageCheck]
-groupChecks ds = NE.groupBy (F.on (==) constInt)
-                            (L.sortBy (F.on compare constInt) ds)
-    where
-          constInt :: PackageCheck -> Int
-          constInt (PackageBuildImpossible {}) = 0
-          constInt (PackageBuildWarning {}) = 1
-          constInt (PackageDistSuspicious {}) = 2
-          constInt (PackageDistSuspiciousWarn {}) = 3
-          constInt (PackageDistInexcusable {}) = 4
+groupChecks ds =
+  NE.groupBy
+    (F.on (==) constInt)
+    (L.sortBy (F.on compare constInt) ds)
+  where
+    constInt :: PackageCheck -> Int
+    constInt (PackageBuildImpossible{}) = 0
+    constInt (PackageBuildWarning{}) = 1
+    constInt (PackageDistSuspicious{}) = 2
+    constInt (PackageDistSuspiciousWarn{}) = 3
+    constInt (PackageDistInexcusable{}) = 4
 
 groupExplanation :: PackageCheck -> String
-groupExplanation (PackageBuildImpossible {}) = "The package will not build sanely due to these errors:"
-groupExplanation (PackageBuildWarning {}) = "The following errors are likely to affect your build negatively:"
-groupExplanation (PackageDistSuspicious {}) = "These warnings will likely cause trouble when distributing the package:"
-groupExplanation (PackageDistSuspiciousWarn {}) = "These warnings may cause trouble when distributing the package:"
-groupExplanation (PackageDistInexcusable {}) = "The following errors will cause portability problems on other environments:"
+groupExplanation (PackageBuildImpossible{}) = "The package will not build sanely due to these errors:"
+groupExplanation (PackageBuildWarning{}) = "The following errors are likely to affect your build negatively:"
+groupExplanation (PackageDistSuspicious{}) = "These warnings will likely cause trouble when distributing the package:"
+groupExplanation (PackageDistSuspiciousWarn{}) = "These warnings may cause trouble when distributing the package:"
+groupExplanation (PackageDistInexcusable{}) = "The following errors will cause portability problems on other environments:"
 
 groupOutputFunction :: PackageCheck -> Verbosity -> String -> IO ()
-groupOutputFunction (PackageBuildImpossible {}) ver = warnError ver
-groupOutputFunction (PackageBuildWarning {}) ver = warnError ver
-groupOutputFunction (PackageDistSuspicious {}) ver = warn ver
-groupOutputFunction (PackageDistSuspiciousWarn {}) ver = warn ver
-groupOutputFunction (PackageDistInexcusable {}) ver = warnError ver
+groupOutputFunction (PackageBuildImpossible{}) ver = warnError ver
+groupOutputFunction (PackageBuildWarning{}) ver = warnError ver
+groupOutputFunction (PackageDistSuspicious{}) ver = warn ver
+groupOutputFunction (PackageDistSuspiciousWarn{}) ver = warn ver
+groupOutputFunction (PackageDistInexcusable{}) ver = warnError ver
 
 outputGroupCheck :: Verbosity -> NE.NonEmpty PackageCheck -> IO ()
 outputGroupCheck ver pcs = do
-          let hp = NE.head pcs
-              outf = groupOutputFunction hp ver
-          notice ver (groupExplanation hp)
-          CM.mapM_ (outf . ppPackageCheck) pcs
-
+  let hp = NE.head pcs
+      outf = groupOutputFunction hp ver
+  notice ver (groupExplanation hp)
+  CM.mapM_ (outf . ppPackageCheck) pcs

cabal-install/src/Distribution/Client/ProjectPlanning.hs

@@ -347,7 +347,9 @@ sanityCheckElaboratedPackage
 -- readProjectConfig also loads the global configuration, which is read with
 -- loadConfig and convertd to a ProjectConfig with convertLegacyGlobalConfig.
 --
--- *Important*
+
+-- * Important *
+
 --
 -- You can notice how some project config options are needed to read the
 -- project config! This is evident by the fact that rebuildProjectConfig
@@ -539,9 +541,10 @@ configureCompiler
             )
           $ defaultProgramDb
 
-
 ------------------------------------------------------------------------------
+
 -- * Deciding what to do: making an 'ElaboratedInstallPlan'
+
 ------------------------------------------------------------------------------
 
 -- | Return an up-to-date elaborated install plan.
@@ -4009,8 +4012,11 @@ setupHsScriptOptions
       , useDistPref = builddir
       , useLoggingHandle = Nothing -- this gets set later
       , useWorkingDir = Just srcdir
-      , useExtraPathEnv = elabExeDependencyPaths elab
-      , useExtraEnvOverrides = dataDirsEnvironmentForPlan distdir plan
+      , useExtraPathEnv = elabExeDependencyPaths elab ++ elabProgramPathExtra
+      , -- note that the above adds the extra-prog-path directly following the elaborated
+        -- dep paths, so that it overrides the normal path, but _not_ the elaborated extensions
+        -- for build-tools-depends.
+        useExtraEnvOverrides = dataDirsEnvironmentForPlan distdir plan
       , useWin32CleanHack = False -- TODO: [required eventually]
       , forceExternalSetupMethod = isParallelBuild
       , setupCacheLock = Just cacheLock

cabal-install/tests/UnitTests/Distribution/Client/ArbitraryInstances.hs

@@ -45,9 +45,24 @@ import Distribution.Solver.Types.PackageConstraint (PackageProperty (..))
 
 import Data.Coerce (Coercible, coerce)
 import Network.URI (URI (..), URIAuth (..), isUnreserved)
-import Test.QuickCheck (Arbitrary(..), Gen, NonEmptyList(..),
-                        arbitraryBoundedEnum, choose, elements, frequency, genericShrink,
-                        liftArbitrary, listOf, oneof, resize, sized, shrinkBoundedEnum, suchThat, vectorOf)
+import Test.QuickCheck
+  ( Arbitrary (..)
+  , Gen
+  , NonEmptyList (..)
+  , arbitraryBoundedEnum
+  , choose
+  , elements
+  , frequency
+  , genericShrink
+  , liftArbitrary
+  , listOf
+  , oneof
+  , resize
+  , shrinkBoundedEnum
+  , sized
+  , suchThat
+  , vectorOf
+  )
 import Test.QuickCheck.GenericArbitrary (genericArbitrary)
 import Test.QuickCheck.Instances.Cabal ()
 

changelog.d/pr-8972

@@ -0,0 +1,9 @@
+synopsis: Fix precedence for PATH for build-tools-depends
+packages: Cabal cabal-install
+prs: #8972
+
+description: {
+
+- fixes a bug introduced in #8506 that caused executables in the path to take precedence over those specified in build-tools-depends.
+
+}