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,