external commands: Wait for process to finish before exiting (#10100)

Issue #10063 points out that cabal exits before the external command
has finished executing. This was a simple oversight to not
waitForProcess on the result of calling createProcess.

This also points out the flaw that there isn't a way for external
commands to signal failure, so we now also propagate the exit code from
the external process.

Fixes #10063

(cherry picked from commit cb57c37)

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

@@ -270,7 +270,7 @@ import System.IO
   , stderr
   , stdout
   )
-import System.Process (createProcess, env, proc)
+import System.Process (createProcess, env, proc, waitForProcess)
 
 -- | Entry point
 --
@@ -378,7 +378,7 @@ mainWorker args = do
       result <- try $ createProcess ((proc exec (name : cmdArgs)){env = Just new_env})
       case result of
         Left ex -> printErrors ["Error executing external command: " ++ show (ex :: SomeException)]
-        Right _ -> return ()
+        Right (_, _, _, ph) -> waitForProcess ph >>= exitWith
 
     printCommandHelp help = do
       pname <- getProgName

cabal-testsuite/PackageTests/ExternalCommandExitCode/cabal.out

@@ -0,0 +1,8 @@
+# cabal v2-build
+Resolving dependencies...
+Build profile: -w ghc-<GHCVER> -O1
+In order, the following will be built:
+ - setup-test-0.1.0.0 (exe:cabal-aaaa) (first run)
+Configuring executable 'cabal-aaaa' for setup-test-0.1.0.0...
+Preprocessing executable 'cabal-aaaa' for setup-test-0.1.0.0...
+Building executable 'cabal-aaaa' for setup-test-0.1.0.0...

cabal-testsuite/PackageTests/ExternalCommandExitCode/cabal.project

@@ -0,0 +1 @@
+packages: setup-test/

cabal-testsuite/PackageTests/ExternalCommandExitCode/cabal.test.hs

@@ -0,0 +1,38 @@
+import Test.Cabal.Prelude
+import qualified System.Process as Process
+import Control.Concurrent (threadDelay)
+import System.Directory (removeFile)
+import Control.Exception (catch, throwIO)
+import System.IO.Error (isDoesNotExistError)
+import qualified Data.Time.Clock as Time
+import qualified Data.Time.Format as Time
+import Data.Maybe
+import System.Environment
+import System.FilePath
+import System.Exit
+
+main = do
+  cabalTest $ do
+    res <- cabalWithStdin "v2-build" ["all"] ""
+    exe_path <- withPlan $ planExePath "setup-test" "cabal-aaaa"
+    addToPath (takeDirectory exe_path) $ do
+      -- Test that the thing works at all
+      res <- fails $ cabal_raw_action ["aaaa"] (\h -> () <$ Process.waitForProcess h)
+      assertOutputContains "aaaa" res
+      -- Check the exit code is the one returned by subcommand
+      unless (resultExitCode res == ExitFailure 99) (assertFailure $ "Incorrect exit code: " ++ show (resultExitCode res))
+
+
+cabal_raw_action :: [String] -> (Process.ProcessHandle -> IO ()) -> TestM Result
+cabal_raw_action args action = do
+    configured_prog <- requireProgramM cabalProgram
+    env <- getTestEnv
+    r <- liftIO $ runAction (testVerbosity env)
+                 (Just $ testCurrentDir env)
+                 (testEnvironment env)
+                 (programPath configured_prog)
+                 args
+                 Nothing
+                 action
+    recordLog r
+    requireSuccess r

cabal-testsuite/PackageTests/ExternalCommandExitCode/setup-test/AAAA.hs

@@ -0,0 +1,8 @@
+module Main where
+
+import System.Environment
+import System.Exit
+
+main = do
+  getArgs >>= print
+  exitWith (ExitFailure 99)

cabal-testsuite/PackageTests/ExternalCommandExitCode/setup-test/CHANGELOG.md

@@ -0,0 +1,5 @@
+# Revision history for setup-test
+
+## 0.1.0.0 -- YYYY-mm-dd
+
+* First version. Released on an unsuspecting world.

cabal-testsuite/PackageTests/ExternalCommandExitCode/setup-test/LICENSE

@@ -0,0 +1,30 @@
+Copyright (c) 2023, Matthew Pickering
+
+All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are met:
+
+    * Redistributions of source code must retain the above copyright
+      notice, this list of conditions and the following disclaimer.
+
+    * Redistributions in binary form must reproduce the above
+      copyright notice, this list of conditions and the following
+      disclaimer in the documentation and/or other materials provided
+      with the distribution.
+
+    * Neither the name of Matthew Pickering nor the names of other
+      contributors may be used to endorse or promote products derived
+      from this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

cabal-testsuite/PackageTests/ExternalCommandExitCode/setup-test/setup-test.cabal

@@ -0,0 +1,25 @@
+cabal-version:      3.0
+name:               setup-test
+version:            0.1.0.0
+-- synopsis:
+-- description:
+license:            BSD-3-Clause
+license-file:       LICENSE
+author:             Matthew Pickering
+maintainer:         [email protected]
+-- copyright:
+build-type:         Simple
+extra-doc-files:    CHANGELOG.md
+-- extra-source-files:
+
+common warnings
+    ghc-options: -Wall
+
+executable cabal-aaaa
+    import:           warnings
+    main-is: AAAA.hs
+    -- other-modules:
+    -- other-extensions:
+    build-depends:    base
+    hs-source-dirs:   .
+    default-language: Haskell2010

changelog.d/issue-10063

@@ -0,0 +1,14 @@
+synopsis: External commands now propagate exit code from child process
+packages: cabal-install
+prs: #10100
+issues: #10063
+
+description: {
+  The exit code from an external command is now propagated as the exit code of
+  cabal-install when invoked by calling an external command.
+
+  For example, if your external command exits with code 1, then cabal-install will
+  also exit with code 1.
+
+  This mechanism can be used by an external command to signal failure.
+}

doc/external-commands.rst

@@ -4,7 +4,8 @@ External Commands
 ``cabal-install`` provides a system for external commands, akin to the ones used by tools like ``git`` or ``cargo``.
 
 If you execute ``cabal <cmd>``, ``cabal-install`` will search the path for an executable named ``cabal-<cmd>`` and execute it. The name of the command is passed as the first argument and
-the remaining arguments are passed afterwards. An error will be thrown in case the custom command is not found.
+the remaining arguments are passed afterwards. An error will be thrown in case the custom command is not found. The exit code of cabal when calling an external command is the same as the exit code
+of the command.
 
 The ``$CABAL`` environment variable is set to the path of the ``cabal-install`` executable
 which invoked the subcommand.