-
Notifications
You must be signed in to change notification settings - Fork 13
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
Update shell detection #3466
Changes from 20 commits
a541ffa
1807cee
958ba7f
2a9fbc2
14e274f
27932e1
e4f6e33
3f56d39
84ba928
05ff825
efe42e9
46e9a00
198b05a
9882ea8
d1a9f8a
976c6fc
0f6e7d5
408e7dd
73fec74
471f509
8d8234f
77c970f
b0886ea
8af3168
e2df1a7
53f8d17
e6eeb7b
b6ddb92
7907c12
723b520
c97aadb
4cf5cda
f00d77c
e532c53
df47edd
4f23d47
7cb0820
396fe68
f57b755
bc1e2ec
a7ac5d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,20 +182,18 @@ func DetectShell(cfg sscommon.Configurable) (string, string) { | |
} | ||
}() | ||
|
||
binary = os.Getenv("SHELL") | ||
if binary == "" && runtime.GOOS == "windows" { | ||
binary = detectShellWindows() | ||
binary = detectShellParent() | ||
logging.Debug("Configured shell: %s", binary) | ||
if binary == "" { | ||
binary = configured | ||
} | ||
|
||
if binary == "" { | ||
binary = configured | ||
binary = os.Getenv(SHELL_ENV_VAR) | ||
} | ||
|
||
if binary == "" { | ||
if runtime.GOOS == "windows" { | ||
binary = "cmd.exe" | ||
} else { | ||
binary = "bash" | ||
} | ||
binary = OS_DEFULAT | ||
} | ||
|
||
path := resolveBinaryPath(binary) | ||
|
@@ -239,24 +237,23 @@ 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 { | ||
logging.Debug("Detecting shell from parent process") | ||
p, err := process.NewProcess(int32(os.Getppid())) | ||
if err != nil && !errors.As(err, ptr.To(&os.PathError{})) { | ||
panic(err) | ||
logging.Error("Failed to get parent process: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why this was a panic, but we went from panicking to essentially ignoring it. Please return the error up the chain instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was originally a windows only function which may be a reason for the panic? No other functions in this package return errors either as Again, I just want to check before making this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, let's go with this for now, but please make it a multilog error so we can track this on rollbar. If this is happening a lot we'll want to know about it.
MDrakos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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") { | ||
logging.Debug("Searching for supported shell in parent process: %s", name) | ||
if supportedShellName(name) { | ||
MDrakos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return name | ||
} | ||
} | ||
p, _ = p.Parent() | ||
} | ||
|
||
return os.Getenv("ComSpec") | ||
return "" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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`>\"" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind, I was right. Here's the output on powershell
And the expect
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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^>` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I realized upon reading the changes that configured had a different connotation than I remembered. In my mind this was "what the user configured", but actually this is just a fallback we ourselves set when we detect the shell.
This is awkward and should probably be revisited, but given the current behaviour I this should probably be a fallback after shell parent and environment.