Make libs/exec fallback to `sh` if `bash` cannot be found (#1114)

## Changes

Falling back to `sh` is also what GitHub Actions do if `bash` is not
found in the path. It is possible `bash` is not available when running
from minimal Docker containers and we must not error out in this case.

See:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell.

This change renames `interpreter` -> `shell`.

## Tests

Unit tests pass.

libs/exec/exec.go

@@ -46,23 +46,23 @@ func (c *command) Stderr() io.ReadCloser {
 }
 
 type Executor struct {
-	interpreter interpreter
+	shell shell
 	dir   string
 }
 
 func NewCommandExecutor(dir string) (*Executor, error) {
-	interpreter, err := findInterpreter()
+	shell, err := findShell()
 	if err != nil {
 		return nil, err
 	}
 	return &Executor{
-		interpreter: interpreter,
+		shell: shell,
 		dir:   dir,
 	}, nil
 }
 
 func (e *Executor) StartCommand(ctx context.Context, command string) (Command, error) {
-	ec, err := e.interpreter.prepare(command)
+	ec, err := e.shell.prepare(command)
 	if err != nil {
 		return nil, err
 	}

libs/exec/exec_test.go

@@ -2,8 +2,11 @@ package exec
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
+	"os"
+	osexec "os/exec"
 	"runtime"
 	"sync"
 	"testing"
@@ -49,51 +52,63 @@ func TestExecutorWithInvalidCommandWithWindowsLikePath(t *testing.T) {
 	assert.Contains(t, string(out), "C:\\Program Files\\invalid-command.exe: No such file or directory")
 }
 
-func TestFindBashInterpreterNonWindows(t *testing.T) {
+func testExecutorWithShell(t *testing.T, shell string) {
+	p, err := osexec.LookPath(shell)
+	if err != nil {
+		if errors.Is(err, osexec.ErrNotFound) {
+			switch runtime.GOOS {
+			case "windows":
+				if shell == "cmd" {
+					// We must find `cmd.exe` on Windows.
+					t.Fatal("cmd.exe not found")
+				}
+			default:
+				if shell == "bash" || shell == "sh" {
+					// We must find `bash` or `sh` on other operating systems.
+					t.Fatal("bash or sh not found")
+				}
+			}
+			t.Skipf("shell %s not found", shell)
+		}
+		t.Fatal(err)
+	}
+
+	// Create temporary directory with only the shell executable in the PATH.
+	tmpDir := t.TempDir()
+	t.Setenv("PATH", tmpDir)
 	if runtime.GOOS == "windows" {
-		t.SkipNow()
+		os.Symlink(p, fmt.Sprintf("%s/%s.exe", tmpDir, shell))
+	} else {
+		os.Symlink(p, fmt.Sprintf("%s/%s", tmpDir, shell))
 	}
 
-	interpreter, err := findBashInterpreter()
+	executor, err := NewCommandExecutor(".")
 	assert.NoError(t, err)
-	assert.NotEmpty(t, interpreter)
-
-	e, err := NewCommandExecutor(".")
+	out, err := executor.Exec(context.Background(), "echo 'Hello from shell'")
 	assert.NoError(t, err)
-	e.interpreter = interpreter
-
-	assert.NoError(t, err)
-	out, err := e.Exec(context.Background(), `echo "Hello from bash"`)
-	assert.NoError(t, err)
-
-	assert.Equal(t, "Hello from bash\n", string(out))
+	assert.NotNil(t, out)
+	assert.Contains(t, string(out), "Hello from shell")
 }
 
-func TestFindCmdInterpreter(t *testing.T) {
-	if runtime.GOOS != "windows" {
-		t.SkipNow()
+func TestExecutorWithDifferentShells(t *testing.T) {
+	for _, shell := range []string{"bash", "sh", "cmd"} {
+		t.Run(shell, func(t *testing.T) {
+			testExecutorWithShell(t, shell)
+		})
+	}
 }
 
-	interpreter, err := findCmdInterpreter()
-	assert.NoError(t, err)
-	assert.NotEmpty(t, interpreter)
-
-	e, err := NewCommandExecutor(".")
-	assert.NoError(t, err)
-	e.interpreter = interpreter
-
-	assert.NoError(t, err)
-	out, err := e.Exec(context.Background(), `echo "Hello from cmd"`)
-	assert.NoError(t, err)
-
-	assert.Contains(t, string(out), "Hello from cmd")
+func TestExecutorNoShellFound(t *testing.T) {
+	t.Setenv("PATH", "")
+	_, err := NewCommandExecutor(".")
+	assert.ErrorContains(t, err, "no shell found")
 }
 
 func TestExecutorCleanupsTempFiles(t *testing.T) {
 	executor, err := NewCommandExecutor(".")
 	assert.NoError(t, err)
 
-	ec, err := executor.interpreter.prepare("echo 'Hello'")
+	ec, err := executor.shell.prepare("echo 'Hello'")
 	assert.NoError(t, err)
 
 	cmd, err := executor.start(context.Background(), ec)

libs/exec/interpreter.go

@@ -1,123 +0,0 @@
-package exec
-
-import (
-	"errors"
-	"fmt"
-	"io"
-	"os"
-	osexec "os/exec"
-)
-
-type interpreter interface {
-	prepare(string) (*execContext, error)
-}
-
-type execContext struct {
-	executable string
-	args       []string
-	scriptFile string
-}
-
-type bashInterpreter struct {
-	executable string
-}
-
-func (b *bashInterpreter) prepare(command string) (*execContext, error) {
-	filename, err := createTempScript(command, ".sh")
-	if err != nil {
-		return nil, err
-	}
-
-	return &execContext{
-		executable: b.executable,
-		args:       []string{"-e", filename},
-		scriptFile: filename,
-	}, nil
-}
-
-type cmdInterpreter struct {
-	executable string
-}
-
-func (c *cmdInterpreter) prepare(command string) (*execContext, error) {
-	filename, err := createTempScript(command, ".cmd")
-	if err != nil {
-		return nil, err
-	}
-
-	return &execContext{
-		executable: c.executable,
-		args:       []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL %s`, filename)},
-		scriptFile: filename,
-	}, nil
-}
-
-func findInterpreter() (interpreter, error) {
-	interpreter, err := findBashInterpreter()
-	if err != nil {
-		return nil, err
-	}
-
-	if interpreter != nil {
-		return interpreter, nil
-	}
-
-	interpreter, err = findCmdInterpreter()
-	if err != nil {
-		return nil, err
-	}
-
-	if interpreter != nil {
-		return interpreter, nil
-	}
-
-	return nil, errors.New("no interpreter found")
-}
-
-func findBashInterpreter() (interpreter, error) {
-	// Lookup for bash executable first (Linux, MacOS, maybe Windows)
-	out, err := osexec.LookPath("bash")
-	if err != nil && !errors.Is(err, osexec.ErrNotFound) {
-		return nil, err
-	}
-
-	// Bash executable is not found, returning early
-	if out == "" {
-		return nil, nil
-	}
-
-	return &bashInterpreter{executable: out}, nil
-}
-
-func findCmdInterpreter() (interpreter, error) {
-	// Lookup for CMD executable (Windows)
-	out, err := osexec.LookPath("cmd")
-	if err != nil && !errors.Is(err, osexec.ErrNotFound) {
-		return nil, err
-	}
-
-	// CMD executable is not found, returning early
-	if out == "" {
-		return nil, nil
-	}
-
-	return &cmdInterpreter{executable: out}, nil
-}
-
-func createTempScript(command string, extension string) (string, error) {
-	file, err := os.CreateTemp(os.TempDir(), "cli-exec*"+extension)
-	if err != nil {
-		return "", err
-	}
-
-	defer file.Close()
-
-	_, err = io.WriteString(file, command)
-	if err != nil {
-		// Try to remove the file if we failed to write to it
-		os.Remove(file.Name())
-		return "", err
-	}
-
-	return file.Name(), nil
-}

libs/exec/shell.go

@@ -0,0 +1,54 @@
+package exec
+
+import (
+	"errors"
+	"io"
+	"os"
+)
+
+type shell interface {
+	prepare(string) (*execContext, error)
+}
+
+type execContext struct {
+	executable string
+	args       []string
+	scriptFile string
+}
+
+func findShell() (shell, error) {
+	for _, fn := range []func() (shell, error){
+		newBashShell,
+		newShShell,
+		newCmdShell,
+	} {
+		shell, err := fn()
+		if err != nil {
+			return nil, err
+		}
+
+		if shell != nil {
+			return shell, nil
+		}
+	}
+
+	return nil, errors.New("no shell found")
+}
+
+func createTempScript(command string, extension string) (string, error) {
+	file, err := os.CreateTemp(os.TempDir(), "cli-exec*"+extension)
+	if err != nil {
+		return "", err
+	}
+
+	defer file.Close()
+
+	_, err = io.WriteString(file, command)
+	if err != nil {
+		// Try to remove the file if we failed to write to it
+		os.Remove(file.Name())
+		return "", err
+	}
+
+	return file.Name(), nil
+}

libs/exec/shell_bash.go

@@ -0,0 +1,37 @@
+package exec
+
+import (
+	"errors"
+	osexec "os/exec"
+)
+
+type bashShell struct {
+	executable string
+}
+
+func (s bashShell) prepare(command string) (*execContext, error) {
+	filename, err := createTempScript(command, ".sh")
+	if err != nil {
+		return nil, err
+	}
+
+	return &execContext{
+		executable: s.executable,
+		args:       []string{"-e", filename},
+		scriptFile: filename,
+	}, nil
+}
+
+func newBashShell() (shell, error) {
+	out, err := osexec.LookPath("bash")
+	if err != nil && !errors.Is(err, osexec.ErrNotFound) {
+		return nil, err
+	}
+
+	// `bash` is not found, return early.
+	if out == "" {
+		return nil, nil
+	}
+
+	return &bashShell{executable: out}, nil
+}

libs/exec/shell_bash_test.go

@@ -0,0 +1,30 @@
+package exec
+
+import (
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestBashFound(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.SkipNow()
+	}
+
+	shell, err := newBashShell()
+	assert.NoError(t, err)
+	assert.NotNil(t, shell)
+}
+
+func TestBashNotFound(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.SkipNow()
+	}
+
+	t.Setenv("PATH", "")
+
+	shell, err := newBashShell()
+	assert.NoError(t, err)
+	assert.Nil(t, shell)
+}

libs/exec/shell_cmd.go

@@ -0,0 +1,38 @@
+package exec
+
+import (
+	"errors"
+	"fmt"
+	osexec "os/exec"
+)
+
+type cmdShell struct {
+	executable string
+}
+
+func (s cmdShell) prepare(command string) (*execContext, error) {
+	filename, err := createTempScript(command, ".cmd")
+	if err != nil {
+		return nil, err
+	}
+
+	return &execContext{
+		executable: s.executable,
+		args:       []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL %s`, filename)},
+		scriptFile: filename,
+	}, nil
+}
+
+func newCmdShell() (shell, error) {
+	out, err := osexec.LookPath("cmd")
+	if err != nil && !errors.Is(err, osexec.ErrNotFound) {
+		return nil, err
+	}
+
+	// `cmd.exe` is not found, return early.
+	if out == "" {
+		return nil, nil
+	}
+
+	return &cmdShell{executable: out}, nil
+}

libs/exec/shell_cmd_test.go

@@ -0,0 +1,30 @@
+package exec
+
+import (
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCmdFound(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.SkipNow()
+	}
+
+	shell, err := newCmdShell()
+	assert.NoError(t, err)
+	assert.NotNil(t, shell)
+}
+
+func TestCmdNotFound(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.SkipNow()
+	}
+
+	t.Setenv("PATH", "")
+
+	shell, err := newCmdShell()
+	assert.NoError(t, err)
+	assert.Nil(t, shell)
+}

libs/exec/shell_sh.go

@@ -0,0 +1,37 @@
+package exec
+
+import (
+	"errors"
+	osexec "os/exec"
+)
+
+type shShell struct {
+	executable string
+}
+
+func (s shShell) prepare(command string) (*execContext, error) {
+	filename, err := createTempScript(command, ".sh")
+	if err != nil {
+		return nil, err
+	}
+
+	return &execContext{
+		executable: s.executable,
+		args:       []string{"-e", filename},
+		scriptFile: filename,
+	}, nil
+}
+
+func newShShell() (shell, error) {
+	out, err := osexec.LookPath("sh")
+	if err != nil && !errors.Is(err, osexec.ErrNotFound) {
+		return nil, err
+	}
+
+	// `sh` is not found, return early.
+	if out == "" {
+		return nil, nil
+	}
+
+	return &shShell{executable: out}, nil
+}

libs/exec/shell_sh_test.go

@@ -0,0 +1,30 @@
+package exec
+
+import (
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestShFound(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.SkipNow()
+	}
+
+	shell, err := newShShell()
+	assert.NoError(t, err)
+	assert.NotNil(t, shell)
+}
+
+func TestShNotFound(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.SkipNow()
+	}
+
+	t.Setenv("PATH", "")
+
+	shell, err := newShShell()
+	assert.NoError(t, err)
+	assert.Nil(t, shell)
+}