From f85aaf7cbd1edded428d780f3ecb26d831c37094 Mon Sep 17 00:00:00 2001
From: Ian Butterworth <i.r.butterworth@gmail.com>
Date: Fri, 16 Aug 2024 14:09:28 +0100
Subject: [PATCH] error handling improvements

---
 src/Operations.jl      | 14 +++++++-------
 src/PlatformEngines.jl | 25 ++++++++++++++++++-------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/src/Operations.jl b/src/Operations.jl
index 128aa89575..5afee37148 100644
--- a/src/Operations.jl
+++ b/src/Operations.jl
@@ -836,7 +836,7 @@ function download_artifacts(ctx::Context;
     end
     push!(pkg_roots, dirname(env.project_file))
     used_artifact_tomls = Set{String}()
-    download_jobs = Channel{Function}(Inf)
+    download_jobs = Function[]
 
     print_lock = Base.ReentrantLock() # for non-fancyprint printing
 
@@ -886,7 +886,6 @@ function download_artifacts(ctx::Context;
             push!(used_artifact_tomls, artifacts_toml)
         end
     end
-    close(download_jobs)
 
     if !isempty(download_jobs)
         longest_name = maximum(textwidth, keys(download_states))
@@ -909,7 +908,7 @@ function download_artifacts(ctx::Context;
                             n_printed = 1
                             show_progress(iostr, main_bar; carriagereturn=false)
                             println(iostr)
-                            for (name, dstate) in sort!(collect(download_states), by=kv->kv[2][2].max, rev=true)
+                            for (name, dstate) in sort!(collect(download_states), by=kv->kv[2].bar.max, rev=true)
                                 dstate.state == :running && dstate.bar.max > 1000 && dstate.bar.current > 0 || continue
                                 show_progress(iostr, dstate.bar; carriagereturn=false)
                                 println(iostr)
@@ -932,8 +931,9 @@ function download_artifacts(ctx::Context;
                 end
             end
             Base.errormonitor(t_print)
+        else
+            printpkgstyle(io, :Downloading, "$(length(download_jobs)) artifacts")
         end
-
         sema = Base.Semaphore(ctx.num_concurrent_downloads)
         interrupted = false
         @sync for f in download_jobs
@@ -949,7 +949,7 @@ function download_artifacts(ctx::Context;
             end
         end
         is_done = true
-        wait(t_print)
+        fancyprint && wait(t_print)
         close(errors)
 
         if !isempty(errors)
@@ -957,10 +957,10 @@ function download_artifacts(ctx::Context;
             str = sprint(context=io) do iostr
                 for e in all_errors
                     Base.showerror(iostr, e)
-                    length(all_errors) > 1 && print(io, "\n----------------\n")
+                    length(all_errors) > 1 && println(iostr)
                 end
             end
-            pkgerror("Failed to download some artifacts:\n\n$str")
+            pkgerror("Failed to download some artifacts:\n\n$(strip(str, '\n'))")
         end
     end
 
diff --git a/src/PlatformEngines.jl b/src/PlatformEngines.jl
index 287572c0be..e15b6483bf 100644
--- a/src/PlatformEngines.jl
+++ b/src/PlatformEngines.jl
@@ -368,7 +368,8 @@ function download_verify(
             end
         end
     end
-    if hash !== nothing && !verify(dest, hash; verbose=verbose)
+    details = String[]
+    if hash !== nothing && !verify(dest, hash; verbose, details)
         # If the file already existed, it's possible the initially downloaded chunk
         # was bad.  If verification fails after downloading, auto-delete the file
         # and start over from scratch.
@@ -380,13 +381,15 @@ function download_verify(
 
             # Download and verify from scratch
             download(url, dest; verbose=verbose || !quiet_download)
-            if hash !== nothing && !verify(dest, hash; verbose=verbose)
-                error("Verification failed. Download does not match expected hash")
+            if hash !== nothing && !verify(dest, hash; verbose, details)
+                @goto verification_failed
             end
         else
+            @label verification_failed
             # If it didn't verify properly and we didn't resume, something is
             # very wrong and we must complain mightily.
-            error("Verification failed. Download does not match expected hash")
+            details_indented = join(map(s -> "      $s", split(join(details, "\n"), '\n')), "\n")
+            error("Verification failed:\n" * details_indented)
         end
     end
 
@@ -555,7 +558,8 @@ end
 
 """
     verify(path::AbstractString, hash::AbstractString;
-           verbose::Bool = false, report_cache_status::Bool = false)
+           verbose::Bool = false, report_cache_status::Bool = false,
+           details::Union{Vector{String},Nothing} = nothing)
 
 Given a file `path` and a `hash`, calculate the SHA256 of the file and compare
 it to `hash`.  This method caches verification results in a `"\$(path).sha256"`
@@ -571,9 +575,12 @@ If `report_cache_status` is set to `true`, then the return value will be a
 `Symbol` giving a granular status report on the state of the hash cache, in
 addition to the `true`/`false` signifying whether verification completed
 successfully.
+
+If `details` is provided, any pertinent detail will be pushed to it rather than logged.
 """
 function verify(path::AbstractString, hash::AbstractString; verbose::Bool = false,
-                report_cache_status::Bool = false, hash_path::AbstractString="$(path).sha256")
+                report_cache_status::Bool = false, hash_path::AbstractString="$(path).sha256",
+                details::Union{Vector{String},Nothing} = nothing)
 
     # Check hash string format
     if !occursin(r"^[0-9a-f]{64}$"i, hash)
@@ -643,7 +650,11 @@ function verify(path::AbstractString, hash::AbstractString; verbose::Bool = fals
         msg  = "Hash Mismatch!\n"
         msg *= "  Expected sha256:   $hash\n"
         msg *= "  Calculated sha256: $calc_hash"
-        @error(msg)
+        if isnothing(details)
+            @error(msg)
+        else
+            push!(details, msg)
+        end
         if report_cache_status
             return false, :hash_mismatch
         else