Repository: benhoyt/goawk
Pull request #64 (2021-09-08 11:34:14)

Add ShellCommand config option and change default to /bin/sh (on Unix)
Attempt at fixing #59.

I checked the test suite (`go test ./... -v`) - seems all green.
1. Though I feel would be good to add some more tests for this specific case, but I'm a bit unsure how exactly. The test suite in `interp_test.go` doen't have means for provinding environment variables to test. Would be glad for an advice.
2. Hopefully I changed `sh` -> `/bin/sh` in all relevant places (to cover `system()`, `"cmd" | getline`, `print | "cmd"`). But now it feels like a bit of copy-paste. Do you think it makes sense to do something about this - like extracting a function or a constant? If yes, my concern is - what is better place for it, since the change is in two files, but it doesn't feel right to complicate things much for little reason.

benhoyt: Thanks for this. It's a good start, though I think we should make a few improvements:

* As mentioned in #59, while we're at it I think we should make this configurable via `interp.Config`. Let's add a new field `ExecCommand []string`, which if nil, defaults to `[]string{"/bin/sh", "-c"}` on Unix, and `[]string{"sh", "-c"}` on Windows.
* Setting the Windows default to just "sh" without the /bin path will make it keep working as it does today on Windows. See the unit test failures on Windows: https://github.com/benhoyt/goawk/pull/64/checks?check_run_id=3550794167
* I'm open to other names for this config field too. `ExecArgs`?
* With the new field, we will need a couple of additional tests for it. Probably a new `TestExecCommand` function in `interp/interp_test.go`. See for example https://github.com/benhoyt/goawk/blob/0eb3f8c8b7889fe1feff86aed767dcdb30cbc98e/interp/interp_test.go#L1014
xonixx: First things first I'm totaly agree with your arguments and I'm ready to do the changes you request.

However please find couple of considerations below.

Regarding failing on Windows. 
Just did a quick check https://github.com/xonixx/playground/runs/3568823431?check_suite_focus=true#step:3:8
I see that on Win this uses sh.exe bundled with Git, which is pre-bundled into Windows environment on Github (https://github.com/actions/virtual-environments/blob/main/images/win/Windows2019-Readme.md). 
Therefore I assume on clean windows (no Git) goawk will fail on `system()` as well.

Does it make sense to specify in docs that on Windows the `system()` functionality of goawk is only available when Git or msys (or other sh.exe provider) is installed?

Also does it make sense to not be Windows-specific but rather go more generic approach and implement the adaptive logic to attempt both
- `/bin/sh` as POSIX require
- `sh` from `$PATH` if above fails

@benhoyt if you think it doesn't make sense to go this deep I'm totally fine just to do as you already prosposed.


benhoyt: It's a good point regarding "sh" only existing on Windows due to Git. However, I'd rather not do a system call in every ExecProgram call due to the cost of hitting the file system (I know some use that in a fairly tight loop). So for now let's do what I suggested, and I can clean up the Windows shell 
handling in a subsequent commit (maybe "cmd.exe").
xonixx: @benhoyt Hopefully I fixed all CR notes

Patch:
diff --git a/interp/functions.go b/interp/functions.go
index b646ec2..a738be0 100644
--- a/interp/functions.go
+++ b/interp/functions.go
@@ -192,7 +192,7 @@ func (p *interp) callBuiltin(op Token, argExprs []Expr) (value, error) {
 			return null(), newError("can't call system() due to NoExec")
 		}
 		cmdline := p.toString(args[0])
-		cmd := exec.Command("sh", "-c", cmdline)
+		cmd := p.execShell(cmdline)
 		cmd.Stdout = p.output
 		cmd.Stderr = p.errorOutput
 		err := cmd.Start()
@@ -261,6 +261,15 @@ func (p *interp) callBuiltin(op Token, argExprs []Expr) (value, error) {
 	}
 }
 
+// Executes code using configured system shell
+func (p *interp) execShell(code string) *exec.Cmd {
+	executable := p.shellCommand[0]
+	args := p.shellCommand[1:]
+	args = append(args, code)
+	cmd := exec.Command(executable, args...)
+	return cmd
+}
+
 // Call user-defined function with given index and arguments, return
 // its return value (or null value if it doesn't return anything)
 func (p *interp) callUser(index int, args []Expr) (value, error) {
@@ -321,10 +330,10 @@ func (p *interp) callUser(index int, args []Expr) (value, error) {
 }
 
 // Call native-defined function with given name and arguments, return
-// return value (or null value if it doesn't return anything).
+// its return value (or null value if it doesn't return anything).
 func (p *interp) callNative(index int, args []Expr) (value, error) {
 	f := p.nativeFuncs[index]
-	minIn := len(f.in) // Mininum number of args we should pass
+	minIn := len(f.in) // Minimum number of args we should pass
 	var variadicType reflect.Type
 	if f.isVariadic {
 		variadicType = f.in[len(f.in)-1].Elem()
diff --git a/interp/interp.go b/interp/interp.go
index 0b5830c..8423226 100644
--- a/interp/interp.go
+++ b/interp/interp.go
@@ -76,6 +76,7 @@ type interp struct {
 	noExec        bool
 	noFileWrites  bool
 	noFileReads   bool
+	shellCommand  []string
 
 	// Scalars, arrays, and function state
 	globals     []value
@@ -189,6 +190,10 @@ type Config struct {
 	NoExec       bool
 	NoFileWrites bool
 	NoFileReads  bool
+
+	// Exec args used to run system shell. Typically, this will
+	// be {"/bin/sh", "-c"}
+	ShellCommand []string
 }
 
 // ExecProgram executes the parsed program using the given interpreter
@@ -247,6 +252,17 @@ func ExecProgram(program *Program, config *Config) (int, error) {
 		}
 	}
 
+	// Setup system shell command
+	if len(config.ShellCommand) != 0 {
+		p.shellCommand = config.ShellCommand
+	} else {
+		executable := "/bin/sh"
+		if runtime.GOOS == "windows" {
+			executable = "sh"
+		}
+		p.shellCommand = []string{executable, "-c"}
+	}
+
 	// Setup I/O structures
 	p.stdin = config.Stdin
 	if p.stdin == nil {
diff --git a/interp/io.go b/interp/io.go
index 31624c4..7633f25 100644
--- a/interp/io.go
+++ b/interp/io.go
@@ -9,7 +9,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"os/exec"
 	"strconv"
 	"strings"
 	"unicode/utf8"
@@ -96,7 +95,7 @@ func (p *interp) getOutputStream(redirect Token, dest Expr) (io.Writer, error) {
 		if p.noExec {
 			return nil, newError("can't write to pipe due to NoExec")
 		}
-		cmd := exec.Command("sh", "-c", name)
+		cmd := p.execShell(name)
 		w, err := cmd.StdinPipe()
 		if err != nil {
 			return nil, newError("error connecting to stdin pipe: %v", err)
@@ -160,7 +159,7 @@ func (p *interp) getInputScannerPipe(name string) (*bufio.Scanner, error) {
 	if p.noExec {
 		return nil, newError("can't read from pipe due to NoExec")
 	}
-	cmd := exec.Command("sh", "-c", name)
+	cmd := p.execShell(name)
 	cmd.Stdin = p.stdin
 	cmd.Stderr = p.errorOutput
 	r, err := cmd.StdoutPipe()


Test patch:
diff --git a/interp/interp_test.go b/interp/interp_test.go
index fff6447..dcdf1ee 100644
--- a/interp/interp_test.go
+++ b/interp/interp_test.go
@@ -10,6 +10,7 @@ import (
 	"os"
 	"os/exec"
 	"reflect"
+	"runtime"
 	"strings"
 	"sync"
 	"testing"
@@ -1029,6 +1030,30 @@ func TestConfigVarsCorrect(t *testing.T) {
 	}
 }
 
+func TestShellCommand(t *testing.T) {
+	testGoAWK(t, `BEGIN { system("echo hello world") }`, "", "hello world\n", "", nil, nil)
+
+	if runtime.GOOS == "windows" {
+		testGoAWK(t, `BEGIN { system("echo hello world") }`, "", "hello world\n", "", nil,
+			func(config *interp.Config) {
+				config.ShellCommand = []string{"cmd.exe", "/c"}
+			})
+	} else {
+		testGoAWK(t, `BEGIN { system("world") }`, "", "hello world\n", "", nil,
+			func(config *interp.Config) {
+				config.ShellCommand = []string{"/bin/echo", "hello"}
+			})
+		testGoAWK(t, `BEGIN { "world" | getline; print }`, "", "hello world\n", "", nil,
+			func(config *interp.Config) {
+				config.ShellCommand = []string{"/bin/echo", "hello"}
+			})
+		testGoAWK(t, `BEGIN { print "hello world" | "-" }`, "", "hello world\n", "", nil,
+			func(config *interp.Config) {
+				config.ShellCommand = []string{"/bin/cat"}
+			})
+	}
+}
+
 type mockFlusher struct {
 	bytes.Buffer
 	flushes []string