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

Update shell detection #3466

Merged
merged 41 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
a541ffa
Update shell detection
MDrakos Aug 27, 2024
1807cee
Fix Windows detection
MDrakos Aug 27, 2024
958ba7f
Remove test code
MDrakos Aug 27, 2024
2a9fbc2
Add newline
MDrakos Aug 27, 2024
14e274f
Debug test failures
MDrakos Aug 27, 2024
27932e1
Debug
MDrakos Aug 27, 2024
e4f6e33
Change order in shell detection
MDrakos Aug 27, 2024
3f56d39
Add bash support on Windows
MDrakos Aug 28, 2024
84ba928
Remove debug logs
MDrakos Aug 28, 2024
05ff825
Merge branch 'version/0-47-0-RC1' into DX-3006
MDrakos Aug 28, 2024
efe42e9
Add back debug logging
MDrakos Aug 28, 2024
46e9a00
Set shell on Windows
MDrakos Aug 28, 2024
198b05a
Merge branch 'DX-3006' of github.com:ActiveState/cli into DX-3006
MDrakos Aug 28, 2024
9882ea8
Run Windows CI tests inside subshell
MDrakos Aug 29, 2024
d1a9f8a
Attempt to fix Windows tests
MDrakos Aug 29, 2024
976c6fc
Address more tests
MDrakos Aug 29, 2024
0f6e7d5
Revert install scripts test change
MDrakos Aug 29, 2024
408e7dd
Change powershell expect
MDrakos Aug 30, 2024
73fec74
Try another expect string
MDrakos Aug 30, 2024
471f509
Remove maybe option
MDrakos Sep 3, 2024
8d8234f
Use env var
MDrakos Sep 3, 2024
77c970f
Fix shells tests
MDrakos Sep 3, 2024
b0886ea
Shells test fix
MDrakos Sep 3, 2024
8af3168
Remove opt
MDrakos Sep 3, 2024
e2df1a7
Remove logging calls
MDrakos Sep 3, 2024
53f8d17
Merge branch 'DX-3006' of github.com:ActiveState/cli into DX-3006
MDrakos Sep 3, 2024
e6eeb7b
Fix env var name
MDrakos Sep 4, 2024
b6ddb92
Make shell name comparison case-insensitive
MDrakos Sep 4, 2024
7907c12
Handle filepaths
MDrakos Sep 4, 2024
723b520
Update argument name
MDrakos Sep 4, 2024
c97aadb
Use configured first
MDrakos Sep 4, 2024
4cf5cda
Clear configured shell in test
MDrakos Sep 4, 2024
f00d77c
Clear configured shell on Windows test
MDrakos Sep 4, 2024
e532c53
Revert config setting on Windows
MDrakos Sep 4, 2024
df47edd
Respect override
MDrakos Sep 5, 2024
4f23d47
Test revert backtick change
MDrakos Sep 5, 2024
7cb0820
Disable override for install scripts tests
MDrakos Sep 5, 2024
396fe68
Update internal/subshell/subshell.go
MDrakos Sep 5, 2024
f57b755
Make configured a fallback
MDrakos Sep 5, 2024
bc1e2ec
Make process error a multilog error
MDrakos Sep 5, 2024
a7ac5d8
Add back powershell change
MDrakos Sep 6, 2024
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
3 changes: 3 additions & 0 deletions internal/constants/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -490,3 +490,6 @@ const OverrideSandbox = "ACTIVESTATE_TEST_OVERRIDE_SANDBOX"

// PlatformPrivateNamespace is the namespace for private packages.
const PlatformPrivateNamespace = "private"

// OverrideShellEnvVarName is the environment variable to set when overriding the shell for shell detection.
const OverrideShellEnvVarName = "ACTIVESTATE_CLI_SHELL_OVERRIDE"
38 changes: 22 additions & 16 deletions internal/subshell/subshell.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import (
"errors"
"os"
"os/exec"
"path"
"path/filepath"
"runtime"
"strings"

"github.com/ActiveState/cli/internal/constants"
"github.com/ActiveState/cli/internal/rtutils/ptr"
"github.com/shirou/gopsutil/v3/process"

Expand Down Expand Up @@ -182,20 +184,24 @@ func DetectShell(cfg sscommon.Configurable) (string, string) {
}
}()

binary = os.Getenv("SHELL")
if binary == "" && runtime.GOOS == "windows" {
binary = detectShellWindows()
if os.Getenv(constants.OverrideShellEnvVarName) != "" {
binary = os.Getenv(constants.OverrideShellEnvVarName)
}

if binary == "" {
binary = detectShellParent()
}

if binary == "" {
binary = configured
}
MDrakos marked this conversation as resolved.
Show resolved Hide resolved

if binary == "" {
if runtime.GOOS == "windows" {
binary = "cmd.exe"
} else {
binary = "bash"
}
binary = os.Getenv(SHELL_ENV_VAR)
}

if binary == "" {
binary = OS_DEFAULT
}

path := resolveBinaryPath(binary)
Expand Down Expand Up @@ -239,24 +245,24 @@ func DetectShell(cfg sscommon.Configurable) (string, string) {
return name, path
}

func detectShellWindows() string {
// Windows does not provide a way of identifying which shell we are running in, so we have to look at the parent
// process.

func detectShellParent() string {
p, err := process.NewProcess(int32(os.Getppid()))
if err != nil && !errors.As(err, ptr.To(&os.PathError{})) {
panic(err)
multilog.Error("Failed to get parent process: %s", errs.JoinMessage(err))
}

for p != nil {
for p != nil && p.Pid != 0 {
name, err := p.Name()
if err == nil {
if strings.Contains(name, "cmd.exe") || strings.Contains(name, "powershell.exe") {
if strings.Contains(name, string(filepath.Separator)) {
name = path.Base(name)
}
if supportedShellName(name) {
MDrakos marked this conversation as resolved.
Show resolved Hide resolved
return name
}
}
p, _ = p.Parent()
}

return os.Getenv("ComSpec")
return ""
}
16 changes: 16 additions & 0 deletions internal/subshell/subshell_lin_mac.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package subshell

import (
"strings"

"github.com/ActiveState/cli/internal/subshell/bash"
"github.com/ActiveState/cli/internal/subshell/cmd"
"github.com/ActiveState/cli/internal/subshell/fish"
Expand All @@ -18,3 +20,17 @@ var supportedShells = []SubShell{
&fish.SubShell{},
&cmd.SubShell{},
}

const (
SHELL_ENV_VAR = "SHELL"
OS_DEFAULT = "bash"
)

func supportedShellName(filename string) bool {
for _, subshell := range supportedShells {
if strings.EqualFold(filename, subshell.Shell()) {
return true
}
}
return false
}
25 changes: 24 additions & 1 deletion internal/subshell/subshell_win.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,31 @@

package subshell

import "github.com/ActiveState/cli/internal/subshell/cmd"
import (
"fmt"
"strings"

"github.com/ActiveState/cli/internal/subshell/bash"
"github.com/ActiveState/cli/internal/subshell/cmd"
"github.com/ActiveState/cli/internal/subshell/pwsh"
)

var supportedShells = []SubShell{
&cmd.SubShell{},
&pwsh.SubShell{},
&bash.SubShell{},
}

const (
SHELL_ENV_VAR = "COMSPEC"
OS_DEFAULT = "cmd.exe"
)

func supportedShellName(filename string) bool {
for _, subshell := range supportedShells {
if strings.EqualFold(filename, fmt.Sprintf("%s.exe", subshell.Shell())) {
return true
}
}
return false
}
8 changes: 7 additions & 1 deletion internal/testhelpers/e2e/env_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const (
)

func platformSpecificEnv(dirs *Dirs) []string {
return []string{
env := []string{
"SystemDrive=C:",
"SystemRoot=C:\\Windows",
"PROGRAMFILES=C:\\Program Files",
Expand All @@ -39,6 +39,12 @@ func platformSpecificEnv(dirs *Dirs) []string {
fmt.Sprintf("LOCALAPPDATA=%s", dirs.TempDir),
fmt.Sprintf("%s=true", constants.DisableActivateEventsEnvVarName),
}

if condition.OnCI() {
env = append(env, fmt.Sprintf("%s=cmd.exe", constants.OverrideShellEnvVarName))
}

return env
}

func platformPath() string {
Expand Down
2 changes: 1 addition & 1 deletion internal/testhelpers/e2e/spawn.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func (s *SpawnedCmd) ExpectInput(opts ...termtest.SetExpectOpt) error {
expect := `expect'input from posix shell`
if cmdName != "bash" && shellName != "bash" && runtime.GOOS == "windows" {
if strings.Contains(cmdName, "powershell") || strings.Contains(shellName, "powershell") {
send = "echo \"`<expect input from powershell`>\""
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this works? IIRC the backtick is an escape on powershell.

Copy link
Member Author

Choose a reason for hiding this comment

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

You were right, I was running into a failure with this because it was defaulting back to cmd.exe and not powershell.

Copy link
Member Author

Choose a reason for hiding this comment

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

Never mind, I was right. Here's the output on powershell

[ActiveState-CLI/small-python] C:\Users\runneradmin\AppData\Local\Temp\2179854665\work\small-python>echo "`<expect input from powershell`>" 
"`<expect input from powershell`>"    

And the expect

'Expected: "<expect input from powershell>": after 40s: timeout':
              'after 40s: timeout': timeout

Copy link
Member

Choose a reason for hiding this comment

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

My guess is it depends on which shell you started from. So your new tests may want this variant but older tests will want the escaping? Worth perhaps looking for other places where we run ExpectInput and running those tests on Windows before merging your PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

All of the integration tests that start powershell are also critical. Only one of them expects input which is the one that flagged this change.

send = "echo \"<expect input from powershell>\""
expect = `<expect input from powershell>`
} else {
send = `echo ^<expect input from cmd prompt^>`
Expand Down
15 changes: 14 additions & 1 deletion test/integration/install_scripts_int_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import (
"runtime"
"testing"

"github.com/ActiveState/cli/internal/config"
"github.com/ActiveState/cli/internal/rtutils/singlethread"
"github.com/ActiveState/cli/internal/subshell"
"github.com/ActiveState/cli/internal/testhelpers/suite"
"github.com/stretchr/testify/require"
"github.com/thoas/go-funk"
Expand Down Expand Up @@ -104,7 +107,10 @@ func (suite *InstallScriptsIntegrationTestSuite) TestInstall() {
}
if runtime.GOOS == "windows" {
cmd = "powershell.exe"
opts = append(opts, e2e.OptAppendEnv("SHELL="))
opts = append(opts,
e2e.OptAppendEnv("SHELL="),
e2e.OptAppendEnv(constants.OverrideShellEnvVarName+"="),
)
}
cp := ts.SpawnCmdWithOpts(cmd, opts...)
cp.Expect("Preparing Installer for State Tool Package Manager")
Expand Down Expand Up @@ -137,12 +143,19 @@ func (suite *InstallScriptsIntegrationTestSuite) TestInstall() {
suite.assertAnalytics(ts)
suite.DirExists(ts.Dirs.Config)

// Clear configured shell.
cfg, err := config.NewCustom(ts.Dirs.Config, singlethread.New(), true)
suite.Require().NoError(err)
err = cfg.Set(subshell.ConfigKeyShell, "")
suite.Require().NoError(err)

// Verify that can install overtop
if runtime.GOOS != "windows" {
cp = ts.SpawnCmdWithOpts("bash", e2e.OptArgs(argsPlain...))
} else {
cp = ts.SpawnCmdWithOpts("powershell.exe", e2e.OptArgs(argsPlain...),
e2e.OptAppendEnv("SHELL="),
e2e.OptAppendEnv(constants.OverrideShellEnvVarName+"="),
)
}
cp.Expect("successfully installed")
Expand Down
19 changes: 17 additions & 2 deletions test/integration/shell_int_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/ActiveState/cli/internal/config"
"github.com/ActiveState/cli/internal/constants"
"github.com/ActiveState/cli/internal/fileutils"
"github.com/ActiveState/cli/internal/rtutils/singlethread"
"github.com/ActiveState/cli/internal/subshell"
"github.com/ActiveState/cli/internal/subshell/bash"
"github.com/ActiveState/cli/internal/subshell/sscommon"
Expand Down Expand Up @@ -489,14 +490,28 @@ func (suite *ShellIntegrationTestSuite) TestWindowsShells() {

hostname, err := os.Hostname()
suite.Require().NoError(err)
cp := ts.SpawnCmd("cmd", "/C", "state", "shell")
cp := ts.SpawnCmdWithOpts(
"cmd",
e2e.OptArgs("/C", "state", "shell"),
e2e.OptAppendEnv(constants.OverrideShellEnvVarName+"="),
)
cp.ExpectInput()
cp.SendLine("hostname")
cp.Expect(hostname) // cmd.exe shows the actual hostname
cp.SendLine("exit")
cp.ExpectExitCode(0)

cp = ts.SpawnCmd("powershell", "-Command", "state", "shell")
// Clear configured shell.
cfg, err := config.NewCustom(ts.Dirs.Config, singlethread.New(), true)
suite.Require().NoError(err)
err = cfg.Set(subshell.ConfigKeyShell, "")
suite.Require().NoError(err)

cp = ts.SpawnCmdWithOpts(
"powershell",
e2e.OptArgs("-Command", "state", "shell"),
e2e.OptAppendEnv(constants.OverrideShellEnvVarName+"="),
)
cp.ExpectInput()
cp.SendLine("$host.name")
cp.Expect("ConsoleHost") // powershell always shows ConsoleHost, go figure
Expand Down
20 changes: 18 additions & 2 deletions test/integration/shells_int_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ import (
"runtime"
"testing"

"github.com/ActiveState/cli/internal/config"
"github.com/ActiveState/cli/internal/constants"
"github.com/ActiveState/cli/internal/rtutils/singlethread"
"github.com/ActiveState/cli/internal/subshell"
"github.com/ActiveState/cli/internal/testhelpers/suite"

"github.com/ActiveState/cli/internal/fileutils"
Expand Down Expand Up @@ -47,8 +51,17 @@ func (suite *ShellsIntegrationTestSuite) TestShells() {
suite.Require().NoError(err)
}

// Clear configured shell.
cfg, err := config.NewCustom(ts.Dirs.Config, singlethread.New(), true)
suite.Require().NoError(err)
err = cfg.Set(subshell.ConfigKeyShell, "")
suite.Require().NoError(err)

// Run the checkout in a particular shell.
cp = ts.SpawnShellWithOpts(shell)
cp = ts.SpawnShellWithOpts(
shell,
e2e.OptAppendEnv(constants.OverrideShellEnvVarName+"="),
)
cp.SendLine(e2e.QuoteCommand(shell, ts.ExecutablePath(), "checkout", "ActiveState-CLI/small-python", string(shell)))
cp.Expect("Checked out project")
cp.SendLine("exit")
Expand All @@ -58,7 +71,10 @@ func (suite *ShellsIntegrationTestSuite) TestShells() {

// There are 2 or more instances checked out, so we should get a prompt in whichever shell we
// use.
cp = ts.SpawnShellWithOpts(shell)
cp = ts.SpawnShellWithOpts(
shell,
e2e.OptAppendEnv(constants.OverrideShellEnvVarName+"="),
)
cp.SendLine(e2e.QuoteCommand(shell, ts.ExecutablePath(), "shell", "small-python"))
cp.Expect("Multiple project paths")

Expand Down
Loading