From e66b83b72b256fee5454b0ffabdbd814f6154a41 Mon Sep 17 00:00:00 2001 From: Bozhidar Batsov Date: Fri, 23 Oct 2026 16:01:01 +0300 Subject: [PATCH 1/3] Stop failing Babashka for a reply REPLy wouldn't show anyway wire.error-terminal failed on any key but a few after an error status, so Babashka's changed-namespaces, which arrives after done, failed it. REPLy and rebel-readline stop reading a request at an error, but all they'd lose is output, values and need-input, so that's what the rule looks for now. The late message is still wire.after-done's warning. --- PLAN.md | 9 ++++----- internal/checks/checks_test.go | 4 ++++ internal/checks/fake_test.go | 8 ++++++++ internal/checks/wire.go | 24 ++++++++++++++++++------ 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/PLAN.md b/PLAN.md index 9f50663..ecebdd9 100644 --- a/PLAN.md +++ b/PLAN.md @@ -35,11 +35,10 @@ lookups and completions, plus requests that go wrong. ### Rough Edges -- `wire.error-terminal` fails Babashka for a message with nothing but - `changed-namespaces` that arrives after `eval-error`. The rule fails - on any key that's not in `afterErrorKeys` (`internal/checks/wire.go`), - but the clients it names (REPLy and rebel-readline) lose only output, - values and `need-input` there, so it should look for those instead. +- Done: `wire.error-terminal` no longer fails Babashka for a message + with nothing but `changed-namespaces` that arrives after `eval-error`. + The clients it names (REPLy and rebel-readline) lose only output, + values and `need-input` there, so that's what it looks for now. - `proof run babashka` and `proof proxy babashka` don't work, even though `proof serve -like babashka` does. The profiles should be embedded in the binary, as people who install proof with `go install` diff --git a/internal/checks/checks_test.go b/internal/checks/checks_test.go index 55a66e5..d6a3ed4 100644 --- a/internal/checks/checks_test.go +++ b/internal/checks/checks_test.go @@ -90,8 +90,12 @@ func TestChecksCatchMisbehaviour(t *testing.T) { {"ns fallback", quirks{nsFallback: true}, map[string]check.Verdict{"eval.unknown-ns": F}}, {"two dones", quirks{twoDones: true}, map[string]check.Verdict{"wire.one-done": W}}, {"value after done", quirks{valueAfterDone: true}, map[string]check.Verdict{"wire.after-done": W, "wire.error-terminal": F}}, + // REPLy and rebel-readline don't read changed-namespaces, so it + // doesn't matter that they miss it after an error. + {"state after done", quirks{stateAfterDone: true}, map[string]check.Verdict{"wire.after-done": W}}, {"out and value together", quirks{payloadTogether: true}, map[string]check.Verdict{"wire.one-payload": F}}, {"output after eval-error", quirks{errorThenOut: true}, map[string]check.Verdict{"wire.error-terminal": F}}, + {"state after eval-error", quirks{errorThenState: true}, map[string]check.Verdict{}}, {"session not echoed", quirks{noSessionEcho: true}, map[string]check.Verdict{"wire.session-echo": F}}, {"output without id", quirks{noID: true}, map[string]check.Verdict{ "wire.id": W, "eval.stdout": F, "eval.stderr": F, "eval.error-report": W}}, diff --git a/internal/checks/fake_test.go b/internal/checks/fake_test.go index ef62c44..52beba2 100644 --- a/internal/checks/fake_test.go +++ b/internal/checks/fake_test.go @@ -36,8 +36,10 @@ type quirks struct { nsFallback bool // unknown ns silently falls back to user twoDones bool valueAfterDone bool + stateAfterDone bool // changed-namespaces follows done, like Babashka's payloadTogether bool // out and value in one message errorThenOut bool // output after the eval-error status + errorThenState bool // changed-namespaces after the eval-error status noSessionEcho bool noID bool // output messages carry no id intValue bool // value is an integer @@ -320,6 +322,9 @@ func (s *fakeServer) eval(c net.Conn, req nrepl.Message, local map[string]bool) if q.valueAfterDone { s.send(c, req, map[string]any{"value": "late"}) } + if q.stateAfterDone { + s.send(c, req, map[string]any{"changed-namespaces": map[string]any{}}) + } } var values []string for _, form := range forms(code) { @@ -351,6 +356,9 @@ func (s *fakeServer) eval(c net.Conn, req nrepl.Message, local map[string]bool) if q.errorThenOut { s.send(c, req, map[string]any{"out": "after the error"}) } + if q.errorThenState { + s.send(c, req, map[string]any{"changed-namespaces": map[string]any{}}) + } finish() return case "define": diff --git a/internal/checks/wire.go b/internal/checks/wire.go index 7055559..33d3b18 100644 --- a/internal/checks/wire.go +++ b/internal/checks/wire.go @@ -17,10 +17,6 @@ import ( // Fields that are strings wherever they appear. var stringFields = []string{"id", "session", "ns", "value", "out", "err", "new-session", "ex", "root-ex"} -// Keys that may legitimately follow an error status, e.g. on the final -// "done" message. -var afterErrorKeys = map[string]bool{"id": true, "session": true, "status": true, "ns": true} - // Keys that may arrive after done: the spec allows late output. var afterDoneKeys = map[string]bool{"id": true, "session": true, "out": true, "err": true} @@ -178,7 +174,7 @@ func WireRules() []*check.Rule { }, { ID: "wire.error-terminal", - Title: "Nothing but done follows an error status", + Title: "No output, value or need-input follows an error status", Severity: check.Fail, Why: "REPLy (behind lein repl) and rebel-readline stop reading a request at the first error or eval-error status, so later output or values never show up.", Refs: []check.Ref{replyTerminal, rebelTerminal}, @@ -189,7 +185,7 @@ func WireRules() []*check.Rule { // nREPL goes on with the next form after one throws, and // REPLy sends one form at a time. Only Clojure code can be // told apart into forms, though. - if k := firstKeyNotIn(r.Msg, afterErrorKeys); k != "" && !severalForms(r.Req.Str("code")) { + if k := lostAfterError(r.Msg); k != "" && !severalForms(r.Req.Str("code")) { report(k+" arrived after an error status", r.Msg.String()) } }), @@ -325,6 +321,22 @@ func eachReply(f func(reply, check.Reporter)) func([]nrepl.Event, check.Reporter } } +// lostAfterError returns the part of m that REPLy and rebel-readline would +// have shown, or "" if there's none. They no longer read a request's +// replies after an error status, but anything else in them (e.g. +// Babashka's changed-namespaces) is of no use to them anyway. +func lostAfterError(m nrepl.Message) string { + for _, k := range []string{"err", "out", "value"} { + if m.Has(k) { + return k + } + } + if m.HasStatus("need-input") { + return "need-input" + } + return "" +} + // firstKeyNotIn returns the first key of m (in sorted order) that allowed // doesn't list, or "". func firstKeyNotIn(m nrepl.Message, allowed map[string]bool) string { From 0164077bbc8eedb71bc972e61b5559826308d837 Mon Sep 17 00:00:00 2001 From: Bozhidar Batsov Date: Fri, 23 Oct 2026 16:03:04 +0300 Subject: [PATCH 2/3] Let proof run and proof proxy use the bundled profiles by name proof serve -like already took a server's name, but run and proxy wanted a file, and people who install proof with go install don't have the profiles folder at all. The profiles are now embedded in the binary, and the name of one of them means that profile, while anything else is the path of a profile. --- PLAN.md | 8 ++-- README.md | 6 ++- cmd/proof/main.go | 7 +++- cmd/proof/proxy.go | 2 +- doc/design.md | 1 + doc/hacking.md | 6 ++- doc/usage.md | 15 +++++--- internal/profile/profile.go | 63 ++++++++++++++++++++++++++++---- internal/profile/profile_test.go | 60 ++++++++++++++++++++++++++---- internal/serve/matrix_test.go | 13 +++++-- profiles/profiles.go | 10 +++++ 11 files changed, 157 insertions(+), 34 deletions(-) create mode 100644 profiles/profiles.go diff --git a/PLAN.md b/PLAN.md index ecebdd9..f6172c8 100644 --- a/PLAN.md +++ b/PLAN.md @@ -39,10 +39,10 @@ lookups and completions, plus requests that go wrong. with nothing but `changed-namespaces` that arrives after `eval-error`. The clients it names (REPLy and rebel-readline) lose only output, values and `need-input` there, so that's what it looks for now. -- `proof run babashka` and `proof proxy babashka` don't work, even - though `proof serve -like babashka` does. The profiles should be - embedded in the binary, as people who install proof with `go install` - don't have the `profiles` folder at all. +- Done: `proof run babashka` and `proof proxy babashka` work, just like + `proof serve -like babashka`, as the profiles are embedded in the + binary. People who install proof with `go install` don't have the + `profiles` folder at all. - `proof run profiles/babashka.toml -only 'eval.*'` fails with "expected one profile", as flags after the profile are taken for arguments. proof should either accept them there or say that flags go diff --git a/README.md b/README.md index 9afccbc..7b85674 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,9 @@ value = "3" The [profiles](profiles) folder has complete profiles for nREPL itself, Babashka, Basilisp, ClojureCLR, jank, dialtone (Erlang) and repartee -(Elixir), which you can use as a starting point. +(Elixir), which you can use as a starting point. They come with proof as +well, so `proof run babashka` will check Babashka (provided you have it +installed). Now you can run the checks: @@ -84,7 +86,7 @@ If you're working on a client, `proof proxy` can sit between it and a server and record everything the two of them say to each other: ```shell -$ proof proxy -listen 127.0.0.1:7888 profiles/babashka.toml +$ proof proxy -listen 127.0.0.1:7888 babashka ``` Connect your client to port 7888, use it for a while (or run its test diff --git a/cmd/proof/main.go b/cmd/proof/main.go index 5c957e7..3f4d107 100644 --- a/cmd/proof/main.go +++ b/cmd/proof/main.go @@ -34,6 +34,9 @@ Usage: proof list list every check, rule and scenario proof version +PROFILE is a profile file or the name of a profile that comes with proof +(e.g. babashka), and proof list shows their names. + The exit status of run is 0 when everything passed (or failed as the profile expects), 1 when the server failed checks, 2 when proof couldn't start (bad flags or profile, or the server didn't come up), and 3 when some @@ -106,7 +109,7 @@ func run(args []string) int { fmt.Fprintln(os.Stderr, "proof run: expected one profile") return 2 } - p, err := profile.Load(fs.Arg(0)) + p, err := profile.Find(fs.Arg(0)) if err != nil { fmt.Fprintln(os.Stderr, "proof:", err) return 2 @@ -293,7 +296,7 @@ func list(w io.Writer) { for _, s := range serve.Scenarios() { fmt.Fprintf(w, "%-*s %s (%s)\n", nameWidth, s.Name, s.Title, s.Who) } - fmt.Fprint(w, "\nServers for proof serve -like, with their scenarios:\n") + fmt.Fprint(w, "\nProfiles that come with proof, and their scenarios for proof serve -like:\n") for _, p := range serve.Presets() { line := fmt.Sprintf("%-*s", nameWidth, p.Name) for i, s := range p.Scenarios { diff --git a/cmd/proof/proxy.go b/cmd/proof/proxy.go index f166319..b6f74fd 100644 --- a/cmd/proof/proxy.go +++ b/cmd/proof/proxy.go @@ -85,7 +85,7 @@ func proxyUntil(ctx context.Context, args []string, stdout, stderr io.Writer, li var p *profile.Profile if fs.NArg() == 1 { var err error - if p, err = profile.Load(fs.Arg(0)); err != nil { + if p, err = profile.Find(fs.Arg(0)); err != nil { fmt.Fprintln(stderr, "proof:", err) return 2 } diff --git a/doc/design.md b/doc/design.md index d885856..f362d53 100644 --- a/doc/design.md +++ b/doc/design.md @@ -269,6 +269,7 @@ internal/server starting servers internal/profile loading profiles nrepl a client that records all messages bencode a strict bencode implementation +profiles the profiles of the servers in the matrix, embedded in the binary ``` Each package depends only on the ones below it. `internal/check` knows diff --git a/doc/hacking.md b/doc/hacking.md index 75e3bb9..3f0d1ca 100644 --- a/doc/hacking.md +++ b/doc/hacking.md @@ -35,6 +35,10 @@ $ bin/proof run profiles/babashka.toml $ bin/proof run profiles/clojure.toml ``` +Note that `bin/proof run clojure` would use the copy of the profile +that's embedded in the binary, so while you're working on a profile it's +best to pass its path. + ## Codebase Overview | Path | Contents | @@ -50,7 +54,7 @@ $ bin/proof run profiles/clojure.toml | `internal/proxy` | Forwarding the traffic between a client and a server, for `proof proxy`. | | `internal/serve` | The server behind `proof serve`: its piece of Clojure (`lang.go` and `eval.go`), the scenarios (`scenarios.go`), sessions (`session.go`) and the server itself (`serve.go`). | | `internal/report` | Text and JSON reports and the compatibility matrix. | -| `profiles` | The profiles for the servers in the compatibility matrix. | +| `profiles` | The profiles for the servers in the compatibility matrix. `profiles/profiles.go` embeds them in the binary, so they can be used by name. | | `doc/spec-changes.md` | All the gaps and disagreements found in the draft spec. | ## Testing diff --git a/doc/usage.md b/doc/usage.md index b546e4f..0e8c26e 100644 --- a/doc/usage.md +++ b/doc/usage.md @@ -40,6 +40,11 @@ closest to your server. If you're working on a Clojure dialect you can probably use `clojure.toml` almost as is, otherwise `dialtone.toml` is a better starting point. +The profiles in this folder come with proof, so you can also use them by +name instead of passing a file (e.g. `proof run babashka`). If you've +made a copy of one of them, pass its path instead (e.g. `proof run +babashka.toml`). + Here's a minimal profile: ```toml @@ -202,7 +207,7 @@ proof: with one row per check and one column per server: ```shell -$ proof run -json clojure.json profiles/clojure.toml +$ proof run -json clojure.json clojure $ proof run -json my-server.json my-server.toml $ proof matrix clojure.json my-server.json > matrix.md ``` @@ -224,7 +229,7 @@ running) and forwards everything between your client and the server, recording every message on the way: ```shell -$ proof proxy -listen 127.0.0.1:7888 profiles/clojure.toml +$ proof proxy -listen 127.0.0.1:7888 clojure Starting Clojure (nrepl/nrepl 1.8.0)... Forwarding 127.0.0.1:7888 to localhost:53613. Connect your client to 127.0.0.1:7888 and press Ctrl-C when it's done. ``` @@ -265,8 +270,8 @@ the chance. It's a good idea to try your client with a few servers, as they don't all support the same ops. Any profile from the [profiles](../profiles) -folder will do, and with `-address` proof will forward your client to a -server that's already running: +folder will do (e.g. `proof proxy babashka`), and with `-address` proof +will forward your client to a server that's already running: ```shell $ proof proxy -address localhost:1667 @@ -290,7 +295,7 @@ the proxy in the background, wait for it to accept connections, run the tests against it and stop it with `SIGINT`: ```shell -proof proxy -listen 127.0.0.1:7888 profiles/babashka.toml & +proof proxy -listen 127.0.0.1:7888 babashka & proxy=$! until nc -z 127.0.0.1 7888; do kill -0 $proxy || exit 2 diff --git a/internal/profile/profile.go b/internal/profile/profile.go index 119ddc7..3a10cbf 100644 --- a/internal/profile/profile.go +++ b/internal/profile/profile.go @@ -5,11 +5,16 @@ package profile import ( "errors" "fmt" + "io/fs" "os" "path/filepath" + "slices" + "strings" "time" "github.com/BurntSushi/toml" + + "github.com/nrepl/proof/profiles" ) // Profile describes one server implementation. @@ -76,22 +81,66 @@ type Snippet struct { // Load reads and validates a profile. func Load(path string) (*Profile, error) { - var p Profile - md, err := toml.DecodeFile(path, &p) + data, err := os.ReadFile(path) if err != nil { return nil, err } - if undecoded := md.Undecoded(); len(undecoded) > 0 { - return nil, fmt.Errorf("%s: unknown keys %v", path, undecoded) - } abs, err := filepath.Abs(path) if err != nil { return nil, err } - p.Dir = filepath.Dir(abs) + return parse(path, data, filepath.Dir(abs)) +} + +// Find loads the bundled profile with the given name (e.g. babashka), or +// the profile at path when it's not one of their names. +func Find(path string) (*Profile, error) { + if !slices.Contains(Bundled(), path) { + p, err := Load(path) + if errors.Is(err, fs.ErrNotExist) && filepath.Base(path) == path && filepath.Ext(path) == "" { + return nil, fmt.Errorf("there's no file named %s and no bundled profile with that name (the bundled ones are %s)", + path, strings.Join(Bundled(), ", ")) + } + return p, err + } + data, err := fs.ReadFile(profiles.FS, path+".toml") + if err != nil { + return nil, err + } + // The bundled profiles start programs from the PATH, so they don't + // need a directory. + return parse(path, data, "") +} + +// Bundled returns the names of the profiles that come with proof. +func Bundled() []string { + entries, err := fs.ReadDir(profiles.FS, ".") + if err != nil { + panic(err) // the profiles are embedded at build time + } + names := make([]string, len(entries)) + for i, e := range entries { + names[i] = strings.TrimSuffix(e.Name(), ".toml") + } + slices.Sort(names) + return names +} + +// parse decodes and validates the profile in data, which was loaded from +// dir. name says where it came from in errors. +func parse(name string, data []byte, dir string) (*Profile, error) { + var p Profile + md, err := toml.Decode(string(data), &p) + if err != nil { + return nil, fmt.Errorf("%s: %w", name, err) + } + if undecoded := md.Undecoded(); len(undecoded) > 0 { + return nil, fmt.Errorf("%s: unknown keys %v", name, undecoded) + } + p.Dir = dir p.setDefaults() if err := p.validate(); err != nil { - return nil, fmt.Errorf("%s: %w", path, err) + return nil, fmt.Errorf("%s: %w", name, err) } return &p, nil } diff --git a/internal/profile/profile_test.go b/internal/profile/profile_test.go index ad765ec..bbd8d22 100644 --- a/internal/profile/profile_test.go +++ b/internal/profile/profile_test.go @@ -1,6 +1,8 @@ package profile import ( + "errors" + "io/fs" "os" "path/filepath" "strings" @@ -9,22 +11,64 @@ import ( ) func TestShippedProfilesLoad(t *testing.T) { - paths, err := filepath.Glob("../../profiles/*.toml") - if err != nil || len(paths) == 0 { - t.Fatalf("no profiles found (%v)", err) + names := Bundled() + if len(names) == 0 { + t.Fatal("no profiles found") } - for _, path := range paths { - p, err := Load(path) + for _, name := range names { + p, err := Find(name) if err != nil { - t.Errorf("%s: %v", path, err) + t.Errorf("%s: %v", name, err) continue } if p.Launch.StartupTimeout < time.Second { - t.Errorf("%s: startup-timeout parsed as %s", path, p.Launch.StartupTimeout) + t.Errorf("%s: startup-timeout parsed as %s", name, p.Launch.StartupTimeout) } if _, ok := p.Snippets["value"]; !ok { - t.Errorf("%s: no value snippet", path) + t.Errorf("%s: no value snippet", name) } + // A bundled profile has no directory to find a program in. + if strings.ContainsAny(p.Launch.Command[0], `/\`) { + t.Errorf("%s: %s isn't a program on the PATH", name, p.Launch.Command[0]) + } + } +} + +func TestFindingProfiles(t *testing.T) { + p, err := Find("babashka") + if err != nil { + t.Fatal(err) + } + if p.Name != "Babashka" { + t.Errorf("babashka found %q", p.Name) + } + if _, err := Find("../../profiles/jank.toml"); err != nil { + t.Errorf("a path to a profile: %v", err) + } + if _, err := Find("nope"); err == nil || !strings.Contains(err.Error(), "babashka, basilisp") { + t.Errorf("an unknown name should list the bundled profiles, got %v", err) + } + // A path is never taken for a name. + for _, path := range []string{"babashka.toml", "elsewhere/babashka"} { + if _, err := Find(path); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("%s: got %v, want a missing file", path, err) + } + } + // A name is never taken for a path, e.g. a checkout of the server. + dir := t.TempDir() + if err := os.Mkdir(filepath.Join(dir, "babashka"), 0o755); err != nil { + t.Fatal(err) + } + wd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(dir); err != nil { + t.Fatal(err) + } + defer os.Chdir(wd) + if _, err := Find("babashka"); err != nil { + t.Errorf("babashka next to a directory with that name: %v", err) } } diff --git a/internal/serve/matrix_test.go b/internal/serve/matrix_test.go index 4d461de..9ad145a 100644 --- a/internal/serve/matrix_test.go +++ b/internal/serve/matrix_test.go @@ -28,7 +28,7 @@ func serveFor(t *testing.T, scenarios ...string) *Server { func loadProfile(t *testing.T, name string) *profile.Profile { t.Helper() - p, err := profile.Load("../../profiles/" + name + ".toml") + p, err := profile.Find(name) if err != nil { t.Fatal(err) } @@ -141,11 +141,13 @@ func TestPresetsGetTheColumnsOfTheirServers(t *testing.T) { } } -// Each server is named by its profile, and the scenarios it has say they're -// what it does. +// Each server is named by its profile, every bundled profile is a server +// and the scenarios a server has say they're what it does. func TestPresetsMatchTheScenarios(t *testing.T) { + var names []string for _, p := range Presets() { - prof, err := profile.Load("../../profiles/" + p.Name + ".toml") + names = append(names, p.Name) + prof, err := profile.Find(p.Name) if err != nil { t.Errorf("%s isn't the name of a profile: %v", p.Name, err) continue @@ -162,4 +164,7 @@ func TestPresetsMatchTheScenarios(t *testing.T) { } } } + if !slices.Equal(slices.Sorted(slices.Values(names)), profile.Bundled()) { + t.Errorf("proof serve can behave like %v, but the bundled profiles are %v", names, profile.Bundled()) + } } diff --git a/profiles/profiles.go b/profiles/profiles.go new file mode 100644 index 0000000..0bc4eb9 --- /dev/null +++ b/profiles/profiles.go @@ -0,0 +1,10 @@ +// Package profiles holds the profiles of the servers in the compatibility +// matrix, so proof can use them by name wherever it's installed. +package profiles + +import "embed" + +// FS has the profiles, e.g. babashka.toml. +// +//go:embed *.toml +var FS embed.FS From 9ea8ad41c61c121f2efc487660eebe72f44adcef Mon Sep 17 00:00:00 2001 From: Bozhidar Batsov Date: Fri, 23 Oct 2026 16:04:20 +0300 Subject: [PATCH 3/3] Take flags after the profile and scenarios too The flag package stops at the first argument that isn't a flag, so proof run profiles/x.toml -only 'eval.*' complained that it expected one profile, and proof serve refused flags after a scenario. Each command now parses its flags wherever they are, and -h shows how to use it rather than Go's bare "Usage of run:". --- PLAN.md | 11 ++++---- cmd/proof/main.go | 58 ++++++++++++++++++++++++++++++++++++----- cmd/proof/proxy.go | 16 +++++------- cmd/proof/proxy_test.go | 29 +++++++++++++++++++++ cmd/proof/serve.go | 17 ++++-------- cmd/proof/serve_test.go | 4 ++- 6 files changed, 100 insertions(+), 35 deletions(-) diff --git a/PLAN.md b/PLAN.md index f6172c8..a31005f 100644 --- a/PLAN.md +++ b/PLAN.md @@ -43,12 +43,11 @@ lookups and completions, plus requests that go wrong. `proof serve -like babashka`, as the profiles are embedded in the binary. People who install proof with `go install` don't have the `profiles` folder at all. -- `proof run profiles/babashka.toml -only 'eval.*'` fails with - "expected one profile", as flags after the profile are taken for - arguments. proof should either accept them there or say that flags go - first. -- `proof run -h` prints Go's bare "Usage of run:", and the same goes for - `proxy` and `serve`. +- Done: `proof run profiles/babashka.toml -only 'eval.*'` works, as + flags can come after the profile (and after the scenarios of `proof + serve`). +- Done: `proof run -h` says how to use `proof run`, rather than Go's bare + "Usage of run:", and the same goes for `proxy` and `serve`. ### Releases diff --git a/cmd/proof/main.go b/cmd/proof/main.go index 3f4d107..aca58b4 100644 --- a/cmd/proof/main.go +++ b/cmd/proof/main.go @@ -4,6 +4,7 @@ package main import ( "context" + "errors" "flag" "fmt" "io" @@ -90,9 +91,51 @@ type options struct { var opts options -func runFlags(out io.Writer) *flag.FlagSet { - fs := flag.NewFlagSet("run", flag.ContinueOnError) +// newFlags makes the flag set of a command, whose -h says how to use it +// with the given arguments. +func newFlags(name, args string, out io.Writer) *flag.FlagSet { + fs := flag.NewFlagSet(name, flag.ContinueOnError) fs.SetOutput(out) + fs.Usage = func() { + fmt.Fprintf(fs.Output(), "Usage: proof %s [flags] %s\n\nFlags:\n", name, args) + fs.PrintDefaults() + } + return fs +} + +// parseArgs parses the flags in args wherever they are, as the flag +// package stops at the first argument that isn't one, and returns the +// other arguments. Everything after "--" is an argument. +func parseArgs(fs *flag.FlagSet, args []string) ([]string, error) { + var rest []string + for { + if err := fs.Parse(args); err != nil { + return nil, err + } + parsed := len(args) - fs.NArg() + if parsed > 0 && args[parsed-1] == "--" { + return append(rest, fs.Args()...), nil + } + args = fs.Args() + if len(args) == 0 { + return rest, nil + } + rest = append(rest, args[0]) + args = args[1:] + } +} + +// flagStatus is the exit status for an error from parseArgs, which has +// shown how to use the command already. Asking for that with -h is fine. +func flagStatus(err error) int { + if errors.Is(err, flag.ErrHelp) { + return 0 + } + return 2 +} + +func runFlags(out io.Writer) *flag.FlagSet { + fs := newFlags("run", "PROFILE", out) fs.StringVar(&opts.address, "address", "", "connect to a server already running at `host:port` instead of launching one") fs.StringVar(&opts.json, "json", "", "also write a JSON report to `file`") fs.StringVar(&opts.only, "only", "", "run only checks whose id matches the glob `pattern` (e.g. 'eval.*')") @@ -102,14 +145,15 @@ func runFlags(out io.Writer) *flag.FlagSet { func run(args []string) int { fs := runFlags(os.Stderr) - if err := fs.Parse(args); err != nil { - return 2 + args, err := parseArgs(fs, args) + if err != nil { + return flagStatus(err) } - if fs.NArg() != 1 { + if len(args) != 1 { fmt.Fprintln(os.Stderr, "proof run: expected one profile") return 2 } - p, err := profile.Find(fs.Arg(0)) + p, err := profile.Find(args[0]) if err != nil { fmt.Fprintln(os.Stderr, "proof:", err) return 2 @@ -118,7 +162,7 @@ func run(args []string) int { // Catch mistakes in the profile or flags before paying for a server // start, which can take minutes. if err := check.UnknownIDs(p.ExpectedFailures, allIDs()); err != nil { - fmt.Fprintf(os.Stderr, "proof: %s: %v\n", fs.Arg(0), err) + fmt.Fprintf(os.Stderr, "proof: %s: %v\n", args[0], err) return 2 } selected, err := filter(checks.All(), opts.only) diff --git a/cmd/proof/proxy.go b/cmd/proof/proxy.go index b6f74fd..03adaba 100644 --- a/cmd/proof/proxy.go +++ b/cmd/proof/proxy.go @@ -41,8 +41,7 @@ type proxyOptions struct { } func proxyFlags(out io.Writer, o *proxyOptions) *flag.FlagSet { - fs := flag.NewFlagSet("proxy", flag.ContinueOnError) - fs.SetOutput(out) + fs := newFlags("proxy", "[PROFILE]", out) fs.StringVar(&o.address, "address", "", "forward clients to a server already running at `host:port` instead of launching the one the profile describes") o.register(fs) return fs @@ -74,18 +73,18 @@ func proxyUntil(ctx context.Context, args []string, stdout, stderr io.Writer, li stderr = &syncWriter{w: stderr} var o proxyOptions fs := proxyFlags(stderr, &o) - if err := fs.Parse(args); err != nil { - return 2 + args, err := parseArgs(fs, args) + if err != nil { + return flagStatus(err) } - if fs.NArg() > 1 || (fs.NArg() == 0 && o.address == "") { + if len(args) > 1 || (len(args) == 0 && o.address == "") { fmt.Fprintln(stderr, "proof proxy: expected a profile or -address") return 2 } upstream, name := o.address, "the server" var p *profile.Profile - if fs.NArg() == 1 { - var err error - if p, err = profile.Find(fs.Arg(0)); err != nil { + if len(args) == 1 { + if p, err = profile.Find(args[0]); err != nil { fmt.Fprintln(stderr, "proof:", err) return 2 } @@ -98,7 +97,6 @@ func proxyUntil(ctx context.Context, args []string, stdout, stderr io.Writer, li var srv *server.Server if upstream == "" { fmt.Fprintf(stderr, "Starting %s...\n", p.Name) - var err error if srv, err = server.Start(ctx, p); err != nil { if ctx.Err() != nil { return 130 diff --git a/cmd/proof/proxy_test.go b/cmd/proof/proxy_test.go index 0727462..3751874 100644 --- a/cmd/proof/proxy_test.go +++ b/cmd/proof/proxy_test.go @@ -157,6 +157,35 @@ func TestProxyNeedsAServer(t *testing.T) { } } +func TestProxyHelp(t *testing.T) { + var stderr bytes.Buffer + if code := proxyUntil(context.Background(), []string{"-h"}, io.Discard, &stderr, nil); code != 0 { + t.Errorf("exit status %d, want 0", code) + } + if !strings.Contains(stderr.String(), "Usage: proof proxy [flags] [PROFILE]") { + t.Errorf("-h doesn't say how to use proof proxy:\n%s", stderr.String()) + } +} + +func TestFlagsCanFollowArguments(t *testing.T) { + var o proxyOptions + fs := proxyFlags(io.Discard, &o) + args, err := parseArgs(fs, []string{"-v", "babashka", "-listen", "127.0.0.1:7888", "extra", "-json", "r.json"}) + if err != nil { + t.Fatal(err) + } + if strings.Join(args, " ") != "babashka extra" || !o.verbose || o.listen != "127.0.0.1:7888" || o.json != "r.json" { + t.Errorf("got arguments %q and options %+v", args, o) + } + if _, err := parseArgs(fs, []string{"babashka", "-no-such-flag"}); err == nil { + t.Error("an unknown flag after an argument should be an error") + } + args, err = parseArgs(fs, []string{"a", "--", "-b", "c"}) + if err != nil || strings.Join(args, " ") != "a -b c" { + t.Errorf("everything after -- should be an argument, got %q (%v)", args, err) + } +} + func TestListShowsEverything(t *testing.T) { var buf bytes.Buffer list(&buf) diff --git a/cmd/proof/serve.go b/cmd/proof/serve.go index 55f4bab..1c67e8b 100644 --- a/cmd/proof/serve.go +++ b/cmd/proof/serve.go @@ -8,7 +8,6 @@ import ( "os" "os/signal" "path/filepath" - "slices" "strings" "syscall" "time" @@ -23,8 +22,7 @@ type serveOptions struct { } func serveFlags(out io.Writer, o *serveOptions) *flag.FlagSet { - fs := flag.NewFlagSet("serve", flag.ContinueOnError) - fs.SetOutput(out) + fs := newFlags("serve", "[SCENARIO...]", out) fs.StringVar(&o.like, "like", "", "behave like the server whose `profile` has this name (e.g. jank or profiles/jank.toml), along with any scenarios given") o.register(fs) return fs @@ -45,18 +43,13 @@ func serveUntil(ctx context.Context, args []string, stdout, stderr io.Writer, li stderr = &syncWriter{w: stderr} var o serveOptions fs := serveFlags(stderr, &o) - if err := fs.Parse(args); err != nil { - return 2 - } - scenarios, shown := fs.Args(), fs.Args() - // The flag package stops at the first scenario. - if i := slices.IndexFunc(scenarios, func(s string) bool { return strings.HasPrefix(s, "-") }); i >= 0 { - fmt.Fprintf(stderr, "proof: %s comes after a scenario, but flags have to go first\n", scenarios[i]) - return 2 + scenarios, err := parseArgs(fs, args) + if err != nil { + return flagStatus(err) } + shown := scenarios if o.like != "" { name := strings.TrimSuffix(filepath.Base(o.like), ".toml") - var err error if scenarios, err = serve.Like(name, scenarios); err != nil { fmt.Fprintln(stderr, "proof:", err) return 2 diff --git a/cmd/proof/serve_test.go b/cmd/proof/serve_test.go index 96a9aba..2e69a07 100644 --- a/cmd/proof/serve_test.go +++ b/cmd/proof/serve_test.go @@ -26,7 +26,9 @@ func TestServeExitCodes(t *testing.T) { "client traffic to proof serve (like jank)"}, {"like a server, but not quite", []string{"-like", "jank", "ns-error"}, nil, 2, "scenarios ns-fallback and ns-error don't go together"}, - {"flag after the scenarios", []string{"byte-writes", "-like", "jank"}, nil, 2, "-like comes after a scenario, but flags have to go first"}, + {"flag after the scenarios", []string{"byte-writes", "-like", "jank"}, []string{"d2:id1:12:op8:describee"}, 0, + "client traffic to proof serve (like jank, byte-writes)"}, + {"help", []string{"-h"}, nil, 0, "Usage: proof serve [flags] [SCENARIO...]"}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) {