diff --git a/cmd/summer/runtime.go b/cmd/summer/runtime.go index b6127156..33b4fe0a 100644 --- a/cmd/summer/runtime.go +++ b/cmd/summer/runtime.go @@ -150,13 +150,14 @@ func runAppBinary(ctx context.Context, argv []string, out bonfire.Output) error // streams, so it sees the terminal: it asks the production question, prints // styled summaries and animates its widgets. Otherwise its stdout goes // through out (tests and bonfire.Call roots capture it) and its stderr to -// summer's stderr, so log lines and error blocks never mix into data such -// as --json output. +// the root's error stream (bonfire.ErrWriter), which is os.Stderr for the +// real summer, so log lines and error blocks never mix into data such as +// --json output, and a root built on buffers captures them. func appStreams(out bonfire.Output, terminal bool) (stdout, stderr io.Writer) { if terminal { return os.Stdout, os.Stderr } - return out, os.Stderr + return out, bonfire.ErrWriter(out) } // runAppCmd runs cmd, the app binary started with argv, forwarding SIGINT diff --git a/cmd/summer/runtime_test.go b/cmd/summer/runtime_test.go index c6345959..8b637454 100644 --- a/cmd/summer/runtime_test.go +++ b/cmd/summer/runtime_test.go @@ -346,6 +346,11 @@ func TestDelegateExitCodePassthrough(t *testing.T) { if code != 2 { t.Errorf("exit %d, want the binary's 2", code) } + // IN-13: the binary's own error block reaches the root's error + // writer. + if n := strings.Count(errOut, "ERROR the app binary failed"); n != 1 { + t.Errorf("the binary's error block reached the root's error writer %d times, want 1:\nstdout %q\nstderr %q", n, out, errOut) + } for _, s := range []string{out, errOut} { if strings.Contains(s, " ERROR ") || strings.Contains(s, "summer: migrate") { t.Errorf("summer printed an error block of its own:\nstdout %q\nstderr %q", out, errOut) @@ -419,9 +424,9 @@ func TestRunAppCmdMasksSecrets(t *testing.T) { }) } -// TestDelegateStreams (CR-03): off a terminal the app binary's stdout goes -// through summer's Output and its stderr to summer's stderr, never into -// summer's stdout. +// TestDelegateStreams (CR-03, IN-13): off a terminal the app binary's +// stdout goes through summer's Output and its stderr to the root's error +// writer (bonfire.ErrWriter), never into summer's stdout. func TestDelegateStreams(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("the fake app binary is a shell script") @@ -438,23 +443,42 @@ func TestDelegateStreams(t *testing.T) { t.Fatal(err) } t.Chdir(app) - out, err := runSummer(t, "route:list", "--json") + var stdout, stderr syncBuffer + root, err := bonfire.NewRootIO("summer", toolCommands(), strings.NewReader(""), &stdout, &stderr) if err != nil { - t.Fatalf("route:list: %v\n%s", err, out) + t.Fatal(err) } - if !strings.Contains(out, "data-line") { - t.Errorf("the binary's stdout did not reach summer's Output: %q", out) + if code := bonfire.Execute(context.Background(), root, []string{"route:list", "--json"}); code != bonfire.ExitOK { + t.Fatalf("route:list: exit %d\nstdout %q\nstderr %q", code, stdout.String(), stderr.String()) } - if strings.Contains(out, "log-line") { - t.Errorf("the binary's stderr landed in summer's stdout: %q", out) + if !strings.Contains(stdout.String(), "data-line") { + t.Errorf("the binary's stdout did not reach summer's Output: %q", stdout.String()) + } + if strings.Contains(stdout.String(), "log-line") { + t.Errorf("the binary's stderr landed in summer's stdout: %q", stdout.String()) + } + if !strings.Contains(stderr.String(), "log-line") { + t.Errorf("the binary's stderr did not reach the root's error writer: %q", stderr.String()) } } -// TestAppStreams (CR-03): on a terminal the app binary gets summer's own -// stdout and stderr; off a terminal its stdout goes through the Output and -// its stderr to summer's stderr. +// TestAppStreams (CR-03, IN-13): on a terminal the app binary gets +// summer's own stdout and stderr; off a terminal its stdout goes through the +// Output and its stderr to the Output's error writer (bonfire.ErrWriter), +// which is os.Stderr for the real summer. func TestAppStreams(t *testing.T) { - var buf bytes.Buffer + var buf, errBuf bytes.Buffer + sep := bonfire.NewOutput(strings.NewReader(""), &buf, &errBuf) + if _, stderr := appStreams(sep, false); stderr != io.Writer(&errBuf) { + t.Errorf("not a terminal: stderr %T, want the Output's error writer", stderr) + } + real, err := bonfire.NewRoot("summer", nil, os.Stdout) + if err != nil { + t.Fatal(err) + } + if real.ErrOrStderr() != os.Stderr { + t.Errorf("the real summer root's error stream is %v, want os.Stderr", real.ErrOrStderr()) + } out := bonfire.NewOutput(strings.NewReader(""), &buf, &buf) if bonfire.IsTerminal(out) { t.Fatal("a buffer console reports a terminal") @@ -464,7 +488,7 @@ func TestAppStreams(t *testing.T) { t.Errorf("terminal: streams %v, %v; want os.Stdout, os.Stderr", stdout, stderr) } stdout, stderr = appStreams(out, false) - if stdout != io.Writer(out) || stderr != os.Stderr { - t.Errorf("not a terminal: streams %v, %v; want the Output, os.Stderr", stdout, stderr) + if stdout != io.Writer(out) || stderr != io.Writer(&buf) { + t.Errorf("not a terminal: streams %T, %T; want the Output and its error writer", stdout, stderr) } } diff --git a/modules/bonfire/README.md b/modules/bonfire/README.md index bc72a312..4ead38f3 100644 --- a/modules/bonfire/README.md +++ b/modules/bonfire/README.md @@ -38,7 +38,7 @@ Use cases: | `-q` | none | info, success and warning lines hidden; errors kept | printed (`Println`, `Printf`, tables) | defaults, as with `-n` | | `--no-ansi` | none | symbols, no colors | printed | plain line prompts | | `-n` | none | as on the terminal | printed | defaults; `Secret` reads piped stdin or fails with a hint | -- Command state for helpers: `bonfire.GlobalsOf`, `bonfire.VerbosityOf`, `bonfire.IsQuiet` and `bonfire.Interactive` read the running command's global options from its `bonfire.Output`. +- Command state for helpers: `bonfire.GlobalsOf`, `bonfire.VerbosityOf`, `bonfire.IsQuiet` and `bonfire.Interactive` read the running command's global options from its `bonfire.Output`, and `bonfire.ErrWriter` returns the stream its root's error block goes to (stderr for the real binary, the injected error writer for a root built on buffers). - Passthrough commands (`bonfire.Command.Passthrough`) skip flag parsing, so every argument, including `--help`, reaches `bonfire.Input.Args` unchanged; this suits commands that forward to another program. `bonfire.Command.Long` adds extended help text that the help command shows instead of the description. - Artisan-style command list: a bare binary (or `--help`, or `list`) prints a version header (`bonfire.Version.Header`, read from the build information by `bonfire.BuildVersion`), the usage line, the global options and the commands, top-level names first and then one block per namespace, with aligned descriptions. - Global options on every command: `-v`/`-vv`/`-vvv` (`--verbose`), `-q` (`--quiet`, implies `-n`), `--ansi`/`--no-ansi` (last wins), `-n` (`--no-interaction`) and `--env`. `bonfire.PeekGlobals` reads them from the raw arguments before the root exists, so the generated main can pick the configuration environment and the log verbosity before plugins boot; it reads the `=value` forms the way pflag does (`--verbose=2`, `-v=3`, `--quiet=false`, `--ansi=false`), and a value-taking command flag followed by a global-looking token (`--name -v`) is read as the global before boot, while the command and its error block use cobra's reading; `bonfire.OnGlobals` hooks receive the parsed `bonfire.Globals` before every command runs (roots built by `bonfire.Call` run no hook). @@ -160,6 +160,7 @@ if catalog.Has("blog:import") { | `bonfire.VerbosityOf` | The `-v` level of the command an Output belongs to. | | `bonfire.IsQuiet` | Whether the command runs with `-q`. | | `bonfire.IsTerminal` | Whether the Output writes to a terminal; long-running commands print a summary block for a person there and log records otherwise. | +| `bonfire.ErrWriter` | The stream the root's error block goes to for the command an Output belongs to: `os.Stderr` for `bonfire.NewRoot` on `os.Stdout`, the error writer of `bonfire.NewRootIO` and `bonfire.NewOutput`, `os.Stderr` for other Output implementations. summer sends a delegated binary's stderr there. | | `bonfire.Details`, `bonfire.Detail` | Prints a title and aligned label/value lines in the palette (a `Warn` value in the warning color); nothing under `-q`. Used for startup summaries and shutdown reports. | | `bonfire.Interactive` | Whether the Output can ask a person: stdin and stdout are terminals and neither `-n` nor `-q` is set. | | `bonfire.ConfirmProceed` | Asks before a destructive step on an interactive terminal; returns true without asking otherwise. | diff --git a/modules/bonfire/helpers.go b/modules/bonfire/helpers.go index abce5fcb..0f12d50c 100644 --- a/modules/bonfire/helpers.go +++ b/modules/bonfire/helpers.go @@ -3,6 +3,8 @@ package bonfire import ( "context" "errors" + "io" + "os" "strings" tea "charm.land/bubbletea/v2" @@ -15,6 +17,7 @@ type consoleState interface { consoleGlobals() Globals consoleInteractive() bool consoleTerminal() bool + consoleErrWriter() io.Writer } func (c *console) consoleGlobals() Globals { return c.globals } @@ -23,6 +26,13 @@ func (c *console) consoleInteractive() bool { return c.canPrompt() } func (c *console) consoleTerminal() bool { return c.tty } +func (c *console) consoleErrWriter() io.Writer { + if c.errStream != nil { + return c.errStream + } + return c.err +} + // GlobalsOf returns the global options of the command out belongs to, or // the zero Globals when out is not a bonfire console. func GlobalsOf(out Output) Globals { @@ -63,6 +73,21 @@ func IsTerminal(out Output) bool { return false } +// ErrWriter returns the stream the root's error block goes to for the +// command out belongs to: os.Stderr for a root built by NewRoot on +// os.Stdout, the error writer given to NewRootIO or NewOutput (stdout when +// NewOutput gets none), and os.Stderr for an Output that is not a bonfire +// console. summer sends an app binary's stderr there, so a root built on +// buffers captures it while the real summer keeps it on stderr. +func ErrWriter(out Output) io.Writer { + if s, ok := out.(consoleState); ok { + if w := s.consoleErrWriter(); w != nil { + return w + } + } + return os.Stderr +} + // Detail is one line of a Details block: a label and its value. Warn // prints the value in the warning color. type Detail struct { diff --git a/modules/bonfire/output.go b/modules/bonfire/output.go index 8c6901bc..8c639d10 100644 --- a/modules/bonfire/output.go +++ b/modules/bonfire/output.go @@ -44,9 +44,12 @@ type Progress interface { } type console struct { - in io.Reader - out io.Writer - err io.Writer + in io.Reader + out io.Writer + err io.Writer + // errStream is the stream the root's error block goes to, when the + // console belongs to a root (ErrWriter); nil means err. + errStream io.Writer lookup func(string) string // tty is true when stdout is a terminal; it picks animated widgets // and boxed tables. diff --git a/modules/bonfire/output_test.go b/modules/bonfire/output_test.go index fc80c63b..1f66b22e 100644 --- a/modules/bonfire/output_test.go +++ b/modules/bonfire/output_test.go @@ -4,9 +4,13 @@ import ( "bytes" "context" "errors" + "io" + "os" "slices" "strings" "testing" + + "github.com/spf13/cobra" ) func TestNonTTYSpinner(t *testing.T) { @@ -387,3 +391,51 @@ func TestHelpUsesSharedAdapter(t *testing.T) { t.Fatalf("help = %q", got) } } + +// stubOutput is an Output that is not a bonfire console. +type stubOutput struct{ Output } + +// TestErrWriter (IN-13): ErrWriter returns the stream the root's error +// block goes to: os.Stderr for NewRoot on os.Stdout, the error writer of +// NewRootIO and NewOutput (stdout when NewOutput gets none), and os.Stderr +// for an Output that is not a bonfire console. +func TestErrWriter(t *testing.T) { + outputOf := func(t *testing.T, build func([]Command) (*cobra.Command, error)) Output { + t.Helper() + var got Output + cmd := Command{Name: "acme:grab", Description: "Grab the output", Run: func(_ context.Context, _ Input, out Output) error { + got = out + return nil + }} + root, err := build([]Command{cmd}) + if err != nil { + t.Fatal(err) + } + root.SetArgs([]string{"acme:grab"}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + return got + } + + out := outputOf(t, func(c []Command) (*cobra.Command, error) { return NewRoot("acme", c, os.Stdout) }) + if w := ErrWriter(out); w != os.Stderr { + t.Errorf("NewRoot on os.Stdout: ErrWriter = %v, want os.Stderr", w) + } + var stdout, stderr bytes.Buffer + out = outputOf(t, func(c []Command) (*cobra.Command, error) { + return NewRootIO("acme", c, strings.NewReader(""), &stdout, &stderr) + }) + if w := ErrWriter(out); w != io.Writer(&stderr) { + t.Errorf("NewRootIO: ErrWriter = %v, want the error writer", w) + } + if w := ErrWriter(NewOutput(strings.NewReader(""), &stdout, &stderr)); w != io.Writer(&stderr) { + t.Errorf("NewOutput: ErrWriter = %v, want the error writer", w) + } + if w := ErrWriter(NewOutput(strings.NewReader(""), &stdout, nil)); w != io.Writer(&stdout) { + t.Errorf("NewOutput without an error writer: ErrWriter = %v, want stdout", w) + } + if w := ErrWriter(stubOutput{}); w != os.Stderr { + t.Errorf("a non-bonfire Output: ErrWriter = %v, want os.Stderr", w) + } +} diff --git a/modules/bonfire/root.go b/modules/bonfire/root.go index 3ff94141..1782ff0b 100644 --- a/modules/bonfire/root.go +++ b/modules/bonfire/root.go @@ -53,18 +53,26 @@ func OnGlobals(fn func(Globals)) RootOption { // It accepts command values rather than a party registry to avoid an import cycle. // When out is the process stdout, the error block of Execute goes to stderr. func NewRoot(name string, commands []Command, out io.Writer, opts ...RootOption) (*cobra.Command, error) { - root, err := NewRootIO(name, commands, os.Stdin, out, out, opts...) + root, console, err := newRootIO(name, commands, os.Stdin, out, out, opts...) if err != nil { return nil, err } if out == nil || out == os.Stdout { root.SetErr(os.Stderr) + console.errStream = os.Stderr } return root, nil } // NewRootIO is NewRoot with injected stdin/stdout/stderr for tests. func NewRootIO(name string, commands []Command, in io.Reader, out, errW io.Writer, opts ...RootOption) (*cobra.Command, error) { + root, _, err := newRootIO(name, commands, in, out, errW, opts...) + return root, err +} + +// newRootIO builds the root of NewRootIO and returns its console too, so +// NewRoot can record where it moved the error stream. +func newRootIO(name string, commands []Command, in io.Reader, out, errW io.Writer, opts ...RootOption) (*cobra.Command, *console, error) { if out == nil { out = os.Stdout } @@ -84,6 +92,7 @@ func NewRootIO(name string, commands []Command, in io.Reader, out, errW io.Write cfg.version = BuildVersion() } output := newConsole(in, out, errW, os.Getenv, nil, nil) + output.errStream = errW root := &cobra.Command{ Use: name, SilenceUsage: true, @@ -116,24 +125,24 @@ func NewRootIO(name string, commands []Command, in io.Reader, out, errW io.Write } for _, command := range commands { if seen[command.Name] { - return nil, fmt.Errorf("%w: %q", ErrDuplicateCommand, command.Name) + return nil, nil, fmt.Errorf("%w: %q", ErrDuplicateCommand, command.Name) } seen[command.Name] = true cmd, err := st.wrap(command, true) if err != nil { - return nil, err + return nil, nil, err } root.AddCommand(cmd) } listCmd, err := st.wrap(st.listCommand(), false) if err != nil { - return nil, err + return nil, nil, err } root.AddCommand(listCmd) helpCmd, err := st.wrap(st.helpCommand(), false) if err != nil { - return nil, err + return nil, nil, err } root.SetHelpCommand(helpCmd) root.SetHelpFunc(func(*cobra.Command, []string) { @@ -160,7 +169,7 @@ func NewRootIO(name string, commands []Command, in io.Reader, out, errW io.Write } return nil } - return root, nil + return root, output, nil } // rootState is what the list, help and error rendering of one root share. diff --git a/scripts/check-phase19.8.sh b/scripts/check-phase19.8.sh index d32953af..9545f558 100755 --- a/scripts/check-phase19.8.sh +++ b/scripts/check-phase19.8.sh @@ -136,8 +136,9 @@ BONFIRE_TESTS=( # quick 261028-rqd (info findings): IN-03, IN-06, IN-02, IN-04. TestPluginErrorWordedUnknownCommand TestExecuteKeepsRootHelp TestErrorBlockUsesParsedGlobals TestFlagBool - # quick 261028-rqd: IN-11, IN-10, IN-12. + # quick 261028-rqd: IN-11, IN-10, IN-12, IN-13. TestPrintAboveBlankLine TestErrorPrintsAboveView TestSignalRelayWaitsForStart + TestErrWriter ) DOCSITE_TESTS=( # Plan 04: the docs checker reads past the global options.