From ba9caeea5f0e38e151b2ba180a8bc5b332f8e905 Mon Sep 17 00:00:00 2001 From: Zoltan Csizmadia Date: Sat, 24 Oct 2026 15:06:11 -0500 Subject: [PATCH] upgrade: decide package-manager ownership before showing the plan Under winget or scoop, `skrog upgrade --apply` listed the app step, upgraded the engine and CLI, and only then refused the app and exited 1. Ownership is now decided first: the app step is shown under "will not run" with the manager's command, the rest is applied, and the exit is 0. With only the app behind, it prints the command without asking. --force still replaces the binary. The decisions after the report move into upgradeSteps, with the parts that touch the machine as fields so the order can be tested. Fixes #545 --- CHANGELOG.md | 6 ++ cmd/skrog/upgrade.go | 133 +++++++++++++++++++++++--------- cmd/skrog/upgrade_test.go | 154 ++++++++++++++++++++++++++++++++++++++ docs/install.md | 4 +- docs/upgrading.md | 25 +++++-- 5 files changed, 281 insertions(+), 41 deletions(-) create mode 100644 cmd/skrog/upgrade_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4763f63..ac0ef36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,12 @@ useful than saying where the real one is. moved by the next `apply` — the diff shows the move before anything is written. `skrog wsl-config` and `skrog top` no longer report the ignored line as in effect. `wsl-config --json` pending changes gain `section`, and `moved_from` for a move. +- **`skrog upgrade --apply` under winget or scoop does what its plan says** ([#545](https://github.com/wslkit/skrog/issues/545)). + It listed the app step, upgraded the engine and CLI, and only then refused the app + because a package manager owns it, exiting 1 after doing everything else. Ownership is + now decided before the plan is shown: the app step appears under "will not run" with the + manager's command, the rest is applied, and the exit is 0. With only the app behind it + prints the command without asking. `--force` still replaces the binary. ### Added diff --git a/cmd/skrog/upgrade.go b/cmd/skrog/upgrade.go index 9c6d5f3..7a08935 100644 --- a/cmd/skrog/upgrade.go +++ b/cmd/skrog/upgrade.go @@ -4,6 +4,7 @@ import ( "context" "flag" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -128,7 +129,7 @@ flags: if reportOnly { if *dryRun && len(plan) > 0 { fmt.Println("\nwould run:") - printUpgradePlan(plan) + printUpgradePlan(os.Stdout, plan) fmt.Println("\nnothing was changed (--dry-run)") } // Exit 3 for "there is something to install", matching @@ -139,50 +140,126 @@ flags: return exitOK } + return upgradeSteps{ + apply: *apply, + yes: *yes, + force: *force, + out: os.Stdout, + ownerOf: runningBinaryOwner, + confirm: confirm, + applyPlan: func(p []upgrade.Action) int { return applyUpgradePlan(p, *stateDir) }, + applyApp: func(v string) int { return applyApp(v, opts.StateDir) }, + }.run(rep) +} + +// upgradeSteps is what `skrog upgrade` does once the report is printed. The +// parts that touch the machine are fields, so a test can drive the real +// order of decisions without upgrading anything. +type upgradeSteps struct { + apply, yes, force bool + out io.Writer + + ownerOf func() (upgrade.Owner, bool) + confirm func(prompt string) bool + applyPlan func([]upgrade.Action) int + applyApp func(version string) int +} + +func (u upgradeSteps) run(rep upgrade.Report) int { + plan := rep.Plan() appBehind := rep.AppStream().Status == upgrade.StatusAvailable + latest := rep.AppStream().Latest + wantApp := appBehind && u.apply + + // Who owns the install is decided BEFORE the plan is shown (#545). Asked + // only after the engine and CLI had been upgraded, the refusal (#370) + // turned a run that did everything it could into a non-zero exit, after + // the user had confirmed an app step that was never going to happen. + var manager *upgrade.Owner + if wantApp && !u.force { + if owner, owned := u.ownerOf(); owned { + manager = &owner + wantApp = false + } + } + managedLine := func() { + fmt.Fprintf(u.out, " %-22s %s -> %s managed by %s — run: %s\n", + "app", buildVersion, latest, manager.Name, manager.Command) + } - if len(plan) == 0 && !(appBehind && *apply) { + if len(plan) == 0 && !wantApp { + if manager != nil { + fmt.Fprintf(u.out, "\nthe app is the only thing behind, and %s manages it:\n", manager.Name) + managedLine() + return exitOK + } // The app being behind is not something this command can act on // without --apply, and saying "nothing to do" without that // qualification would read as "you are current". if rep.Available() { - fmt.Println("\nnothing here is Skrog's to apply — see above") + fmt.Fprintln(u.out, "\nnothing here is Skrog's to apply — see above") if appBehind { - fmt.Println("`skrog upgrade --apply` will replace skrog.exe itself") + fmt.Fprintln(u.out, "`skrog upgrade --apply` will replace skrog.exe itself") } } return exitOK } - if !*yes { - fmt.Println("\nwill run:") - printUpgradePlan(plan) - if appBehind && *apply { - fmt.Printf(" %-16s %s -> %s (replaces skrog.exe; the supervisor restarts)\n", - "app", buildVersion, rep.AppStream().Latest) + if !u.yes { + fmt.Fprintln(u.out, "\nwill run:") + printUpgradePlan(u.out, plan) + if wantApp { + fmt.Fprintf(u.out, " %-22s %s -> %s (replaces skrog.exe; the supervisor restarts)\n", + "app", buildVersion, latest) + } + if manager != nil { + fmt.Fprintln(u.out, "\nwill not run:") + managedLine() } - if !confirm("\nproceed?") { - fmt.Println("nothing was changed") + if !u.confirm("\nproceed?") { + fmt.Fprintln(u.out, "nothing was changed") return exitOK } } - if code := applyUpgradePlan(plan, *stateDir); code != exitOK { + if code := u.applyPlan(plan); code != exitOK { return code } // The app last: it is the only step that restarts the supervisor, so a // failure in the engine or CLI half leaves the running binary the one that // produced the log the user is about to read. - if appBehind && *apply { - return applyApp(rep.AppStream().Latest, opts.StateDir, *force) + if wantApp { + return u.applyApp(latest) + } + // Exit 0: everything this command said it would do is done. The app step + // was shown as the manager's to run, not as something that failed. + if manager != nil { + fmt.Fprintf(u.out, "\nthe app is managed by %s; upgrade it with: %s\n"+ + "(`skrog upgrade --apply --force` replaces the binary anyway)\n", + manager.Name, manager.Command) } return exitOK } +// runningBinaryOwner reports the package manager that owns the running +// skrog.exe, if any (#370). +// +// The check needs the RESOLVED path, because a winget install is reached +// through an alias symlink and only the target is inside the package +// directory (#360). applyApp's swap deliberately uses the UNRESOLVED one: +// they are different questions, and conflating them is what #360 was. +func runningBinaryOwner() (upgrade.Owner, bool) { + resolved, err := selfexe.Path() + if err != nil { + return upgrade.Owner{}, false + } + return upgrade.OwnedBy(resolved) +} + // printUpgradePlan shows the commands, not a summary of them: the reader can // then run any of them by hand, and can see that nothing else is happening. -func printUpgradePlan(plan []upgrade.Action) { +func printUpgradePlan(w io.Writer, plan []upgrade.Action) { for _, a := range plan { - fmt.Printf(" skrog %-16s %s -> %s\n", joinArgs(a.Args), orUnset(a.From), a.To) + fmt.Fprintf(w, " skrog %-16s %s -> %s\n", joinArgs(a.Args), orUnset(a.From), a.To) } } @@ -359,29 +436,17 @@ func installedCLIVersion(opts provision.Options) string { // touch the install directory. Nothing here can leave a half-upgraded install // -- SwapBinaries rolls back if any file cannot be replaced, and a download // that fails verification never reaches the swap at all. -func applyApp(version, stateDir string, force bool) int { +// +// Whether a package manager owns the install is decided before the plan is +// shown (upgradeSteps.run), so by the time this runs the answer is "nobody" +// or --force. +func applyApp(version, stateDir string) int { exe, err := os.Executable() if err != nil { fmt.Fprintf(os.Stderr, "skrog: locating the running binary: %v\n", err) return exitError } - // Refuse to fight a package manager (#370). - // - // The check needs the RESOLVED path, because a winget install is reached - // through an alias symlink and only the target is inside the package - // directory (#360). The swap below deliberately uses the UNRESOLVED one: - // they are different questions, and conflating them is what #360 was. - if resolved, rerr := selfexe.Path(); rerr == nil { - if owner, owned := upgrade.OwnedBy(resolved); owned && !force { - fmt.Fprintf(os.Stderr, "skrog: this install is managed by %s, so `skrog upgrade --apply` "+ - "would overwrite files it owns and leave it reporting a version you no longer have.\n\n"+ - " upgrade with: %s\n\n"+ - "Pass --force to replace the binary anyway.\n", owner.Name, owner.Command) - return exitError - } - } - // Deliberately NOT resolved through symlinks (#360). Everywhere else that // derives a sibling uses internal/selfexe, but replacing a binary is the // one case where following a link would be wrong. diff --git a/cmd/skrog/upgrade_test.go b/cmd/skrog/upgrade_test.go new file mode 100644 index 0000000..63d84ba --- /dev/null +++ b/cmd/skrog/upgrade_test.go @@ -0,0 +1,154 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/wslkit/skrog/internal/upgrade" +) + +// upgradeRun is one drive of upgradeSteps.run with the machine faked out: what +// was printed, what was asked, and what was applied, in order. +type upgradeRun struct { + code int + out string + events []string +} + +func driveUpgrade(t *testing.T, rep upgrade.Report, exe string, apply, yes, force bool) upgradeRun { + t.Helper() + var out strings.Builder + var r upgradeRun + r.code = upgradeSteps{ + apply: apply, yes: yes, force: force, out: &out, + // The real ownership check, against a path the test controls. + ownerOf: func() (upgrade.Owner, bool) { return upgrade.OwnedBy(exe) }, + confirm: func(string) bool { + r.events = append(r.events, "confirm") + return true + }, + applyPlan: func(p []upgrade.Action) int { + for _, a := range p { + r.events = append(r.events, "apply "+a.Stream) + } + return exitOK + }, + applyApp: func(v string) int { + r.events = append(r.events, "apply app "+v) + return exitOK + }, + }.run(rep) + r.out = out.String() + return r +} + +// wingetExe is a skrog.exe inside a winget portable package directory, with +// LOCALAPPDATA pointed at a temp dir so OwnedBy finds that root. +func wingetExe(t *testing.T) string { + t.Helper() + local := t.TempDir() + t.Setenv("LOCALAPPDATA", local) + dir := filepath.Join(local, "Microsoft", "WinGet", "Packages", "wslkit.skrog_Microsoft.Winget.Source_8wekyb3d8bbwe") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + return filepath.Join(dir, "skrog.exe") +} + +// plainExe is a zip unpacked by hand: owned by nobody. +func plainExe(t *testing.T) string { + t.Helper() + t.Setenv("LOCALAPPDATA", t.TempDir()) + return filepath.Join(t.TempDir(), "Programs", "skrog", "skrog.exe") +} + +func behindReport(engineBehind bool) upgrade.Report { + engine := upgrade.Stream{Name: "engine", Current: "29.9.1", Latest: "29.9.1", Status: upgrade.StatusCurrent} + if engineBehind { + engine = upgrade.Stream{Name: "engine", Current: "29.8.0", Latest: "29.9.1", Status: upgrade.StatusAvailable} + } + return upgrade.Report{Streams: []upgrade.Stream{ + {Name: "app", Current: "0.8.0", Latest: "0.10.0", Status: upgrade.StatusAvailable}, + engine, + {Name: "cli", Current: "29.9.1", Latest: "29.9.1", Status: upgrade.StatusCurrent}, + }} +} + +// #545: under winget, --apply used to list the app step, upgrade the engine, +// and only then refuse the app and exit non-zero. The plan the user confirms +// must leave the app out, name the manager's command, and the run must exit 0 +// having done exactly what it showed. +func TestUpgradeApplyUnderWingetDecidesOwnershipBeforeThePlan(t *testing.T) { + r := driveUpgrade(t, behindReport(true), wingetExe(t), true, false, false) + + if r.code != exitOK { + t.Errorf("exit %d, want %d\n%s", r.code, exitOK, r.out) + } + if got, want := strings.Join(r.events, ", "), "confirm, apply engine"; got != want { + t.Errorf("events = %q, want %q (the app step must never run)", got, want) + } + shown, _, _ := strings.Cut(r.out, "will not run:") + if strings.Contains(shown, "app") { + t.Errorf("the plan to confirm lists the app step:\n%s", shown) + } + if !strings.Contains(r.out, "managed by winget — run: winget upgrade wslkit.skrog") { + t.Errorf("the plan does not name winget's command:\n%s", r.out) + } +} + +// Nothing to apply except the app, and the app is the manager's: say so and +// exit 0 without asking a question whose only answer changes nothing. +func TestUpgradeApplyUnderWingetWithOnlyTheAppBehind(t *testing.T) { + r := driveUpgrade(t, behindReport(false), wingetExe(t), true, false, false) + + if r.code != exitOK { + t.Errorf("exit %d, want %d\n%s", r.code, exitOK, r.out) + } + if len(r.events) != 0 { + t.Errorf("events = %v, want none: no prompt, nothing applied", r.events) + } + if !strings.Contains(r.out, "winget upgrade wslkit.skrog") { + t.Errorf("output does not name winget's command:\n%s", r.out) + } +} + +// --yes shows no plan, so the manager's command is printed after the run. +func TestUpgradeApplyYesUnderWingetSaysWhatToRunAfter(t *testing.T) { + r := driveUpgrade(t, behindReport(true), wingetExe(t), true, true, false) + + if r.code != exitOK { + t.Errorf("exit %d, want %d\n%s", r.code, exitOK, r.out) + } + if got, want := strings.Join(r.events, ", "), "apply engine"; got != want { + t.Errorf("events = %q, want %q", got, want) + } + if !strings.Contains(r.out, "upgrade it with: winget upgrade wslkit.skrog") { + t.Errorf("output does not name winget's command:\n%s", r.out) + } +} + +// --force keeps the old behaviour: the app is replaced anyway. +func TestUpgradeApplyForceUnderWingetReplacesTheApp(t *testing.T) { + r := driveUpgrade(t, behindReport(true), wingetExe(t), true, false, true) + + if got, want := strings.Join(r.events, ", "), "confirm, apply engine, apply app 0.10.0"; got != want { + t.Errorf("events = %q, want %q", got, want) + } + if !strings.Contains(r.out, "replaces skrog.exe") { + t.Errorf("the plan does not show the app step:\n%s", r.out) + } +} + +// An unmanaged install is unchanged by #545: app last, after the plan. +func TestUpgradeApplyUnmanagedReplacesTheAppLast(t *testing.T) { + r := driveUpgrade(t, behindReport(true), plainExe(t), true, false, false) + + if got, want := strings.Join(r.events, ", "), "confirm, apply engine, apply app 0.10.0"; got != want { + t.Errorf("events = %q, want %q", got, want) + } + if strings.Contains(r.out, "managed by") { + t.Errorf("an unmanaged install is reported as managed:\n%s", r.out) + } +} diff --git a/docs/install.md b/docs/install.md index ede6ec3..10ec8d1 100644 --- a/docs/install.md +++ b/docs/install.md @@ -109,8 +109,8 @@ scoop install skrog Then `scoop update skrog` keeps it current. Skrog notices when winget or scoop manages it — `skrog upgrade --apply` -refuses inside a package-manager directory and names the manager's own upgrade -command, rather than fighting it for the same files. Neither package is +still upgrades the engine and CLI, but leaves the app to the manager and names +its upgrade command, rather than fighting it for the same files. Neither package is blocked on code signing, which this project believed for longer than it should have; only the MSI is. diff --git a/docs/upgrading.md b/docs/upgrading.md index 67143fd..d8e7308 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -54,18 +54,33 @@ Same verb at two scopes: ### If a package manager installed Skrog, let it do the upgrade -`--apply` refuses when the binary lives inside a package manager's directory: +`--apply` leaves the app alone when the binary lives inside a package manager's +directory. It decides that before showing the plan, so the plan you confirm is +the one that runs: the engine and CLI are applied as usual, and the app step +is shown as the manager's to run. ``` $ skrog upgrade --apply -skrog: this install is managed by winget, so `skrog upgrade --apply` would -overwrite files it owns and leave it reporting a version you no longer have. +... +will run: + skrog cli install 29.8.2 -> 29.9.1 + skrog engine upgrade 29.8.1-3 -> 29.9.0-2 - upgrade with: winget upgrade wslkit.skrog +will not run: + app 0.8.0 -> 0.10.0 managed by winget — run: winget upgrade wslkit.skrog -Pass --force to replace the binary anyway. +proceed? [y/N] ``` +It exits 0: everything it said it would do is done. When the app is the only +thing behind, it prints the manager's command and exits without asking. +`--force` replaces the binary anyway. + +Up to 0.10.0 the ownership check came last +([#545](https://github.com/wslkit/skrog/issues/545)): the plan listed the app, +the engine and CLI were upgraded, and only then was the app refused, with +exit 1. + Replacing the files behind the manager's back leaves it believing something untrue: `winget list` keeps reporting the version it installed, a later `winget upgrade` reinstalls over the newer binary and silently downgrades it,