diff --git a/go.mod b/go.mod index c5d52594..b869b035 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/spf13/cobra v1.7.0 github.com/spf13/pflag v1.0.5 github.com/spf13/viper v1.13.0 + go.uber.org/multierr v1.11.0 go.uber.org/zap v1.27.0 golang.org/x/term v0.18.0 gotest.tools/v3 v3.3.0 @@ -116,7 +117,6 @@ require ( github.com/xlab/treeprint v1.2.0 // indirect go.opencensus.io v0.24.0 // indirect go.starlark.net v0.0.0-20230525235612-a134d8f9ddca // indirect - go.uber.org/multierr v1.11.0 // indirect golang.org/x/mod v0.16.0 // indirect golang.org/x/net v0.22.0 // indirect golang.org/x/oauth2 v0.18.0 // indirect diff --git a/pkg/output/logging/context.go b/pkg/output/logging/context.go index 1b94ba01..534d7ca9 100644 --- a/pkg/output/logging/context.go +++ b/pkg/output/logging/context.go @@ -132,7 +132,7 @@ func createDefaultLogger(ctx context.Context) *zap.Logger { zapcore.AddSync(errout), lvl, )) - if !term.IsTerminal(errout) { + if !term.IsFancy(errout) { ec = zap.NewProductionEncoderConfig() logger = zap.New(zapcore.NewCore( zapcore.NewJSONEncoder(ec), diff --git a/pkg/output/term/io.go b/pkg/output/term/io.go index b481ebea..38cbcca1 100644 --- a/pkg/output/term/io.go +++ b/pkg/output/term/io.go @@ -30,11 +30,17 @@ func IsReaderTerminal(r io.Reader) bool { return ok && term.IsTerminal(int(f.Fd())) } -// IsTerminal returns true if the given writer is a real terminal. -func IsTerminal(w io.Writer) bool { +// IsWriterTerminal returns true if the given writer is a real terminal. +func IsWriterTerminal(w io.Writer) bool { + f, ok := w.(*os.File) + return ok && term.IsTerminal(int(f.Fd())) +} + +// IsFancy returns true if the given writer is a real terminal, or the color +// is forced by the environment variables. +func IsFancy(w io.Writer) bool { if environment.ColorIsForced() { return true } - f, ok := w.(*os.File) - return ok && environment.NonColorRequested() && term.IsTerminal(int(f.Fd())) + return !environment.NonColorRequested() && IsWriterTerminal(w) } diff --git a/pkg/output/tui/bubbletea964.go b/pkg/output/tui/bubbletea964.go new file mode 100644 index 00000000..76c2d628 --- /dev/null +++ b/pkg/output/tui/bubbletea964.go @@ -0,0 +1,49 @@ +/* + Copyright 2024 The Knative Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package tui + +import ( + "io" + "log" + "os" +) + +// safeguardBubbletea964 will safeguard the io.Reader by returning a new +// io.Reader that will prevent the +// https://github.com/charmbracelet/bubbletea/issues/964 issue. +// +// TODO: Remove this function once the issue is resolved. +func safeguardBubbletea964(in io.Reader) io.Reader { + if in == nil { + return nil + } + if f, ok := in.(*os.File); ok { + if st, err := f.Stat(); err != nil { + log.Fatal("unexpected: ", err) + } else { + if !st.Mode().IsRegular() { + if st.Name() != os.DevNull { + log.Println("WARN: non-regular file given as input: ", + st.Name(), " (mode: ", st.Mode(), + "). Using `nil` as input.") + } + return nil + } + } + } + return in +} diff --git a/pkg/output/tui/bubbletea964_test.go b/pkg/output/tui/bubbletea964_test.go new file mode 100644 index 00000000..4c66247b --- /dev/null +++ b/pkg/output/tui/bubbletea964_test.go @@ -0,0 +1,77 @@ +/* + Copyright 2024 The Knative Authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package tui + +import ( + "io" + "os" + "testing" + + "gotest.tools/v3/assert" +) + +func TestSafeguardBubbletea964(t *testing.T) { + t.Parallel() + tmp := t.TempDir() + assert.NilError(t, os.WriteFile(tmp+"/file", []byte("test"), 0o600)) + tf := openFile(t, tmp+"/file") + tcs := []safeguardBubbletea964TestCase{{ + name: "nil input", + in: nil, + want: nil, + }, { + name: "non-regular file", + in: os.NewFile(0, "/"), + want: nil, + }, { + name: "regular file", + in: tf, + want: tf, + }, { + name: "dev null", + in: openFile(t, os.DevNull), + want: nil, + }} + for _, tc := range tcs { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if tf, ok := tc.in.(*os.File); ok { + defer func(tf *os.File) { + _ = tf.Close() + }(tf) + } + + got := safeguardBubbletea964(tc.in) + assert.Equal(t, got, tc.want) + }) + + } +} + +type safeguardBubbletea964TestCase struct { + name string + in io.Reader + want io.Reader +} + +func openFile(tb testing.TB, name string) *os.File { + tb.Helper() + f, err := os.Open(name) + assert.NilError(tb, err) + return f +} diff --git a/pkg/output/tui/progress.go b/pkg/output/tui/progress.go index cb26f70f..51d66b67 100644 --- a/pkg/output/tui/progress.go +++ b/pkg/output/tui/progress.go @@ -26,6 +26,7 @@ import ( "github.com/charmbracelet/bubbles/progress" tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" + "go.uber.org/multierr" "knative.dev/client-pkg/pkg/output" "knative.dev/client-pkg/pkg/output/term" ) @@ -79,12 +80,16 @@ type BubbleProgress struct { prevSpeed []int err error quitChan chan struct{} + teaErr error } func (b *BubbleProgress) With(fn func(ProgressControl) error) error { b.start() - defer b.stop() - return fn(b) + err := func() error { + defer b.stop() + return fn(b) + }() + return multierr.Combine(err, b.err, b.teaErr) } func (b *BubbleProgress) Error(err error) { @@ -238,15 +243,17 @@ func (b *BubbleProgress) start() { b.prog = progress.New(progress.WithDefaultGradient()) out := b.OutOrStdout() b.tea = tea.NewProgram(b, - tea.WithInput(b.InOrStdin()), + tea.WithInput(safeguardBubbletea964(b.InOrStdin())), tea.WithOutput(out), ) b.quitChan = make(chan struct{}) go func() { t := b.tea - _, _ = t.Run() + if _, err := t.Run(); err != nil { + b.teaErr = err + } close(b.quitChan) - if term.IsTerminal(out) { + if term.IsWriterTerminal(out) { if err := t.ReleaseTerminal(); err != nil { panic(err) } diff --git a/pkg/output/tui/progress_test.go b/pkg/output/tui/progress_test.go index 29e482cd..82d06aea 100644 --- a/pkg/output/tui/progress_test.go +++ b/pkg/output/tui/progress_test.go @@ -33,9 +33,6 @@ import ( ) func TestProgress(t *testing.T) { - if testing.Short() { - t.Skip("skipping test in short mode.") - } ctx := context.TestContext(t) prt := output.NewTestPrinter() ctx = output.WithContext(ctx, prt) diff --git a/pkg/output/tui/spinner.go b/pkg/output/tui/spinner.go index 438ef358..9c0665bb 100644 --- a/pkg/output/tui/spinner.go +++ b/pkg/output/tui/spinner.go @@ -22,6 +22,7 @@ import ( "github.com/charmbracelet/bubbles/spinner" tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" + "go.uber.org/multierr" "knative.dev/client-pkg/pkg/output" "knative.dev/client-pkg/pkg/output/term" ) @@ -46,12 +47,16 @@ type BubbleSpinner struct { spin spinner.Model tea *tea.Program quitChan chan struct{} + teaErr error } func (b *BubbleSpinner) With(fn func(Spinner) error) error { b.start() - defer b.stop() - return fn(b) + err := func() error { + defer b.stop() + return fn(b) + }() + return multierr.Combine(err, b.teaErr) } func (b *BubbleSpinner) Init() tea.Cmd { @@ -75,15 +80,17 @@ func (b *BubbleSpinner) start() { ) out := b.OutOrStdout() b.tea = tea.NewProgram(b, - tea.WithInput(b.InOrStdin()), + tea.WithInput(safeguardBubbletea964(b.InOrStdin())), tea.WithOutput(out), ) b.quitChan = make(chan struct{}) go func() { t := b.tea - _, _ = t.Run() + if _, err := t.Run(); err != nil { + b.teaErr = err + } close(b.quitChan) - if term.IsTerminal(out) { + if term.IsWriterTerminal(out) { _ = t.ReleaseTerminal() } }() diff --git a/pkg/output/tui/spinner_test.go b/pkg/output/tui/spinner_test.go index 6e102ecc..343f44be 100644 --- a/pkg/output/tui/spinner_test.go +++ b/pkg/output/tui/spinner_test.go @@ -26,9 +26,6 @@ import ( ) func TestSpinner(t *testing.T) { - if testing.Short() { - t.Skip("skipping test in short mode.") - } ctx := context.TestContext(t) prt := output.NewTestPrinter() ctx = output.WithContext(ctx, prt)