Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add status messages for artifact installation #4005

Merged
merged 5 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Artifacts.jl
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ function download_artifact(
try
download_verify_unpack(tarball_url, tarball_hash, temp_dir;
ignore_existence=true, verbose, quiet_download, io, progress)
isnothing(progress) || progress(10000, 10000; status="verifying")
calc_hash = SHA1(GitTools.tree_hash(temp_dir))

# Did we get what we expected? If not, freak out.
Expand Down Expand Up @@ -368,6 +369,7 @@ function download_artifact(
end
end
# Move it to the location we expected
isnothing(progress) || progress(10000, 10000; status="moving to artifact store")
_mv_temp_artifact_dir(temp_dir, dst)
catch err
@debug "download_artifact error" tree_hash tarball_url tarball_hash err
Expand Down
16 changes: 11 additions & 5 deletions src/MiniProgressBars.jl
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@ function pkg_format_bytes(bytes; binary=true, sigdigits::Integer=3)
end

Base.@kwdef mutable struct MiniProgressBar
max::Int = 1.0
max::Int = 1
header::String = ""
color::Symbol = :nothing
width::Int = 40
current::Int = 0
status::String = "" # If not empty this string replaces the bar
prev::Int = 0
has_shown::Bool = false
time_shown::Float64 = 0.0
Expand Down Expand Up @@ -82,10 +83,15 @@ function show_progress(io::IO, p::MiniProgressBar; termwidth=nothing, carriagere
else
print(io, p.header, " ")
end
printstyled(io, "━"^n_filled; color=p.color)
printstyled(io, perc >= 95 ? "━" : "╸"; color=p.color)
printstyled(io, "━"^n_left, " "; color=:light_black)
print(io, progress_text)
print(io, " ")
if !isempty(p.status)
print(io, p.status)
else
printstyled(io, "━"^n_filled; color=p.color)
printstyled(io, perc >= 95 ? "━" : "╸"; color=p.color)
printstyled(io, "━"^n_left, " "; color=:light_black)
print(io, progress_text)
end
carriagereturn && print(io, "\r")
end
# Print everything in one call
Expand Down
106 changes: 60 additions & 46 deletions src/Operations.jl
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,10 @@ function collect_artifacts(pkg_root::String; platform::AbstractPlatform=HostPlat
end

mutable struct DownloadState
state::Symbol
state::Symbol # is :ready, :running, :done, or :failed
status::String
status_update_time::UInt64 # ns
status_lock::Base.ReentrantLock
const bar::MiniProgressBar
end

Expand All @@ -835,13 +838,11 @@ function download_artifacts(ctx::Context;
pkg_root === nothing || push!(pkg_roots, pkg_root)
end
push!(pkg_roots, dirname(env.project_file))
used_artifact_tomls = Set{String}()
download_jobs = Function[]
download_jobs = Dict{SHA1, Function}()

print_lock = Base.ReentrantLock() # for non-fancyprint printing

# name -> (state, bar) where state is :ready, :running, :done, or :failed
download_states = Dict{String, DownloadState}()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Artifact names are not globally unique, so I changed the key to the artifact tree hash.

download_states = Dict{SHA1, DownloadState}()

errors = Channel{Any}(Inf)
is_done = false
Expand All @@ -852,65 +853,78 @@ function download_artifacts(ctx::Context;
ansi_enablecursor = "\e[?25h"
ansi_disablecursor = "\e[?25l"

longest_name_length = 0 # will be updated later
for pkg_root in pkg_roots
for (artifacts_toml, artifacts) in collect_artifacts(pkg_root; platform)
# For each Artifacts.toml, install each artifact we've collected from it
for name in keys(artifacts)
bar = MiniProgressBar(; main=false, indent=2, color = Base.info_color(), mode=:data, always_reprint=true)
progress = (total, current) -> (bar.max = total; bar.current = current)
# returns a string if exists, or function that downloads the artifact if not
ret = ensure_artifact_installed(name, artifacts[name], artifacts_toml;
verbose, quiet_download=!(usable_io(io)), io, progress)
if ret isa Function
download_states[name] = DownloadState(:ready, bar)
push!(download_jobs,
() -> begin
try
download_states[name].state = :running
ret()
if !fancyprint
rname = rpad(name, longest_name_length)
@lock print_lock printpkgstyle(io, :Downloaded, "artifact $rname $(MiniProgressBars.pkg_format_bytes(bar.max; sigdigits=1))")
end
catch
download_states[name].state = :failed
rethrow()
else
download_states[name].state = :done
all_collected_artifacts = reduce(vcat, map(pkg_root -> collect_artifacts(pkg_root; platform), pkg_roots))
used_artifact_tomls = Set{String}(map(first, all_collected_artifacts))
longest_name_length = maximum(all_collected_artifacts; init=0) do (artifacts_toml, artifacts)
maximum(textwidth, keys(artifacts); init=0)
end
for (artifacts_toml, artifacts) in all_collected_artifacts
# For each Artifacts.toml, install each artifact we've collected from it
for name in keys(artifacts)
local rname = rpad(name, longest_name_length)
local hash = SHA1(artifacts[name]["git-tree-sha1"])
local bar = MiniProgressBar(;header=rname, main=false, indent=2, color = Base.info_color(), mode=:data, always_reprint=true)
local dstate = DownloadState(:ready, "", time_ns(), Base.ReentrantLock(), bar)
function progress(total, current; status="")
local t = time_ns()
if isempty(status)
dstate.bar.max = total
dstate.bar.current = current
end
lock(dstate.status_lock) do
dstate.status = status
dstate.status_update_time = t
end
end
# returns a string if exists, or function that downloads the artifact if not
local ret = ensure_artifact_installed(name, artifacts[name], artifacts_toml;
verbose, quiet_download=!(usable_io(io)), io, progress)
if ret isa Function
download_states[hash] = dstate
download_jobs[hash] =
() -> begin
try
dstate.state = :running
ret()
if !fancyprint
@lock print_lock printpkgstyle(io, :Installed, "artifact $rname $(MiniProgressBars.pkg_format_bytes(dstate.bar.max; sigdigits=1))")
end
catch
dstate.state = :failed
rethrow()
Comment on lines +893 to +894
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set progress status to failed here and keep failed jobs around in the list until all complete/failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this but got strange formatting when many artifacts failed to install.

 Installing artifacts ╸━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 0/83
  XML2                     failed
  Qt6Wayland               failed
  Gettext                  failed
  Xorg_xcb_util_wm         failed
  Libuuid                  failed
  EarCut                   failed
  Xorg_xcb_util            failed
  Vulkan_Loader            failed
 Installing artifacts ╸━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 0/83
  XML2                     failed
  Qt6Wayland               failed
  Gettext                  failed
 Installing artifacts ╸━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 0/83
 Installing artifacts ╸━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 0/83
  XML2                     failed
 Installing artifacts ╸━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 0/83
  XML2                     failed
 Installing artifacts ╸━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 0/83

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to change the counting of rows to include to include :failed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it an issue if more rows than can fit in the terminal are printed?

else
dstate.state = :done
end
)
end
end
end
push!(used_artifact_tomls, artifacts_toml)
end
end

if !isempty(download_jobs)
longest_name_length = maximum(textwidth, keys(download_states))
for (name, dstate) in download_states
dstate.bar.header = rpad(name, longest_name_length)
end

if fancyprint
t_print = Threads.@spawn begin
try
print(io, ansi_disablecursor)
first = true
timer = Timer(0, interval=1/10)
# TODO: Implement as a new MiniMultiProgressBar
main_bar = MiniProgressBar(; indent=1, header = "Downloading artifacts", color = :green, mode = :int, always_reprint=true)
main_bar = MiniProgressBar(; indent=2, header = "Installing artifacts", color = :green, mode = :int, always_reprint=true)
main_bar.max = length(download_states)
while !is_done
main_bar.current = count(x -> x[2].state == :done, download_states)
main_bar.current = count(x -> x.state == :done, values(download_states))
str = sprint(context=io) do iostr
first || print(iostr, ansi_cleartoend)
n_printed = 1
show_progress(iostr, main_bar; carriagereturn=false)
println(iostr)
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
for dstate in sort!(collect(values(download_states)), by=v->v.bar.max, rev=true)
local status, status_update_time = lock(()->(dstate.status, dstate.status_update_time), dstate.status_lock)
# only update the bar's status message if it is stalled for at least 0.5 s.
# If the new status message is empty, go back to showing the bar without waiting.
if isempty(status) || time_ns() - status_update_time > UInt64(500_000_000)
dstate.bar.status = status
end
dstate.state == :running && (dstate.bar.max > 1000 || !isempty(dstate.bar.status)) || continue
show_progress(iostr, dstate.bar; carriagereturn=false)
println(iostr)
n_printed += 1
Expand All @@ -933,11 +947,11 @@ function download_artifacts(ctx::Context;
end
Base.errormonitor(t_print)
else
printpkgstyle(io, :Downloading, "$(length(download_jobs)) artifacts")
printpkgstyle(io, :Installing, "$(length(download_jobs)) artifacts")
end
sema = Base.Semaphore(ctx.num_concurrent_downloads)
interrupted = false
@sync for f in download_jobs
@sync for f in values(download_jobs)
interrupted && break
Base.acquire(sema)
Threads.@spawn try
Expand All @@ -961,7 +975,7 @@ function download_artifacts(ctx::Context;
length(all_errors) > 1 && println(iostr)
end
end
pkgerror("Failed to download some artifacts:\n\n$(strip(str, '\n'))")
pkgerror("Failed to install some artifacts:\n\n$(strip(str, '\n'))")
end
end

Expand Down
1 change: 1 addition & 0 deletions src/PlatformEngines.jl
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ function download_verify_unpack(
if verbose
@info("Unpacking $(tarball_path) into $(dest)...")
end
isnothing(progress) || progress(10000, 10000; status="unpacking")
open(`$(exe7z()) x $tarball_path -so`) do io
Tar.extract(io, dest, copy_symlinks = copy_symlinks())
end
Expand Down
Loading