Add ProcessExitedError rather than using error (#27900)

NEWS.md

@@ -20,13 +20,18 @@ New language features
 Multi-threading changes
 -----------------------
 
-  * The `Condition` type now has a thread-safe replacement, accessed as `Threads.Condition`.
+* The `Condition` type now has a thread-safe replacement, accessed as `Threads.Condition`.
     With that addition, task scheduling primitives such as `ReentrantLock` are now thread-safe ([#30061]).
 
 Language changes
 ----------------
 * Empty entries in `JULIA_DEPOT_PATH` are now expanded to default depot entries ([#31009]).
 * `Enum` now behaves like a scalar when used in broadcasting ([#30670]).
+* If a `pipeline` is specified with `append=true` set, but no redirection, an `ArgumentError`
+is thrown, rather than a `ErrorException` ([#27900]).
+* Functions that invoke commands (e.g. `run(::Cmd)`) now throw a `ProcessFailedException`
+rather than an `ErrorException`, if those commands exit with non-zero exit code.
+([#27900]).
 
 Command-line option changes
 ---------------------------

base/download.jl

@@ -15,7 +15,7 @@ if Sys.iswindows()
             if proc.exitcode % Int32 == -393216
                 # appears to be "wrong version" exit code, based on
                 # https://docs.microsoft.com/en-us/azure/cloud-services/cloud-services-startup-tasks-common
-                error("Downloading files requires Windows Management Framework 3.0 or later.")
+                @error "Downloading files requires Windows Management Framework 3.0 or later."
             end
             pipeline_error(proc)
         end
@@ -39,8 +39,9 @@ function download_curl(curl_exe::AbstractString, url::AbstractString, filename::
     err = PipeBuffer()
     process = run(pipeline(`$curl_exe -s -S -g -L -f -o $filename $url`, stderr=err), wait=false)
     if !success(process)
-        stderr = readline(err)
-        error(stderr)
+        error_msg = readline(err)
+        @error "Download Failed" error_msg
+        pipeline_error(process)
     end
     return filename
 end

base/exports.jl

@@ -117,13 +117,14 @@ export
     Cwstring,
 
 # Exceptions
-    DimensionMismatch,
     CapturedException,
     CompositeException,
+    DimensionMismatch,
     EOFError,
     InvalidStateException,
     KeyError,
     MissingException,
+    ProcessFailedException,
     SystemError,
     StringIndexError,
 

base/process.jl

@@ -268,7 +268,7 @@ run(pipeline(`update`, stdout="log.txt", append=true))
 """
 function pipeline(cmd::AbstractCmd; stdin=nothing, stdout=nothing, stderr=nothing, append::Bool=false)
     if append && stdout === nothing && stderr === nothing
-        error("append set to true, but no output redirections specified")
+        throw(ArgumentError("append set to true, but no output redirections specified"))
     end
     if stdin !== nothing
         cmd = redir_out(stdin, cmd)
@@ -783,9 +783,34 @@ An exception is raised if the process cannot be started.
 """
 success(cmd::AbstractCmd) = success(_spawn(cmd))
 
+
+"""
+    ProcessFailedException
+
+Indicates problematic exit status of a process.
+When running commands or pipelines, this is thrown to indicate
+a nonzero exit code was returned (i.e. that the invoked process failed).
+"""
+struct ProcessFailedException <: Exception
+    procs::Vector{Process}
+end
+ProcessFailedException(proc::Process) = ProcessFailedException([proc])
+
+function showerror(io::IO, err::ProcessFailedException)
+    if length(err.procs) == 1
+        proc = err.procs[1]
+        println(io, "failed process: ", proc, " [", proc.exitcode, "]")
+    else
+        println(io, "failed processes:")
+        for proc in err.procs
+            println(io, "  ", proc, " [", proc.exitcode, "]")
+        end
+    end
+end
+
 function pipeline_error(proc::Process)
     if !proc.cmd.ignorestatus
-        error("failed process: ", proc, " [", proc.exitcode, "]")
+        throw(ProcessFailedException(proc))
     end
     nothing
 end
@@ -798,12 +823,7 @@ function pipeline_error(procs::ProcessChain)
         end
     end
     isempty(failed) && return nothing
-    length(failed) == 1 && pipeline_error(failed[1])
-    msg = "failed processes:"
-    for proc in failed
-        msg = string(msg, "\n  ", proc, " [", proc.exitcode, "]")
-    end
-    error(msg)
+    throw(ProcessFailedException(failed))
 end
 
 """

doc/src/base/base.md

@@ -347,6 +347,7 @@ Base.MissingException
 Core.OutOfMemoryError
 Core.ReadOnlyMemoryError
 Core.OverflowError
+Base.ProcessFailedException
 Core.StackOverflowError
 Base.SystemError
 Core.TypeError

test/download.jl

@@ -16,7 +16,7 @@ mktempdir() do temp_dir
 
     # Make sure that failed downloads do not leave files around
     missing_file = joinpath(temp_dir, "missing")
-    @test_throws ErrorException download("http://httpbin.org/status/404", missing_file)
+    @test_throws ProcessFailedException download("http://httpbin.org/status/404", missing_file)
     @test !isfile(missing_file)
 
     # Make sure we properly handle metachar '
@@ -28,6 +28,6 @@ mktempdir() do temp_dir
 
     # Use a TEST-NET (192.0.2.0/24) address which shouldn't be bound
     invalid_host_file = joinpath(temp_dir, "invalid_host")
-    @test_throws ErrorException download("http://192.0.2.1", invalid_host_file)
+    @test_throws ProcessFailedException download("http://192.0.2.1", invalid_host_file)
     @test !isfile(invalid_host_file)
 end

test/spawn.jl

@@ -420,13 +420,23 @@ end
 
 @test Base.shell_split("\"\\\\\"") == ["\\"]
 
-# issue #13616
-pcatcmd = `$catcmd _doesnt_exist__111_`
-let p = eachline(pipeline(`$catcmd _doesnt_exist__111_`, stderr=devnull))
-    @test_throws(ErrorException("failed process: Process($pcatcmd, ProcessExited(1)) [1]"),
-                 collect(p))
+# Test failing commands
+failing_cmd = `$catcmd _doesnt_exist__111_`
+failing_pipeline = pipeline(failing_cmd, stderr=devnull) # make quiet for tests
+for testrun in (failing_pipeline, pipeline(failing_pipeline, failing_pipeline))
+    try
+        run(testrun)
+    catch err
+        @test err isa ProcessFailedException
+        errmsg = sprint(showerror, err)
+        @test occursin(string(failing_cmd), errmsg)
+    end
 end
 
+# issue #13616
+@test_throws(ProcessFailedException, collect(eachline(failing_pipeline)))
+
+
 # make sure windows_verbatim strips quotes
 if Sys.iswindows()
     @test read(`cmd.exe /c dir /b spawn.jl`, String) == read(Cmd(`cmd.exe /c dir /b "\"spawn.jl\""`, windows_verbatim=true), String)