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

Disable terminal echo while State Tool is running. #2882

Merged
merged 4 commits into from
Nov 15, 2023
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
7 changes: 7 additions & 0 deletions cmd/state/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,13 @@ func run(args []string, isInteractive bool, cfg *config.Instance, out output.Out

// Set up conditional, which accesses a lot of primer data
sshell := subshell.New(cfg)
if isInteractive {
// Disable terminal echo while State Tool is running.
// Other than in prompts and subshells (which temporarily re-enable echo), user typing should
// not interfere with output (e.g. runtime progress bars).
sshell.TurnOffEcho()
defer sshell.TurnOnEcho()
Comment on lines +211 to +215
Copy link
Member

Choose a reason for hiding this comment

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

Does this also disable ctrl+c and other similar inputs? I don't believe that's something we want to disallow.

Copy link
Member

Choose a reason for hiding this comment

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

Also, is it possible that if the state tool errors mid-execution, we could leave the user's shell in a state where they cannot echo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You didn't see this, but in my original PR (#2768), I noted that echo is restored after ^C, so no problem there. The defer does its job.

If there is a hard crash, echo will probably be lost, but this is the case for any terminal program that messes with termios, so the only resolution is for the user to exit this terminal and start up a new one.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the defer will run even on a panic as long as it's on the function call stack so we should be good even in the event of a crash as this defer is placed ahead of the actual command logic.

}

conditional := constraints.NewPrimeConditional(auth, pj, sshell.Shell())
project.RegisterConditional(conditional)
Expand Down
2 changes: 2 additions & 0 deletions internal/runbits/activation/activation.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ func ActivateAndWait(
if err := ss.Activate(proj, cfg, out); err != nil {
return locale.WrapError(err, "error_could_not_activate_subshell", "Could not activate a new subshell.")
}
ss.TurnOnEcho() // temporarily re-enable echo while the subshell is active
defer ss.TurnOffEcho()

a, err := process.NewActivation(cfg, os.Getpid())
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/runbits/runtime/progress/progress.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ import (
"sync"
"time"

"github.com/ActiveState/cli/internal/multilog"
"github.com/go-openapi/strfmt"
"github.com/vbauerster/mpb/v7"
"golang.org/x/net/context"

"github.com/ActiveState/cli/internal/errs"
"github.com/ActiveState/cli/internal/locale"
"github.com/ActiveState/cli/internal/logging"
"github.com/ActiveState/cli/internal/multilog"
"github.com/ActiveState/cli/internal/output"
"github.com/ActiveState/cli/pkg/platform/runtime/artifact"
"github.com/ActiveState/cli/pkg/platform/runtime/setup/events"
Expand Down
15 changes: 15 additions & 0 deletions internal/subshell/bash/bash.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/ActiveState/cli/internal/osutils/user"
"github.com/ActiveState/cli/internal/output"
"github.com/ActiveState/cli/internal/subshell/sscommon"
"github.com/ActiveState/cli/internal/subshell/termecho"
"github.com/ActiveState/cli/pkg/project"
)

Expand Down Expand Up @@ -219,3 +220,17 @@ func (v *SubShell) IsAvailable() bool {
}
return fileutils.FileExists(rcFile)
}

func (v *SubShell) TurnOffEcho() {
if runtime.GOOS == "windows" {
return // not supported
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows does not have access to the termios functions needed to disable echo in emulated shells like Git Bash (which is not interactive anyway).

}
termecho.Off()
}

func (v *SubShell) TurnOnEcho() {
if runtime.GOOS == "windows" {
return // not supported
}
termecho.On()
}
9 changes: 9 additions & 0 deletions internal/subshell/cmd/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/ActiveState/cli/internal/osutils"
"github.com/ActiveState/cli/internal/output"
"github.com/ActiveState/cli/internal/subshell/sscommon"
"github.com/ActiveState/cli/internal/subshell/termecho"
"github.com/ActiveState/cli/pkg/project"
)

Expand Down Expand Up @@ -203,3 +204,11 @@ func (v *SubShell) IsActive() bool {
func (v *SubShell) IsAvailable() bool {
return runtime.GOOS == "windows"
}

func (v *SubShell) TurnOffEcho() {
termecho.Off()
}

func (v *SubShell) TurnOnEcho() {
termecho.On()
}
16 changes: 16 additions & 0 deletions internal/subshell/fish/fish.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"os"
"os/exec"
"path/filepath"
"runtime"

"github.com/ActiveState/cli/internal/constants"
"github.com/ActiveState/cli/internal/errs"
Expand All @@ -14,6 +15,7 @@ import (
"github.com/ActiveState/cli/internal/osutils/user"
"github.com/ActiveState/cli/internal/output"
"github.com/ActiveState/cli/internal/subshell/sscommon"
"github.com/ActiveState/cli/internal/subshell/termecho"
"github.com/ActiveState/cli/pkg/project"
)

Expand Down Expand Up @@ -195,3 +197,17 @@ func (v *SubShell) IsAvailable() bool {
}
return fileutils.FileExists(rcFile)
}

func (v *SubShell) TurnOffEcho() {
if runtime.GOOS == "windows" {
return // not supported
}
termecho.Off()
}

func (v *SubShell) TurnOnEcho() {
if runtime.GOOS == "windows" {
return // not supported
}
termecho.On()
}
6 changes: 6 additions & 0 deletions internal/subshell/subshell.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ type SubShell interface {

// IsAvailable returns whether the shell is available on the system
IsAvailable() bool

// TurnOffEcho turns off input echoing.
TurnOffEcho()

// TurnOnEcho turns on input echoing.
TurnOnEcho()
}

// New returns the subshell relevant to the current process, but does not activate it
Expand Down
16 changes: 16 additions & 0 deletions internal/subshell/tcsh/tcsh.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"os"
"os/exec"
"path/filepath"
"runtime"

"github.com/ActiveState/cli/internal/errs"
"github.com/ActiveState/cli/internal/fileutils"
Expand All @@ -13,6 +14,7 @@ import (
"github.com/ActiveState/cli/internal/osutils/user"
"github.com/ActiveState/cli/internal/output"
"github.com/ActiveState/cli/internal/subshell/sscommon"
"github.com/ActiveState/cli/internal/subshell/termecho"
"github.com/ActiveState/cli/pkg/project"
)

Expand Down Expand Up @@ -189,3 +191,17 @@ func (v *SubShell) IsAvailable() bool {
}
return fileutils.FileExists(rcFile)
}

func (v *SubShell) TurnOffEcho() {
if runtime.GOOS == "windows" {
return // not supported
}
termecho.Off()
}

func (v *SubShell) TurnOnEcho() {
if runtime.GOOS == "windows" {
return // not supported
}
termecho.On()
}
20 changes: 20 additions & 0 deletions internal/subshell/termecho/termecho.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package termecho

import (
"github.com/ActiveState/cli/internal/errs"
"github.com/ActiveState/cli/internal/multilog"
)

func Off() {
err := toggle(false)
if err != nil {
multilog.Error("Unable to turn off terminal echoing: %v", errs.JoinMessage(err))
}
}

func On() {
err := toggle(true)
if err != nil {
multilog.Error("Unable to turn off terminal echoing: %v", errs.JoinMessage(err))
}
}
6 changes: 6 additions & 0 deletions internal/subshell/termecho/termecho_darwin.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package termecho

import "golang.org/x/sys/unix"

const ioctlReadTermios = unix.TIOCGETA
const ioctlWriteTermios = unix.TIOCSETA
6 changes: 6 additions & 0 deletions internal/subshell/termecho/termecho_linux.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package termecho

import "golang.org/x/sys/unix"

const ioctlReadTermios = unix.TCGETS
const ioctlWriteTermios = unix.TCSETS
32 changes: 32 additions & 0 deletions internal/subshell/termecho/termecho_unix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//go:build linux || darwin
// +build linux darwin

package termecho

import (
"os"

"github.com/ActiveState/cli/internal/errs"
"golang.org/x/sys/unix"
)

func toggle(on bool) error {
fd := int(os.Stdin.Fd())
termios, err := unix.IoctlGetTermios(fd, ioctlReadTermios)
if err != nil {
return errs.Wrap(err, "Could not get termios")
}

newState := *termios // copy
if !on {
newState.Lflag &^= unix.ECHO
} else {
newState.Lflag |= unix.ECHO
}
err = unix.IoctlSetTermios(fd, ioctlWriteTermios, &newState)
if err != nil {
return errs.Wrap(err, "Could not set termios")
}

return nil
}
30 changes: 30 additions & 0 deletions internal/subshell/termecho/termecho_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package termecho

import (
"os"

"github.com/ActiveState/cli/internal/errs"
"golang.org/x/sys/windows"
)

func toggle(on bool) error {
fd := windows.Handle(os.Stdin.Fd())
var mode uint32
err := windows.GetConsoleMode(fd, &mode)
if err != nil {
return errs.Wrap(err, "Error calling GetConsoleMode")
}

newMode := mode
if !on {
newMode &^= windows.ENABLE_ECHO_INPUT
} else {
newMode |= windows.ENABLE_ECHO_INPUT
}
err = windows.SetConsoleMode(fd, newMode)
if err != nil {
return errs.Wrap(err, "Error calling SetConsoleMode")
}

return nil
}
16 changes: 16 additions & 0 deletions internal/subshell/zsh/zsh.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"

"github.com/ActiveState/cli/internal/constants"
Expand All @@ -18,6 +19,7 @@ import (
"github.com/ActiveState/cli/internal/osutils/user"
"github.com/ActiveState/cli/internal/output"
"github.com/ActiveState/cli/internal/subshell/sscommon"
"github.com/ActiveState/cli/internal/subshell/termecho"
"github.com/ActiveState/cli/pkg/project"
)

Expand Down Expand Up @@ -239,3 +241,17 @@ func (v *SubShell) IsAvailable() bool {
}
return fileutils.FileExists(rcFile)
}

func (v *SubShell) TurnOffEcho() {
if runtime.GOOS == "windows" {
return // not supported
}
termecho.Off()
}

func (v *SubShell) TurnOnEcho() {
if runtime.GOOS == "windows" {
return // not supported
}
termecho.On()
}
Loading