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.

Sign up for GitHub

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

Disable terminal echo while State Tool is running. #2882

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a4ad8f
Turn off terminal echo while progressbar is present.
mitchell-as Sep 20, 2023
e693344
Merge branch 'mitchell/dx-2050' into mitchell/dx-2214-2
mitchell-as Nov 13, 2023
7c45563
Disable terminal echo while State Tool is running.
mitchell-as Nov 13, 2023
750654a
Fixed typo.
mitchell-as Nov 14, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.Off()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these, and the other TurnOnEcho functions be calling termecho.On()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, how embarrassing. Thanks for catching this.

}
9 changes: 9 additions & 0 deletions internal/subshell/cmd/cmd.go
View file
Open in desktop
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.Off()
}
16 changes: 16 additions & 0 deletions internal/subshell/fish/fish.go
View file
Open in desktop
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.Off()
}
6 changes: 6 additions & 0 deletions internal/subshell/subshell.go
View file
Open in desktop
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
View file
Open in desktop
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.Off()
}
20 changes: 20 additions & 0 deletions internal/subshell/termecho/termecho.go
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.Off()
}
Loading