From 1feabf498de5a0e70917a34eee3522a06d879f3b Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Wed, 20 May 2026 00:39:30 +0800 Subject: [PATCH] fix(progress): show bytes/speed/ETA during formula installs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Formula installs only rendered "[n/N] pkg" in the bottom status line while casks showed "[n/N] pkg 12M/189M · 7M/s · ~24s" — the head split was the visible inconsistency users noticed during longer formula downloads like vhs. Parameterize CacheTracker by CacheKind (cask|formula) so it queries `brew --cache --formula ` for bottles, wire a tracker into the formula install loop the same way installCasksWithProgress does, and collapse formatFormulaHead / formatCaskHead into a single formatHead. SetPhase stays — it still gates the per-package byte-state reset. --- internal/archtest/baseline/no-direct-exec.txt | 2 +- internal/brew/brew_install.go | 35 ++++++++++++++- internal/brew/cache.go | 44 +++++++++++++------ internal/brew/cache_test.go | 25 +++++++++++ internal/ui/progress.go | 23 ++++------ internal/ui/progress_test.go | 40 ++++++++++++++++- 6 files changed, 136 insertions(+), 33 deletions(-) diff --git a/internal/archtest/baseline/no-direct-exec.txt b/internal/archtest/baseline/no-direct-exec.txt index e7f413c..946e014 100644 --- a/internal/archtest/baseline/no-direct-exec.txt +++ b/internal/archtest/baseline/no-direct-exec.txt @@ -2,7 +2,7 @@ # Each line is : of a known existing violation. # Regenerate: ARCHTEST_UPDATE_BASELINE=1 go test ./internal/archtest/... internal/auth/login.go:191 -internal/brew/brew_install.go:400 +internal/brew/brew_install.go:433 internal/cli/snapshot.go:21 internal/diff/compare.go:247 internal/diff/compare.go:253 diff --git a/internal/brew/brew_install.go b/internal/brew/brew_install.go index e67700c..4da358d 100644 --- a/internal/brew/brew_install.go +++ b/internal/brew/brew_install.go @@ -232,7 +232,7 @@ func installCasksWithProgress(pkgs []string, sizes map[string]int64, progress *u var trackerCancel context.CancelFunc var trackerDone chan struct{} if size, ok := sizes[pkg]; ok && size > 0 { - tracker, err := NewCacheTracker(pkg) + tracker, err := NewCacheTracker(pkg, CacheKindCask) if err == nil { ctx, cancel := context.WithCancel(context.Background()) trackerCancel = cancel @@ -351,9 +351,42 @@ func runSerialInstallWithProgress(pkgs []string, sizes map[string]int64, progres for _, pkg := range pkgs { job := installJob{name: pkg, isCask: false} progress.SetCurrent(job.name) + + // Seed totalBytes immediately so the head shows "0B/" instead + // of "—" while the tracker's first poll lands. Formulae with unknown + // size pass 0 and the head's bytes column stays "—". + progress.SetCurrentBytes(0, sizes[pkg]) + + // Start a CacheTracker for the formula bottle. Mirrors the cask path: + // brew writes to .incomplete during download then renames, and + // we poll the file size at 500ms intervals. trackerDone gates on the + // goroutine's final emit so stale bytes can't bleed into the next pkg. + var trackerCancel context.CancelFunc + var trackerDone chan struct{} + if size, ok := sizes[pkg]; ok && size > 0 { + tracker, err := NewCacheTracker(pkg, CacheKindFormula) + if err == nil { + ctx, cancel := context.WithCancel(context.Background()) + trackerCancel = cancel + trackerDone = make(chan struct{}) + go func() { + defer close(trackerDone) + tracker.Run(ctx, func(b int64) { + progress.SetCurrentBytes(b, size) + }) + }() + } + } + start := time.Now() errMsg := installFormulaWithError(job.name) elapsed := time.Since(start) + + if trackerCancel != nil { + trackerCancel() + <-trackerDone + } + progress.IncrementWithStatus(errMsg == "") duration := ui.FormatDuration(elapsed) if errMsg == "" { diff --git a/internal/brew/cache.go b/internal/brew/cache.go index a5e4e24..3e53f3a 100644 --- a/internal/brew/cache.go +++ b/internal/brew/cache.go @@ -7,19 +7,31 @@ import ( "time" ) -// CacheTracker polls the exact brew download path for a cask and reports the -// partial file's current size. During download brew writes to +// CacheKind disambiguates the brew --cache lookup. Formulae and casks share +// a global name space where some tokens collide (e.g. `docker` is both), so +// we must pass --formula or --cask explicitly when resolving the cache path. +type CacheKind int + +const ( + CacheKindCask CacheKind = iota + CacheKindFormula +) + +// CacheTracker polls the exact brew download path for a package and reports +// the partial file's current size. During download brew writes to // `.incomplete`, then renames to `` on success — we -// stat both and report the larger. +// stat both and report the larger. Works for both formula bottles and cask +// downloads — they follow the same .incomplete → rename convention. type CacheTracker struct { finalPath string interval time.Duration } -// NewCacheTracker builds a tracker for the given cask. The exact cache path -// is resolved via `brew --cache --cask `. -func NewCacheTracker(caskName string) (*CacheTracker, error) { - path, err := resolveBrewCachePath(caskName) +// NewCacheTracker builds a tracker for the given package. The exact cache +// path is resolved via `brew --cache --cask ` or +// `brew --cache --formula ` per kind. +func NewCacheTracker(name string, kind CacheKind) (*CacheTracker, error) { + path, err := resolveBrewCachePath(name, kind) if err != nil { return nil, err } @@ -63,13 +75,17 @@ func (t *CacheTracker) currentSize() int64 { return largest } -// resolveBrewCachePath returns the exact path brew uses for the cask's -// download. Matching the previous substring-based approach against the cask -// name is unreliable: brew names the cached file after the URL's basename -// (e.g. `google-chrome` → `…--googlechrome.dmg`), so the cask name often -// doesn't appear in it. -func resolveBrewCachePath(caskName string) (string, error) { - out, err := currentRunner().Output("--cache", "--cask", caskName) +// resolveBrewCachePath returns the exact path brew uses for the package's +// download. Matching the previous substring-based approach against the +// package name is unreliable: brew names the cached file after the URL's +// basename (e.g. `google-chrome` → `…--googlechrome.dmg`, formula bottles +// embed sha + platform), so the name often doesn't appear in it. +func resolveBrewCachePath(name string, kind CacheKind) (string, error) { + flag := "--cask" + if kind == CacheKindFormula { + flag = "--formula" + } + out, err := currentRunner().Output("--cache", flag, name) if err != nil { return "", err } diff --git a/internal/brew/cache_test.go b/internal/brew/cache_test.go index 5d19cd5..9f707e1 100644 --- a/internal/brew/cache_test.go +++ b/internal/brew/cache_test.go @@ -77,6 +77,31 @@ func TestCacheTrackerReportsLargerWhenBothExist(t *testing.T) { assert.EqualValues(t, 5000, observed.Load()) } +func TestNewCacheTrackerPassesKindFlag(t *testing.T) { + cases := []struct { + name string + kind CacheKind + wantFlag string + }{ + {"cask uses --cask", CacheKindCask, "--cask"}, + {"formula uses --formula", CacheKindFormula, "--formula"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var captured []string + withFakeBrew(t, func(args []string) ([]byte, error) { + captured = append([]string(nil), args...) + return []byte("/tmp/abc--pkg\n"), nil + }) + + tracker, err := NewCacheTracker("pkg", tc.kind) + require.NoError(t, err) + require.NotNil(t, tracker) + assert.Equal(t, []string{"--cache", tc.wantFlag, "pkg"}, captured) + }) + } +} + func TestCacheTrackerNoMatchReportsZero(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "unrelated--Other.dmg"), make([]byte, 100), 0o644)) diff --git a/internal/ui/progress.go b/internal/ui/progress.go index b150d38..7744325 100644 --- a/internal/ui/progress.go +++ b/internal/ui/progress.go @@ -196,13 +196,7 @@ func (sp *StickyProgress) renderInline() { // the data row so it visually separates the scrolling output from the // status line at the very last terminal row. func (sp *StickyProgress) formatLines() []string { - var head string - switch sp.phase { - case PhaseCask: - head = sp.formatCaskHead() - default: - head = sp.formatFormulaHead() - } + head := sp.formatHead() cols := 80 if sp.region != nil { @@ -232,12 +226,11 @@ func (sp *StickyProgress) formatLines() []string { return []string{divider, status} } -func (sp *StickyProgress) formatFormulaHead() string { - pkg := truncate(sp.currentPkg, 24) - return fmt.Sprintf("[%d/%d] %-24s", sp.completed, sp.total, pkg) -} - -func (sp *StickyProgress) formatCaskHead() string { +// formatHead renders the status-line prefix for both formula and cask phases. +// Columns: [completed/total] pkg bytes/total · speed · ETA. Each column falls +// back to "—" when the underlying data is unavailable (HEAD pre-fetch failed, +// tracker hasn't polled yet, or speed EMA not yet established). +func (sp *StickyProgress) formatHead() string { pkg := truncate(sp.currentPkg, 18) bytes := "—" speed := "—" @@ -247,7 +240,7 @@ func (sp *StickyProgress) formatCaskHead() string { if sp.speed > 0 { speed = fmt.Sprintf("%s/s", humanBytes(int64(sp.speed))) } - eta := sp.estimateCurrentCaskETA() + eta := sp.estimateCurrentETA() if eta == "" { eta = "—" } @@ -362,7 +355,7 @@ func (sp *StickyProgress) observeBytesAt(current int64, now time.Time) { sp.lastTime = now } -func (sp *StickyProgress) estimateCurrentCaskETA() string { +func (sp *StickyProgress) estimateCurrentETA() string { if sp.speed <= 0 || sp.totalBytes <= 0 || sp.currentBytes >= sp.totalBytes { if sp.totalBytes > 0 && sp.currentBytes < sp.totalBytes { return "estimating..." diff --git a/internal/ui/progress_test.go b/internal/ui/progress_test.go index 0c62f67..ec5a070 100644 --- a/internal/ui/progress_test.go +++ b/internal/ui/progress_test.go @@ -249,7 +249,7 @@ func TestStickyProgressBytesETAUsesSpeed(t *testing.T) { sp.currentBytes = 100_000_000 sp.totalBytes = 500_000_000 sp.speed = 10_000_000 // 10 MB/s - eta := sp.estimateCurrentCaskETA() + eta := sp.estimateCurrentETA() // 400MB / 10 MB/s = 40s. assert.Equal(t, "~40s", eta) } @@ -260,10 +260,46 @@ func TestStickyProgressEstimatingPlaceholder(t *testing.T) { sp.currentBytes = 0 sp.totalBytes = 100_000_000 sp.speed = 0 - eta := sp.estimateCurrentCaskETA() + eta := sp.estimateCurrentETA() assert.Equal(t, "estimating...", eta) } +func TestStickyProgressFormatHeadShowsBytesSpeedETA(t *testing.T) { + // The head must be identical in shape for formula and cask phases — + // the prior split (formula = count only, cask = bytes/speed/ETA) was + // the visible inconsistency users noticed during longer formula + // downloads like vhs. + for _, phase := range []Phase{PhaseFormula, PhaseCask} { + sp := NewStickyProgress(5) + sp.SetPhase(phase) + sp.currentPkg = "vhs" + sp.completed = 1 + sp.currentBytes = 12 * 1024 * 1024 + sp.totalBytes = 28 * 1024 * 1024 + sp.speed = 5 * 1024 * 1024 + + head := sp.formatHead() + assert.Contains(t, head, "[1/5]") + assert.Contains(t, head, "vhs") + assert.Contains(t, head, "12M/28M") + assert.Contains(t, head, "5M/s") + // 16M remaining / 5M/s ≈ 3s. + assert.Contains(t, head, "~3s") + } +} + +func TestStickyProgressFormatHeadFallsBackToDashes(t *testing.T) { + // HEAD pre-fetch failed (size unknown) and tracker hasn't seeded speed + // yet → all three data columns show "—" instead of bogus zeros. + sp := NewStickyProgress(3) + sp.currentPkg = "wget" + sp.completed = 0 + + head := sp.formatHead() + assert.Contains(t, head, "wget") + assert.Contains(t, head, "— · — · —") +} + func TestStickyProgressFallsBackWhenScrollRegionUnsupported(t *testing.T) { t.Setenv("TERM", "dumb") sp := NewStickyProgress(3)