Update shell detection #3466

MDrakos · 2024-08-27T22:05:59Z

DX-3006 Improve SHELL detection; base on parent process

MDrakos · 2024-09-03T18:31:29Z

@Naatan I'm looking for feedback on how I addressed the integration test failures. I think the comment explain it but if there are any questions let me know. With this approach or a different one I will have to go through all of the integration tests and update them.

Of course feel free to review everything. I'm just asking for a review early before I address the remaining tests.

Naatan

This looks ok to me, but I don't think we need to make such sweeping changes. The only place this ought to matter is where we specifically test shell detection, which I imagine should be only one test.

I imagine what you're running into is that other tests have started to break because we shifted shell detection to be process based? If that's the case I think a more general purpose solution is appropriate, eg. add an env var that will trump shell detection that we can set on CI (ACTIVESTATE_OVERRIDE_SHELL?).

I'd prefer that over updating all these tests and the e2e code, as that feels less maintainable.

Naatan · 2024-09-04T17:06:19Z

internal/subshell/subshell.go

+	if os.Getenv(constants.OverrideShellEnvVarName) != "" {
+		binary = os.Getenv(constants.OverrideShellEnvVarName)
+	}
+
+	if binary == "" {
+		binary = detectShellParent()
 	}

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


Can we change this into a switch case? We don't need to assert binary == nil, instead assert whether we have something to populate it with.

We can however this leads to us calling functions like Getenv or detectShellParent() twice. We could also move the function calls outside of the switch but then there will be cases where we are calling them needlessly.

I'm not sure how much of a performance impact detectShellParent() has but we would be adding inefficiency for a marginal gain in readability. I'm fine with whatever direction we want to go but I wanted to flag this before I make the change.

Hmm I see. Annoying Go doesn't let you assign in a switch case like it does in a regular conditional. I guess there isn't really a decent pattern for us to use here :\

internal/subshell/subshell.go

Naatan · 2024-09-04T17:08:54Z

internal/subshell/subshell.go

 	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)


Not sure why this was a panic, but we went from panicking to essentially ignoring it. Please return the error up the chain instead.

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 DetectShell has fallbacks so it always returns a shell name. This function is also used but subshell.New. So even if we get a fallback shell from either the SHELL env var or the system default we would error out because we couldn't detect the parent.

Again, I just want to check before making this change.

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.

internal/subshell/subshell_lin_mac.go

internal/subshell/subshell_win.go

internal/subshell/subshell_lin_mac.go

internal/subshell/subshell.go

internal/subshell/subshell_win.go

internal/testhelpers/e2e/session.go

Naatan · 2024-09-04T17:17:51Z

internal/testhelpers/e2e/spawn.go

@@ -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`>\""


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

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

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

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.

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.

Handle case insensitivitiy

and clear config

Naatan · 2024-09-05T19:53:50Z

internal/subshell/subshell.go

+	if os.Getenv(constants.OverrideShellEnvVarName) != "" {
+		binary = os.Getenv(constants.OverrideShellEnvVarName)
+	}
+
+	if binary == "" {
+		binary = detectShellParent()
 	}

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


Hmm I see. Annoying Go doesn't let you assign in a switch case like it does in a regular conditional. I guess there isn't really a decent pattern for us to use here :\

Naatan · 2024-09-05T19:55:02Z

internal/subshell/subshell.go

 	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)


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.

internal/subshell/subshell.go

Naatan · 2024-09-05T20:03:16Z

internal/subshell/subshell.go

@@ -188,19 +189,19 @@ func DetectShell(cfg sscommon.Configurable) (string, string) {
 	}

 	if binary == "" {
-		binary = detectShellParent()
+		binary = configured


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.

Co-authored-by: Nathan Rijksen <[email protected]>

Naatan

Looks good, but please read the comment on ExpectInput.

Naatan · 2024-09-09T18:21:30Z

internal/testhelpers/e2e/spawn.go

@@ -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`>\""


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.

MDrakos added 3 commits August 27, 2024 14:24

Update shell detection

a541ffa

Fix Windows detection

1807cee

Remove test code

958ba7f

MDrakos added the Test: critical label Aug 27, 2024

Add newline

2a9fbc2

MDrakos changed the base branch from master to version/0-47-0-RC1 August 27, 2024 22:20

MDrakos added 8 commits August 27, 2024 15:49

Debug test failures

14e274f

Debug

27932e1

Change order in shell detection

e4f6e33

Add bash support on Windows

3f56d39

Remove debug logs

84ba928

Merge branch 'version/0-47-0-RC1' into DX-3006

05ff825

Add back debug logging

efe42e9

Set shell on Windows

46e9a00

MDrakos force-pushed the DX-3006 branch from efe42e9 to 46e9a00 Compare August 28, 2024 19:54

MDrakos added 8 commits August 28, 2024 13:47

Merge branch 'DX-3006' of github.com:ActiveState/cli into DX-3006

198b05a

Run Windows CI tests inside subshell

9882ea8

Attempt to fix Windows tests

d1a9f8a

Address more tests

976c6fc

Revert install scripts test change

0f6e7d5

Change powershell expect

408e7dd

Try another expect string

73fec74

Remove maybe option

471f509

MDrakos marked this pull request as ready for review September 3, 2024 18:26

MDrakos requested a review from Naatan September 3, 2024 18:31

Naatan requested changes Sep 3, 2024

View reviewed changes

MDrakos added 3 commits September 3, 2024 11:52

Use env var

8d8234f

Fix shells tests

77c970f

Shells test fix

b0886ea

MDrakos added 3 commits September 3, 2024 15:06

Remove opt

8af3168

Remove logging calls

e2df1a7

Merge branch 'DX-3006' of github.com:ActiveState/cli into DX-3006

53f8d17

MDrakos requested a review from Naatan September 3, 2024 22:30

Naatan requested changes Sep 4, 2024

View reviewed changes

MDrakos added 11 commits September 4, 2024 14:59

Fix env var name

e6eeb7b

Make shell name comparison case-insensitive

b6ddb92

Handle filepaths

7907c12

Update argument name

723b520

Handle case insensitivitiy

Use configured first

c97aadb

Clear configured shell in test

4cf5cda

Clear configured shell on Windows test

f00d77c

Revert config setting on Windows

e532c53

Respect override

df47edd

Test revert backtick change

4f23d47

Disable override for install scripts tests

7cb0820

and clear config

MDrakos requested a review from Naatan September 5, 2024 18:31

Naatan requested changes Sep 5, 2024

View reviewed changes

MDrakos and others added 4 commits September 5, 2024 16:07

Update internal/subshell/subshell.go

396fe68

Co-authored-by: Nathan Rijksen <[email protected]>

Make configured a fallback

f57b755

Make process error a multilog error

bc1e2ec

Add back powershell change

a7ac5d8

MDrakos requested a review from Naatan September 6, 2024 18:19

Naatan approved these changes Sep 9, 2024

View reviewed changes

MDrakos merged commit 7ecd46e into version/0-47-0-RC1 Sep 9, 2024
7 of 8 checks passed

MDrakos deleted the DX-3006 branch September 9, 2024 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update shell detection #3466

Update shell detection #3466

MDrakos commented Aug 27, 2024 •

edited by github-actions bot

Loading

MDrakos commented Sep 3, 2024

Naatan left a comment

Naatan Sep 4, 2024

MDrakos Sep 4, 2024

Naatan Sep 5, 2024

Naatan Sep 4, 2024

MDrakos Sep 4, 2024

Naatan Sep 5, 2024

Naatan Sep 4, 2024

MDrakos Sep 5, 2024

MDrakos Sep 6, 2024

Naatan Sep 9, 2024

MDrakos Sep 9, 2024

Naatan Sep 5, 2024

Naatan Sep 5, 2024

Naatan Sep 5, 2024

Naatan left a comment

Naatan Sep 9, 2024

Update shell detection #3466

Update shell detection #3466

Conversation

MDrakos commented Aug 27, 2024 • edited by github-actions bot Loading

MDrakos commented Sep 3, 2024

Naatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MDrakos commented Aug 27, 2024 •

edited by github-actions bot

Loading