Merge pull request #10731 from mpickering/wip/t10692

cabal-install: Be less eager to configure external programs

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

@@ -484,6 +484,7 @@ configureCompiler
     let fileMonitorCompiler = newFileMonitor $ distProjectCacheFile "compiler"
 
     progsearchpath <- liftIO $ getSystemSearchPath
+
     rerunIfChanged
       verbosity
       fileMonitorCompiler
@@ -499,7 +500,7 @@ configureCompiler
         let extraPath = fromNubList packageConfigProgramPathExtra
         progdb <- liftIO $ prependProgramSearchPath verbosity extraPath [] defaultProgramDb
         let progdb' = userSpecifyPaths (Map.toList (getMapLast packageConfigProgramPaths)) progdb
-        (comp, plat, progdb'') <-
+        result@(_, _, progdb'') <-
           liftIO $
             Cabal.configCompilerEx
               hcFlavor
@@ -516,17 +517,55 @@ configureCompiler
         -- programs it cares about, and those are the ones we monitor here.
         monitorFiles (programsMonitorFiles progdb'')
 
-        -- Configure the unconfigured programs in the program database,
-        -- as we can't serialise unconfigured programs.
-        -- See also #2241 and #9840.
-        finalProgDb <- liftIO $ configureAllKnownPrograms verbosity progdb''
+        -- Note: There is currently a bug here: we are dropping unconfigured
+        -- programs from the 'ProgramDb' when we re-use the cache created by
+        -- 'rerunIfChanged'.
+        --
+        -- See Note [Caching the result of configuring the compiler]
 
-        return (comp, plat, finalProgDb)
+        return result
     where
       hcFlavor = flagToMaybe projectConfigHcFlavor
       hcPath = flagToMaybe projectConfigHcPath
       hcPkg = flagToMaybe projectConfigHcPkg
 
+{- Note [Caching the result of configuring the compiler]
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+We can't straightforwardly cache anything that contains a 'ProgramDb', because
+the 'Binary' instance for 'ProgramDb' discards all unconfigured programs.
+See that instance, as well as 'restoreProgramDb', for a few more details.
+
+This means that if we try to cache the result of configuring the compiler (which
+contains a 'ProgramDb'):
+
+ - On the first run, we will obtain a 'ProgramDb' which may contain several
+   unconfigured programs. In particular, configuring GHC will add tools such
+   as `ar` and `ld` as unconfigured programs to the 'ProgramDb', with custom
+   logic for finding their location based on the location of the GHC binary.
+ - On subsequent runs, if we use the cache created by 'rerunIfChanged', we will
+   deserialise the 'ProgramDb' from disk, which means it won't include any
+   unconfigured programs, which might mean we are unable to find 'ar' or 'ld'.
+
+This is not currently a huge problem because, in the Cabal library, we eagerly
+re-run the configureCompiler step (thus recovering any lost information), but
+this is wasted work that we should stop doing in Cabal, given that cabal-install
+has already figured out all the necessary information about the compiler.
+
+To fix this bug, we can't simply eagerly configure all unconfigured programs,
+as was originally attempted, for a couple of reasons:
+
+ - it does more work than necessary, by configuring programs that we may not
+   end up needing,
+ - it means that we prioritise system executables for built-in build tools
+   (such as `alex` and `happy`), instead of using the proper version for a
+   package or package component, as specified by a `build-tool-depends` stanza
+   or by package-level `extra-prog-path` arguments.
+   This lead to bug reports #10633 and #10692.
+
+See #9840 for more information about the problems surrounding the lossly
+Binary ProgramDb instance.
+-}
+
 ------------------------------------------------------------------------------
 
 -- * Deciding what to do: making an 'ElaboratedInstallPlan'

cabal-testsuite/PackageTests/BuildToolDependsExternal/cabal.project

@@ -0,0 +1,2 @@
+packages: client
+optional-packages: pre-proc

cabal-testsuite/PackageTests/BuildToolDependsExternal/client/Hello.x

@@ -0,0 +1,3 @@
+module Main where
+
+main = print 0

cabal-testsuite/PackageTests/BuildToolDependsExternal/client/client.cabal

@@ -0,0 +1,13 @@
+name:                client
+version:             0.1.0.0
+synopsis:            Checks build-tool-depends are put in PATH
+license:             BSD3
+category:            Testing
+build-type:          Simple
+cabal-version:       >=1.10
+
+executable             hello-world
+  main-is:             Hello.hs
+  build-depends:       base
+  build-tool-depends:  pre-proc:alex
+  default-language:    Haskell2010

cabal-testsuite/PackageTests/BuildToolDependsExternal/pre-proc/MyCustomPreprocessor.hs

@@ -0,0 +1,13 @@
+module Main where
+
+import System.Environment
+import System.IO
+
+-- This is a "fake" version of alex, so it should take the command line arguments
+-- as alex.
+main :: IO ()
+main = do
+  (_:"-o":target:source:_) <- getArgs
+  let f '0' = '1'
+      f c = c
+  writeFile target . map f  =<< readFile source

cabal-testsuite/PackageTests/BuildToolDependsExternal/pre-proc/pre-proc.cabal

@@ -0,0 +1,17 @@
+name:                pre-proc
+version:             999.999.999
+synopsis:            Checks build-tool-depends are put in PATH
+license:             BSD3
+category:            Testing
+build-type:          Simple
+cabal-version:       >=1.10
+
+executable             alex
+  main-is:             MyCustomPreprocessor.hs
+  build-depends:       base, directory
+  default-language:    Haskell2010
+
+executable             bad-do-not-build-me
+  main-is:             MyMissingPreprocessor.hs
+  build-depends:       base, directory
+  default-language:    Haskell2010

cabal-testsuite/PackageTests/BuildToolDependsExternal/scripts/alex

@@ -0,0 +1,4 @@
+#! /usr/bin/env bash
+
+echo "I am not the alex you are looking for"
+exit 1

cabal-testsuite/PackageTests/BuildToolDependsExternal/setup.out

@@ -0,0 +1,14 @@
+# cabal v2-build
+Configuration is affected by the following files:
+- cabal.project
+Resolving dependencies...
+Build profile: -w ghc-<GHCVER> -O1
+In order, the following will be built:
+ - pre-proc-999.999.999 (exe:alex) (first run)
+ - client-0.1.0.0 (exe:hello-world) (first run)
+Configuring executable 'alex' for pre-proc-999.999.999...
+Preprocessing executable 'alex' for pre-proc-999.999.999...
+Building executable 'alex' for pre-proc-999.999.999...
+Configuring executable 'hello-world' for client-0.1.0.0...
+Preprocessing executable 'hello-world' for client-0.1.0.0...
+Building executable 'hello-world' for client-0.1.0.0...

cabal-testsuite/PackageTests/BuildToolDependsExternal/setup.test.hs

@@ -0,0 +1,5 @@
+import Test.Cabal.Prelude
+-- Test build-tool-depends isn't influenced by PATH
+main = cabalTest $ do
+    env <- getTestEnv
+    addToPath (testTmpDir env </> "scripts/") $ cabal "v2-build" ["client"]

cabal-testsuite/PackageTests/ExtraProgPath/setup.out

@@ -1,8 +1,10 @@
 # cabal v2-build
-Warning: cannot determine version of <ROOT>/pkg-config :
-""
 Configuration is affected by the following files:
 - cabal.project
+Warning: cannot determine version of <ROOT>/pkg-config :
+""
+Warning: cannot determine version of <ROOT>/pkg-config :
+""
 Resolving dependencies...
 Error: [Cabal-7107]
 Could not resolve dependencies:

cabal-testsuite/PackageTests/ExtraProgPathLocal/cabal.project

@@ -0,0 +1 @@
+packages: client

cabal-testsuite/PackageTests/ExtraProgPathLocal/client/Hello.x

@@ -0,0 +1,3 @@
+This file is deliberately an invalid .x file,
+to ensure that we pick up the local alex script rather than
+any system-wide alex executable.

cabal-testsuite/PackageTests/ExtraProgPathLocal/client/client.cabal

@@ -0,0 +1,12 @@
+name:                client
+version:             0.1.0.0
+synopsis:            Checks build-tool-depends are put in PATH
+license:             BSD3
+category:            Testing
+build-type:          Simple
+cabal-version:       >=1.10
+
+executable             hello-world
+  main-is:             Hello.hs
+  build-depends:       base
+  default-language:    Haskell2010

cabal-testsuite/PackageTests/ExtraProgPathLocal/scripts/alex

@@ -0,0 +1,3 @@
+#! /usr/bin/env bash
+
+echo "I am not the alex you are looking for"

cabal-testsuite/PackageTests/ExtraProgPathLocal/scripts/alex.shim

@@ -0,0 +1,2 @@
+path = "FINDSH/sh.exe"
+args = "SCRIPTSDIR/alex"

cabal-testsuite/PackageTests/ExtraProgPathLocal/scripts2/alex

@@ -0,0 +1,4 @@
+#! /usr/bin/env bash
+
+echo "I am the alex you are looking for"
+echo "module Main where main = print ()" > $3

cabal-testsuite/PackageTests/ExtraProgPathLocal/scripts2/alex.shim

@@ -0,0 +1,2 @@
+path = "FINDSH/sh.exe"
+args = "SCRIPTS2DIR/alex"

cabal-testsuite/PackageTests/ExtraProgPathLocal/setup.out

@@ -0,0 +1,10 @@
+# cabal v2-build
+Configuration is affected by the following files:
+- cabal.project
+Resolving dependencies...
+Build profile: -w ghc-<GHCVER> -O1
+In order, the following will be built:
+ - client-0.1.0.0 (exe:hello-world) (first run)
+Configuring executable 'hello-world' for client-0.1.0.0...
+Preprocessing executable 'hello-world' for client-0.1.0.0...
+Building executable 'hello-world' for client-0.1.0.0...

cabal-testsuite/PackageTests/ExtraProgPathLocal/setup.test.hs

@@ -0,0 +1,39 @@
+import Test.Cabal.Prelude
+import System.Directory
+
+-- Test package-local extra-prog-path works.
+main = cabalTest $ do
+    env <- getTestEnv
+    let
+      testDir = testCurrentDir env
+      tmpDir  = testTmpDir     env
+      scripts1 = tmpDir </> "scripts"
+      scripts2 = tmpDir </> "scripts2"
+
+    -------------------------
+    -- Workaround for the fact that, on Windows, Cabal will only look for
+    -- .exe files to satisfy executable dependencs. So we have to create
+    -- shim alex.exe files (the good one in 'scripts2', the bad one in 'scripts')
+    -- with the logic below.
+    when isWindows $ do
+      mb_sh <- fmap takeDirectory <$> liftIO (findExecutable "sh")
+      case mb_sh of
+          Nothing -> skip "no sh"
+          Just sh -> do
+            let escape = concatMap (\c -> case c of '\\' -> "\\\\\\\\"; x -> [x])
+            void $ shell "sed" [ "-i", "-e", "s/FINDSH/" <> escape sh <> "/g", escape (scripts1 </> "alex.shim"), escape (scripts2 </> "alex.shim") ]
+            void $ shell "sed" [ "-i", "-e", "s/SCRIPTSDIR/" <> escape scripts1 <> "/g", escape (scripts1 </> "alex.shim") ]
+            void $ shell "sed" [ "-i", "-e", "s/SCRIPTS2DIR/" <> escape scripts2 <> "/g", escape (scripts2 </> "alex.shim") ]
+
+    -- End of Windows workarounds
+    ------------------------------
+
+    -- Add the 'scripts' directory to PATH, and add the 'scripts2' directory
+    -- to extra-prog-path.
+    --
+    -- This checks that the executables in extra-prog-path take priority over
+    -- those in PATH: 'scripts/alex' will fail, while 'scripts2/alex' will succeed.
+
+    liftIO $ appendFile (testDir </> "cabal.project") $
+      "\npackage client\n  extra-prog-path:" ++ scripts2
+    addToPath scripts1 $ cabal "v2-build" ["client"]

changelog.d/pr-10731

@@ -0,0 +1,4 @@
+synopsis: Fix regression where build-tool-depends are not used
+packages: cabal-install
+prs: #10731
+issues: #10633 #10692