From 7433d788931e5688c207b802bcae94952d51c7cc Mon Sep 17 00:00:00 2001 From: Zoltan Csizmadia Date: Sun, 25 Oct 2026 11:29:22 -0500 Subject: [PATCH] cli: refuse a --distro that conflicts with the installed engine (#561) --- CHANGELOG.md | 6 +++ cmd/skrog/compact.go | 3 ++ cmd/skrog/distroconflict_test.go | 91 ++++++++++++++++++++++++++++++++ cmd/skrog/distroinstall.go | 34 ++++++++++++ cmd/skrog/gpu.go | 3 ++ cmd/skrog/install.go | 3 ++ cmd/skrog/proxy.go | 3 ++ cmd/skrog/relocate.go | 3 ++ cmd/skrog/serve.go | 5 +- cmd/skrog/supervise.go | 5 ++ 10 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 cmd/skrog/distroconflict_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6de510c..74f9c2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,12 @@ useful than saying where the real one is. process". It now retries the compaction itself until the disk is released (about a minute) or `--wait` runs out. `compact` also no longer boots an engine that was already stopped just to stop it, which with `--no-trim` saves that minute. +- **`--distro` no longer acts on a different engine** ([#561](https://github.com/wslkit/skrog/issues/561)). + Wherever an install exists, its manifest decided the distro and the flag was silently + ignored: `skrog compact --distro X` compacted the installed engine instead. `compact`, + `enable-gpu`, `relocate`, `serve`, `proxy`, `supervise` and `uninstall` now refuse a + `--distro` that differs from the installed one (exit 2) and point at `--state-dir`, which + is how another install is reached. The installed name itself, in any case, is accepted. - **`wsl.auto-memory-reclaim` now takes effect** ([#550](https://github.com/wslkit/skrog/issues/550)). `skrog wsl-config apply` wrote `autoMemoryReclaim` under `[wsl2]`, but WSL reads it only as `experimental.autoMemoryReclaim`: it ignored the line and printed `Unknown key diff --git a/cmd/skrog/compact.go b/cmd/skrog/compact.go index be55b25..cbd4bf6 100644 --- a/cmd/skrog/compact.go +++ b/cmd/skrog/compact.go @@ -72,6 +72,9 @@ flags: opts := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir, Distro: *distro}) p := &provision.Provisioner{Logger: cliLogger(false)} + if refuseDistroConflict(p, opts) { + return exitUsage + } target, ok := requireDistroInstall(p, opts) if !ok { return exitNotFound diff --git a/cmd/skrog/distroconflict_test.go b/cmd/skrog/distroconflict_test.go new file mode 100644 index 0000000..9147d84 --- /dev/null +++ b/cmd/skrog/distroconflict_test.go @@ -0,0 +1,91 @@ +package main + +import ( + "io" + "os" + "strings" + "testing" + + "github.com/wslkit/skrog/internal/provision" +) + +// runStderr runs a command's entry point and returns its exit code and stderr. +func runStderr(t *testing.T, run func([]string) int, args []string) (int, string) { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + old := os.Stderr + os.Stderr = w + done := make(chan string, 1) + go func() { + b, _ := io.ReadAll(r) + done <- string(b) + }() + code := run(args) + w.Close() + os.Stderr = old + return code, <-done +} + +// `compact --distro X` on a machine whose engine is Y compacted Y (#561): the +// manifest wins wherever a distro is resolved. Every command that takes +// --distro must refuse the conflict before doing anything at all. +// +// Neither distro exists, so a command that ignored the conflict would fail +// against nothing -- but each one also runs only as far as its own refusal +// once fixed, which is what this asserts. +func TestDistroConflictIsRefusedBeforeAnything(t *testing.T) { + cases := []struct { + name string + run func([]string) int + args []string + }{ + {"compact", runCompact, nil}, + {"enable-gpu", runEnableGPU, nil}, + {"relocate", runRelocate, []string{`Z:\skrog-561-nowhere`, "--dry-run"}}, + {"serve", runServe, []string{"--tcp", "127.0.0.1:0"}}, + {"proxy", runProxy, nil}, + {"supervise", runSupervise, nil}, + {"uninstall", runUninstall, []string{"--yes"}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + opts := writeManifest(t, provision.Manifest{Distro: "skrog-561-recorded"}) + before, _ := os.ReadDir(opts.StateDir) + args := append([]string{"--distro", "skrog-561-other", "--state-dir", opts.StateDir}, c.args...) + + code, stderr := runStderr(t, c.run, args) + if code != exitUsage { + t.Errorf("exit = %d, want %d (usage)", code, exitUsage) + } + for _, want := range []string{"skrog-561-other", "skrog-561-recorded", "--state-dir"} { + if !strings.Contains(stderr, want) { + t.Errorf("stderr does not mention %q:\n%s", want, stderr) + } + } + after, _ := os.ReadDir(opts.StateDir) + if len(after) != len(before) { + t.Errorf("the refused command changed the state dir: %d entries, then %d", len(before), len(after)) + } + }) + } +} + +// The same name, in any case, is not a conflict: WSL distro names are +// case-insensitive, and the e2e suite passes --distro with the manifest's own. +func TestDistroMatchingTheManifestIsNoConflict(t *testing.T) { + opts := writeManifest(t, provision.Manifest{Distro: "skrog-engine"}) + for _, d := range []string{"", "skrog-engine", "Skrog-Engine"} { + opts.Distro = d + if msg := distroConflict(&provision.Provisioner{}, opts); msg != "" { + t.Errorf("--distro %q: %s", d, msg) + } + } + // With no install, --distro is how a distro is chosen at all. + none := provision.Options{StateDir: t.TempDir(), Distro: "hand-rolled"} + if msg := distroConflict(&provision.Provisioner{}, none); msg != "" { + t.Errorf("no manifest: %s", msg) + } +} diff --git a/cmd/skrog/distroinstall.go b/cmd/skrog/distroinstall.go index 03249a1..194b011 100644 --- a/cmd/skrog/distroinstall.go +++ b/cmd/skrog/distroinstall.go @@ -3,6 +3,7 @@ package main import ( "fmt" "os" + "strings" "github.com/wslkit/skrog/internal/provision" "github.com/wslkit/skrog/internal/selfexe" @@ -47,6 +48,39 @@ const legacyBackendMessage = "skrog: this install uses the WSL container session " belong to the other engine. Save anything you need with `wslc` before\n" + " uninstalling.\n" +// distroConflict is what to say when --distro names a different distro from +// the one the install manifest records, or "" when there is no conflict. +// +// The manifest wins everywhere a distro is resolved, so `compact --distro X` +// on a machine whose engine is Y used to compact Y (#561). Honouring the flag +// instead would pair X with Y's state -- its disk path, its secret, its +// supervisor -- so a conflict is refused. Another install is reached by its +// --state-dir, which carries its own manifest. +func distroConflict(p *provision.Provisioner, opts provision.Options) string { + if opts.Distro == "" { + return "" + } + opts = optsWithResolvedStateDir(opts) + m, err := p.ReadManifest(opts) + if err != nil || m.Distro == "" || strings.EqualFold(m.Distro, opts.Distro) { + return "" + } + return fmt.Sprintf("skrog: --distro %s does not match this install's engine, %s (recorded in %s).\n"+ + " --distro only chooses a distro where there is no install; to act on another\n"+ + " install, pass its --state-dir.\n", opts.Distro, m.Distro, opts.StateDir) +} + +// refuseDistroConflict prints distroConflict's message, reporting whether +// there was one. +func refuseDistroConflict(p *provision.Provisioner, opts provision.Options) bool { + msg := distroConflict(p, opts) + if msg == "" { + return false + } + fmt.Fprint(os.Stderr, msg) + return true +} + // requireDistroInstall resolves the engine distro for a command that needs // one, and explains itself when there is not one. func requireDistroInstall(p *provision.Provisioner, opts provision.Options) (string, bool) { diff --git a/cmd/skrog/gpu.go b/cmd/skrog/gpu.go index e7af395..90009f1 100644 --- a/cmd/skrog/gpu.go +++ b/cmd/skrog/gpu.go @@ -74,6 +74,9 @@ flags: opts := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir, Distro: *distro, GPUVendor: string(v)}) log := cliLogger(false) p := &provision.Provisioner{Logger: log} + if refuseDistroConflict(p, opts) { + return exitUsage + } targetDistro, ok := requireDistroInstall(p, opts) if !ok { diff --git a/cmd/skrog/install.go b/cmd/skrog/install.go index bad01f5..8174e02 100644 --- a/cmd/skrog/install.go +++ b/cmd/skrog/install.go @@ -326,6 +326,9 @@ flags: opts := provision.Options{Distro: *distro, StateDir: *stateDir} log := cliLogger(false) p := &provision.Provisioner{Logger: log} + if refuseDistroConflict(p, opts) { + return exitUsage + } // A named engine is removed with `skrog target remove`; uninstalling one as // if it were an install would leave its context and logon entry behind. diff --git a/cmd/skrog/proxy.go b/cmd/skrog/proxy.go index a8e9c92..b0cc70e 100644 --- a/cmd/skrog/proxy.go +++ b/cmd/skrog/proxy.go @@ -67,6 +67,9 @@ flags: // The manifest knows which distro this machine actually has, which matters // when it was installed under a custom name. p := &provision.Provisioner{Logger: log} + if refuseDistroConflict(p, opts) { + return exitUsage + } targetDistro := opts.Distro if m, err := p.ReadManifest(opts); err == nil && m.Distro != "" { targetDistro = m.Distro diff --git a/cmd/skrog/relocate.go b/cmd/skrog/relocate.go index a6fcce7..8a79775 100644 --- a/cmd/skrog/relocate.go +++ b/cmd/skrog/relocate.go @@ -87,6 +87,9 @@ flags: opts := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir, Distro: *distro}) p := &provision.Provisioner{Logger: cliLogger(false)} + if refuseDistroConflict(p, opts) { + return exitUsage + } name, ok := requireDistroInstall(p, opts) if !ok { return exitNotFound diff --git a/cmd/skrog/serve.go b/cmd/skrog/serve.go index 703b419..1c642cf 100644 --- a/cmd/skrog/serve.go +++ b/cmd/skrog/serve.go @@ -72,6 +72,10 @@ flags: opts := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir, Distro: *distro}) log := cliLogger(false) + p := &provision.Provisioner{Logger: log} + if refuseDistroConflict(p, opts) { + return exitUsage + } cfg, err := serverTLSConfig(tlsDir(opts.StateDir)) if err != nil { @@ -79,7 +83,6 @@ flags: return exitNotFound } - p := &provision.Provisioner{Logger: log} targetDistro, ok := resolveDistro(p, opts) if !ok { fmt.Fprintln(os.Stderr, "skrog: no install found. Run `skrog install` first.") diff --git a/cmd/skrog/supervise.go b/cmd/skrog/supervise.go index a28e7a8..84c2dd3 100644 --- a/cmd/skrog/supervise.go +++ b/cmd/skrog/supervise.go @@ -180,6 +180,11 @@ flags: opts := provision.Options{Distro: *distro, StateDir: *stateDir} opts = optsWithResolvedStateDir(opts) + // A refused command must not take the lock or clear the endpoint record. + if refuseDistroConflict(&provision.Provisioner{}, opts) { + return exitUsage + } + // Single instance before anything else: two supervisors would fight over // the pipe and the engine. lock, err := supervise.Acquire(opts.StateDir)