Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UI panic and login errors on mintty/Windows #1561

Merged
merged 4 commits into from
Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cmd/login_cloud.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@ package cmd

import (
"os"
"syscall"

"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/afero"
"github.com/spf13/cobra"
"golang.org/x/crypto/ssh/terminal"
"gopkg.in/guregu/null.v3"

"github.com/loadimpact/k6/lib/consts"
Expand All @@ -51,6 +54,8 @@ This will set the default token used when just "k6 run -o cloud" is passed.`,
k6 login cloud`[1:],
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
// TODO: don't use a global... or maybe change the logger?
logger := logrus.StandardLogger()
fs := afero.NewOsFs()

k6Conf, err := getConsolidatedConfig(fs, Config{}, nil)
Expand Down Expand Up @@ -89,6 +94,9 @@ This will set the default token used when just "k6 run -o cloud" is passed.`,
},
},
}
if !terminal.IsTerminal(int(syscall.Stdin)) { // nolint: unconvert
logger.Warn("Stdin is not a terminal, falling back to plain text input")
}
vals, err := form.Run(os.Stdin, stdout)
if err != nil {
return err
Expand Down
8 changes: 8 additions & 0 deletions cmd/login_influxdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,14 @@ package cmd

import (
"os"
"syscall"
"time"

"github.com/mitchellh/mapstructure"
"github.com/sirupsen/logrus"
"github.com/spf13/afero"
"github.com/spf13/cobra"
"golang.org/x/crypto/ssh/terminal"

"github.com/loadimpact/k6/lib/types"
"github.com/loadimpact/k6/stats/influxdb"
Expand All @@ -42,6 +45,8 @@ var loginInfluxDBCommand = &cobra.Command{
This will set the default server used when just "-o influxdb" is passed.`,
Args: cobra.MaximumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
// TODO: don't use a global... or maybe change the logger?
logger := logrus.StandardLogger()
fs := afero.NewOsFs()
config, configPath, err := readDiskConfig(fs)
if err != nil {
Expand Down Expand Up @@ -80,6 +85,9 @@ This will set the default server used when just "-o influxdb" is passed.`,
},
},
}
if !terminal.IsTerminal(int(syscall.Stdin)) { // nolint: unconvert
logger.Warn("Stdin is not a terminal, falling back to plain text input")
}
vals, err := form.Run(os.Stdin, stdout)
if err != nil {
return err
Expand Down
23 changes: 17 additions & 6 deletions cmd/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ const (
maxLeftLength = 30
// Amount of padding in chars between rendered progress
// bar text and right-side terminal window edge.
termPadding = 1
termPadding = 1
defaultTermWidth = 80
)

// A writer that syncs writes with a mutex and, if the output is a TTY, clears before newlines.
Expand Down Expand Up @@ -252,10 +253,12 @@ func showProgress(
pbs = append(pbs, s.GetProgress())
}

var errTermGetSize bool
termWidth, _, err := terminal.GetSize(int(os.Stdout.Fd()))
if err != nil && stdoutTTY {
logger.WithError(err).Warn("error getting terminal size")
termWidth = 80 // TODO: something safer, return error?
termWidth = defaultTermWidth
errTermGetSize = true
}

// Get the longest left side string length, to align progress bars
Expand Down Expand Up @@ -330,12 +333,20 @@ func showProgress(
outMutex.Unlock()
return
case <-winch:
// More responsive progress bar resizing on platforms with SIGWINCH (*nix)
termWidth, _, _ = terminal.GetSize(fd)
if !errTermGetSize {
// More responsive progress bar resizing on platforms with SIGWINCH (*nix)
termWidth, _, err = terminal.GetSize(fd)
if err != nil {
Copy link
Contributor Author

@imiric imiric Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can ignore checking this err if it didn't fail the first time?

termWidth = defaultTermWidth
}
}
case <-ticker.C:
// Default ticker-based progress bar resizing
if winch == nil {
termWidth, _, _ = terminal.GetSize(fd)
if !errTermGetSize && winch == nil {
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
termWidth, _, err = terminal.GetSize(fd)
if err != nil {
termWidth = defaultTermWidth
}
}
}
renderProgressBars(true)
Expand Down
11 changes: 11 additions & 0 deletions ui/form_fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package ui

import (
"bufio"
"io"
"os"
"strings"
Expand Down Expand Up @@ -122,6 +123,16 @@ func (f PasswordField) GetContents(r io.Reader) (string, error) {
return "", errors.New("Cannot read password from the supplied terminal")
}
password, err := terminal.ReadPassword(int(stdin.Fd()))
if err != nil {
// Possibly running on Cygwin/mintty which doesn't emulate
// pseudo terminals properly, so fallback to plain text input.
// Note that passwords will be echoed if this is the case.
// See https://github.com/mintty/mintty/issues/56
// A workaround is to use winpty or mintty compiled with
// Cygwin >=3.1.0 which supports the new ConPTY Windows API.
bufR := bufio.NewReader(r)
password, err = bufR.ReadBytes('\n')
}
return string(password), err
}

Expand Down