From d0b152fc66213c3bddabd8f107b77ae8b9543f30 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 18 Dec 2023 18:16:58 +0100 Subject: [PATCH 01/14] Added cmdio.NewCommandExecutor to execute artifact build commands --- bundle/config/artifact.go | 20 ++--------- bundle/config/artifacts_test.go | 18 ++++++++++ bundle/scripts/scripts.go | 27 ++------------- libs/cmdio/exec.go | 60 +++++++++++++++++++++++++++++++++ libs/cmdio/exec_test.go | 24 +++++++++++++ 5 files changed, 107 insertions(+), 42 deletions(-) create mode 100644 bundle/config/artifacts_test.go create mode 100644 libs/cmdio/exec.go create mode 100644 libs/cmdio/exec_test.go diff --git a/bundle/config/artifact.go b/bundle/config/artifact.go index 63ab6c489d..f0fb49a538 100644 --- a/bundle/config/artifact.go +++ b/bundle/config/artifact.go @@ -1,14 +1,12 @@ package config import ( - "bytes" "context" "fmt" "path" - "strings" "github.com/databricks/cli/bundle/config/paths" - "github.com/databricks/cli/libs/process" + "github.com/databricks/cli/libs/cmdio" "github.com/databricks/databricks-sdk-go/service/compute" ) @@ -52,20 +50,8 @@ func (a *Artifact) Build(ctx context.Context) ([]byte, error) { return nil, fmt.Errorf("no build property defined") } - out := make([][]byte, 0) - commands := strings.Split(a.BuildCommand, " && ") - for _, command := range commands { - buildParts := strings.Split(command, " ") - var buf bytes.Buffer - _, err := process.Background(ctx, buildParts, - process.WithCombinedOutput(&buf), - process.WithDir(a.Path)) - if err != nil { - return buf.Bytes(), err - } - out = append(out, buf.Bytes()) - } - return bytes.Join(out, []byte{}), nil + exec := cmdio.NewCommandExecutor(a.Path) + return exec.Exec(ctx, a.BuildCommand) } func (a *Artifact) NormalisePaths() { diff --git a/bundle/config/artifacts_test.go b/bundle/config/artifacts_test.go new file mode 100644 index 0000000000..5fa159fdf3 --- /dev/null +++ b/bundle/config/artifacts_test.go @@ -0,0 +1,18 @@ +package config + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestArtifactBuild(t *testing.T) { + artifact := Artifact{ + BuildCommand: "echo 'Hello from build command'", + } + res, err := artifact.Build(context.Background()) + assert.NoError(t, err) + assert.NotNil(t, res) + assert.Equal(t, "Hello from build command\n", string(res)) +} diff --git a/bundle/scripts/scripts.go b/bundle/scripts/scripts.go index 90c1914fa9..d41c921787 100644 --- a/bundle/scripts/scripts.go +++ b/bundle/scripts/scripts.go @@ -56,26 +56,8 @@ func executeHook(ctx context.Context, b *bundle.Bundle, hook config.ScriptHook) return nil, nil, nil } - interpreter, err := findInterpreter() - if err != nil { - return nil, nil, err - } - - // TODO: switch to process.Background(...) - cmd := exec.CommandContext(ctx, interpreter, "-c", string(command)) - cmd.Dir = b.Config.Path - - outPipe, err := cmd.StdoutPipe() - if err != nil { - return nil, nil, err - } - - errPipe, err := cmd.StderrPipe() - if err != nil { - return nil, nil, err - } - - return cmd, io.MultiReader(outPipe, errPipe), cmd.Start() + executor := cmdio.NewCommandExecutor(b.Config.Path) + return executor.StartCommand(ctx, string(command)) } func getCommmand(b *bundle.Bundle, hook config.ScriptHook) config.Command { @@ -85,8 +67,3 @@ func getCommmand(b *bundle.Bundle, hook config.ScriptHook) config.Command { return b.Config.Experimental.Scripts[hook] } - -func findInterpreter() (string, error) { - // At the moment we just return 'sh' on all platforms and use it to execute scripts - return "sh", nil -} diff --git a/libs/cmdio/exec.go b/libs/cmdio/exec.go new file mode 100644 index 0000000000..091338feed --- /dev/null +++ b/libs/cmdio/exec.go @@ -0,0 +1,60 @@ +package cmdio + +import ( + "context" + "io" + "os/exec" +) + +type Executor struct { + dir string +} + +func NewCommandExecutor(dir string) *Executor { + return &Executor{ + dir: dir, + } +} + +func (e *Executor) StartCommand(ctx context.Context, command string) (*exec.Cmd, io.Reader, error) { + interpreter, err := findInterpreter() + if err != nil { + return nil, nil, err + } + + // TODO: switch to process.Background(...) + cmd := exec.CommandContext(ctx, interpreter, "-c", command) + cmd.Dir = e.dir + + outPipe, err := cmd.StdoutPipe() + if err != nil { + return nil, nil, err + } + + errPipe, err := cmd.StderrPipe() + if err != nil { + return nil, nil, err + } + + return cmd, io.MultiReader(outPipe, errPipe), cmd.Start() + +} + +func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { + cmd, out, err := e.StartCommand(ctx, command) + if err != nil { + return nil, err + } + + res, err := io.ReadAll(out) + if err != nil { + return nil, err + } + + return res, cmd.Wait() +} + +func findInterpreter() (string, error) { + // At the moment we just return 'sh' on all platforms and use it to execute scripts + return "sh", nil +} diff --git a/libs/cmdio/exec_test.go b/libs/cmdio/exec_test.go new file mode 100644 index 0000000000..0d5c699b45 --- /dev/null +++ b/libs/cmdio/exec_test.go @@ -0,0 +1,24 @@ +package cmdio + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestExecutorWithSimpleInput(t *testing.T) { + executor := NewCommandExecutor(".") + out, err := executor.Exec(context.Background(), "echo 'Hello'") + assert.NoError(t, err) + assert.NotNil(t, out) + assert.Equal(t, "Hello\n", string(out)) +} + +func TestExecutorWithComplexInput(t *testing.T) { + executor := NewCommandExecutor(".") + out, err := executor.Exec(context.Background(), "echo 'Hello' && echo 'World'") + assert.NoError(t, err) + assert.NotNil(t, out) + assert.Equal(t, "Hello\nWorld\n", string(out)) +} From 3d317b0092d0aca282d2468ca2a76578d11aa6b2 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 19 Dec 2023 13:44:03 +0100 Subject: [PATCH 02/14] find correct interpreter per os --- bundle/artifacts/whl/infer.go | 2 +- libs/cmdio/exec.go | 68 ++++++++++++++++++++++++++++++++--- libs/cmdio/exec_test.go | 24 +++++++++++++ 3 files changed, 88 insertions(+), 6 deletions(-) diff --git a/bundle/artifacts/whl/infer.go b/bundle/artifacts/whl/infer.go index dedecc3041..dc2b8e233e 100644 --- a/bundle/artifacts/whl/infer.go +++ b/bundle/artifacts/whl/infer.go @@ -33,7 +33,7 @@ func (m *infer) Apply(ctx context.Context, b *bundle.Bundle) error { // version=datetime.datetime.utcnow().strftime("%Y%m%d.%H%M%S"), // ... //) - artifact.BuildCommand = fmt.Sprintf("%s setup.py bdist_wheel", py) + artifact.BuildCommand = fmt.Sprintf(`"%s" setup.py bdist_wheel`, py) return nil } diff --git a/libs/cmdio/exec.go b/libs/cmdio/exec.go index 091338feed..d7f996a83c 100644 --- a/libs/cmdio/exec.go +++ b/libs/cmdio/exec.go @@ -2,6 +2,7 @@ package cmdio import ( "context" + "errors" "io" "os/exec" ) @@ -22,8 +23,9 @@ func (e *Executor) StartCommand(ctx context.Context, command string) (*exec.Cmd, return nil, nil, err } - // TODO: switch to process.Background(...) - cmd := exec.CommandContext(ctx, interpreter, "-c", command) + args := interpreter.args + args = append(args, command) + cmd := exec.CommandContext(ctx, interpreter.executable, args...) cmd.Dir = e.dir outPipe, err := cmd.StdoutPipe() @@ -54,7 +56,63 @@ func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { return res, cmd.Wait() } -func findInterpreter() (string, error) { - // At the moment we just return 'sh' on all platforms and use it to execute scripts - return "sh", nil +type interpreter struct { + executable string + args []string +} + +func findInterpreter() (*interpreter, error) { + // Lookup for bash executable first + out, err := exec.LookPath("bash") + if err != nil && !errors.Is(err, exec.ErrNotFound) { + return nil, err + } + + if out != "" { + return &interpreter{ + executable: out, + args: []string{"-c"}, + }, nil + } + + // Lookup for sh executable + out, err = exec.LookPath("sh") + if err != nil && !errors.Is(err, exec.ErrNotFound) { + return nil, err + } + + if out != "" { + return &interpreter{ + executable: out, + args: []string{"-c"}, + }, nil + } + + // Lookup for PowerShell executable + out, err = exec.LookPath("powershell") + if err != nil && !errors.Is(err, exec.ErrNotFound) { + return nil, err + } + + if out != "" { + return &interpreter{ + executable: out, + args: []string{"-Command"}, + }, nil + } + + // Lookup for CMD executable + out, err = exec.LookPath("cmd") + if err != nil && !errors.Is(err, exec.ErrNotFound) { + return nil, err + } + + if out != "" { + return &interpreter{ + executable: out, + args: []string{"/C"}, + }, nil + } + + return nil, errors.New("no interpreter found") } diff --git a/libs/cmdio/exec_test.go b/libs/cmdio/exec_test.go index 0d5c699b45..501c0d9665 100644 --- a/libs/cmdio/exec_test.go +++ b/libs/cmdio/exec_test.go @@ -2,6 +2,7 @@ package cmdio import ( "context" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -22,3 +23,26 @@ func TestExecutorWithComplexInput(t *testing.T) { assert.NotNil(t, out) assert.Equal(t, "Hello\nWorld\n", string(out)) } + +func TestExecutorWithInvalidCommand(t *testing.T) { + executor := NewCommandExecutor(".") + out, err := executor.Exec(context.Background(), "invalid-command") + assert.Error(t, err) + + if runtime.GOOS == "windows" { + assert.Contains(t, string(out), "'invalid-command' is not recognized") + } else { + assert.Contains(t, string(out), "invalid-command: command not found") + } +} + +func TestExecutorWithInvalidCommandWithWindowsLikePath(t *testing.T) { + if runtime.GOOS != "windows" { + t.SkipNow() + } + + executor := NewCommandExecutor(".") + out, err := executor.Exec(context.Background(), `"C:\Program Files\invalid-command.exe"`) + assert.Error(t, err) + assert.Contains(t, string(out), "'C:\\Program Files\\invalid-command.exe' is not recognized") +} From 09d32b430c04e80fe1747ca754740ed19f6b5d58 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 19 Dec 2023 15:02:36 +0100 Subject: [PATCH 03/14] execute with temp file --- libs/cmdio/exec.go | 31 ++++++++++++++++++++++--------- libs/cmdio/exec_test.go | 9 ++------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/libs/cmdio/exec.go b/libs/cmdio/exec.go index d7f996a83c..562f830adc 100644 --- a/libs/cmdio/exec.go +++ b/libs/cmdio/exec.go @@ -3,7 +3,9 @@ package cmdio import ( "context" "errors" + "fmt" "io" + "os" "os/exec" ) @@ -18,14 +20,12 @@ func NewCommandExecutor(dir string) *Executor { } func (e *Executor) StartCommand(ctx context.Context, command string) (*exec.Cmd, io.Reader, error) { - interpreter, err := findInterpreter() + interpreter, err := wrapInShellCall(command) if err != nil { return nil, nil, err } - args := interpreter.args - args = append(args, command) - cmd := exec.CommandContext(ctx, interpreter.executable, args...) + cmd := exec.CommandContext(ctx, interpreter.executable, interpreter.args...) cmd.Dir = e.dir outPipe, err := cmd.StdoutPipe() @@ -61,7 +61,20 @@ type interpreter struct { args []string } -func findInterpreter() (*interpreter, error) { +func wrapInShellCall(command string) (*interpreter, error) { + file, err := os.CreateTemp(os.TempDir(), "cli-exec") + if err != nil { + return nil, err + } + + defer file.Close() + + _, err = io.WriteString(file, command) + file.Close() + if err != nil { + return nil, err + } + // Lookup for bash executable first out, err := exec.LookPath("bash") if err != nil && !errors.Is(err, exec.ErrNotFound) { @@ -71,7 +84,7 @@ func findInterpreter() (*interpreter, error) { if out != "" { return &interpreter{ executable: out, - args: []string{"-c"}, + args: []string{"-e", file.Name()}, }, nil } @@ -84,7 +97,7 @@ func findInterpreter() (*interpreter, error) { if out != "" { return &interpreter{ executable: out, - args: []string{"-c"}, + args: []string{"-e", file.Name()}, }, nil } @@ -97,7 +110,7 @@ func findInterpreter() (*interpreter, error) { if out != "" { return &interpreter{ executable: out, - args: []string{"-Command"}, + args: []string{"-Command", fmt.Sprintf(". %s'", file.Name())}, }, nil } @@ -110,7 +123,7 @@ func findInterpreter() (*interpreter, error) { if out != "" { return &interpreter{ executable: out, - args: []string{"/C"}, + args: []string{"/C", fmt.Sprintf(`CALL "%s"`, file.Name())}, }, nil } diff --git a/libs/cmdio/exec_test.go b/libs/cmdio/exec_test.go index 501c0d9665..adf3b20589 100644 --- a/libs/cmdio/exec_test.go +++ b/libs/cmdio/exec_test.go @@ -28,12 +28,7 @@ func TestExecutorWithInvalidCommand(t *testing.T) { executor := NewCommandExecutor(".") out, err := executor.Exec(context.Background(), "invalid-command") assert.Error(t, err) - - if runtime.GOOS == "windows" { - assert.Contains(t, string(out), "'invalid-command' is not recognized") - } else { - assert.Contains(t, string(out), "invalid-command: command not found") - } + assert.Contains(t, string(out), "invalid-command: command not found") } func TestExecutorWithInvalidCommandWithWindowsLikePath(t *testing.T) { @@ -44,5 +39,5 @@ func TestExecutorWithInvalidCommandWithWindowsLikePath(t *testing.T) { executor := NewCommandExecutor(".") out, err := executor.Exec(context.Background(), `"C:\Program Files\invalid-command.exe"`) assert.Error(t, err) - assert.Contains(t, string(out), "'C:\\Program Files\\invalid-command.exe' is not recognized") + assert.Contains(t, string(out), "C:\\Program Files\\invalid-command.exe: No such file or directory") } From 6d880fc35d68449b7941e9074abea1179ee93b33 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 19 Dec 2023 15:15:51 +0100 Subject: [PATCH 04/14] use correct file extension --- libs/cmdio/exec.go | 54 +++++++++++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/libs/cmdio/exec.go b/libs/cmdio/exec.go index 562f830adc..b4e8d08485 100644 --- a/libs/cmdio/exec.go +++ b/libs/cmdio/exec.go @@ -62,19 +62,6 @@ type interpreter struct { } func wrapInShellCall(command string) (*interpreter, error) { - file, err := os.CreateTemp(os.TempDir(), "cli-exec") - if err != nil { - return nil, err - } - - defer file.Close() - - _, err = io.WriteString(file, command) - file.Close() - if err != nil { - return nil, err - } - // Lookup for bash executable first out, err := exec.LookPath("bash") if err != nil && !errors.Is(err, exec.ErrNotFound) { @@ -82,9 +69,13 @@ func wrapInShellCall(command string) (*interpreter, error) { } if out != "" { + filename, err := createTempScript(command, ".sh") + if err != nil { + return nil, err + } return &interpreter{ executable: out, - args: []string{"-e", file.Name()}, + args: []string{"-e", filename}, }, nil } @@ -95,9 +86,13 @@ func wrapInShellCall(command string) (*interpreter, error) { } if out != "" { + filename, err := createTempScript(command, ".sh") + if err != nil { + return nil, err + } return &interpreter{ executable: out, - args: []string{"-e", file.Name()}, + args: []string{"-e", filename}, }, nil } @@ -108,9 +103,13 @@ func wrapInShellCall(command string) (*interpreter, error) { } if out != "" { + filename, err := createTempScript(command, ".ps1") + if err != nil { + return nil, err + } return &interpreter{ executable: out, - args: []string{"-Command", fmt.Sprintf(". %s'", file.Name())}, + args: []string{"-Command", fmt.Sprintf(". '%s'", filename)}, }, nil } @@ -121,11 +120,32 @@ func wrapInShellCall(command string) (*interpreter, error) { } if out != "" { + filename, err := createTempScript(command, ".cmd") + if err != nil { + return nil, err + } return &interpreter{ executable: out, - args: []string{"/C", fmt.Sprintf(`CALL "%s"`, file.Name())}, + args: []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL "%s"`, filename)}, }, nil } return nil, errors.New("no interpreter found") } + +func createTempScript(command string, extension string) (string, error) { + file, err := os.CreateTemp(os.TempDir(), "cli-exec") + if err != nil { + return "", err + } + + defer file.Close() + + _, err = io.WriteString(file, command) + file.Close() + if err != nil { + return "", err + } + + return file.Name() + extension, nil +} From a21ec7c1daadacdab5c33155ba1580fa43ea9d7a Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 19 Dec 2023 15:31:11 +0100 Subject: [PATCH 05/14] cleanup test files --- bundle/scripts/scripts.go | 7 ++++--- libs/cmdio/exec.go | 27 +++++++++++++++++++++++---- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/bundle/scripts/scripts.go b/bundle/scripts/scripts.go index d41c921787..af73c0eb62 100644 --- a/bundle/scripts/scripts.go +++ b/bundle/scripts/scripts.go @@ -29,7 +29,8 @@ func (m *script) Name() string { } func (m *script) Apply(ctx context.Context, b *bundle.Bundle) error { - cmd, out, err := executeHook(ctx, b, m.scriptHook) + executor := cmdio.NewCommandExecutor(b.Config.Path) + cmd, out, err := executeHook(ctx, executor, b, m.scriptHook) if err != nil { return err } @@ -47,16 +48,16 @@ func (m *script) Apply(ctx context.Context, b *bundle.Bundle) error { line, err = reader.ReadString('\n') } + defer executor.Cleanup() return cmd.Wait() } -func executeHook(ctx context.Context, b *bundle.Bundle, hook config.ScriptHook) (*exec.Cmd, io.Reader, error) { +func executeHook(ctx context.Context, executor *cmdio.Executor, b *bundle.Bundle, hook config.ScriptHook) (*exec.Cmd, io.Reader, error) { command := getCommmand(b, hook) if command == "" { return nil, nil, nil } - executor := cmdio.NewCommandExecutor(b.Config.Path) return executor.StartCommand(ctx, string(command)) } diff --git a/libs/cmdio/exec.go b/libs/cmdio/exec.go index b4e8d08485..506371d10b 100644 --- a/libs/cmdio/exec.go +++ b/libs/cmdio/exec.go @@ -10,21 +10,25 @@ import ( ) type Executor struct { - dir string + dir string + scriptFiles []string } func NewCommandExecutor(dir string) *Executor { return &Executor{ - dir: dir, + dir: dir, + scriptFiles: nil, } } func (e *Executor) StartCommand(ctx context.Context, command string) (*exec.Cmd, io.Reader, error) { interpreter, err := wrapInShellCall(command) + if err != nil { return nil, nil, err } + e.scriptFiles = append(e.scriptFiles, interpreter.scriptFile) cmd := exec.CommandContext(ctx, interpreter.executable, interpreter.args...) cmd.Dir = e.dir @@ -53,12 +57,23 @@ func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { return nil, err } + defer e.Cleanup() return res, cmd.Wait() } +func (e *Executor) Cleanup() { + if e.scriptFiles != nil { + for _, file := range e.scriptFiles { + os.Remove(file) + } + } + e.scriptFiles = nil +} + type interpreter struct { executable string args []string + scriptFile string } func wrapInShellCall(command string) (*interpreter, error) { @@ -76,6 +91,7 @@ func wrapInShellCall(command string) (*interpreter, error) { return &interpreter{ executable: out, args: []string{"-e", filename}, + scriptFile: filename, }, nil } @@ -93,6 +109,7 @@ func wrapInShellCall(command string) (*interpreter, error) { return &interpreter{ executable: out, args: []string{"-e", filename}, + scriptFile: filename, }, nil } @@ -110,6 +127,7 @@ func wrapInShellCall(command string) (*interpreter, error) { return &interpreter{ executable: out, args: []string{"-Command", fmt.Sprintf(". '%s'", filename)}, + scriptFile: filename, }, nil } @@ -127,6 +145,7 @@ func wrapInShellCall(command string) (*interpreter, error) { return &interpreter{ executable: out, args: []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL "%s"`, filename)}, + scriptFile: filename, }, nil } @@ -134,7 +153,7 @@ func wrapInShellCall(command string) (*interpreter, error) { } func createTempScript(command string, extension string) (string, error) { - file, err := os.CreateTemp(os.TempDir(), "cli-exec") + file, err := os.CreateTemp(os.TempDir(), "cli-exec*"+extension) if err != nil { return "", err } @@ -147,5 +166,5 @@ func createTempScript(command string, extension string) (string, error) { return "", err } - return file.Name() + extension, nil + return file.Name(), nil } From 9703eafdfa4e43e60a407dcff215e722de779bd8 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 19 Dec 2023 15:55:27 +0100 Subject: [PATCH 06/14] removed unnecessary interpreters --- libs/cmdio/exec.go | 42 +++--------------------------------------- 1 file changed, 3 insertions(+), 39 deletions(-) diff --git a/libs/cmdio/exec.go b/libs/cmdio/exec.go index 506371d10b..07c49dc1e9 100644 --- a/libs/cmdio/exec.go +++ b/libs/cmdio/exec.go @@ -77,7 +77,7 @@ type interpreter struct { } func wrapInShellCall(command string) (*interpreter, error) { - // Lookup for bash executable first + // Lookup for bash executable first (Linux, MacOS, maybe Windows) out, err := exec.LookPath("bash") if err != nil && !errors.Is(err, exec.ErrNotFound) { return nil, err @@ -95,43 +95,7 @@ func wrapInShellCall(command string) (*interpreter, error) { }, nil } - // Lookup for sh executable - out, err = exec.LookPath("sh") - if err != nil && !errors.Is(err, exec.ErrNotFound) { - return nil, err - } - - if out != "" { - filename, err := createTempScript(command, ".sh") - if err != nil { - return nil, err - } - return &interpreter{ - executable: out, - args: []string{"-e", filename}, - scriptFile: filename, - }, nil - } - - // Lookup for PowerShell executable - out, err = exec.LookPath("powershell") - if err != nil && !errors.Is(err, exec.ErrNotFound) { - return nil, err - } - - if out != "" { - filename, err := createTempScript(command, ".ps1") - if err != nil { - return nil, err - } - return &interpreter{ - executable: out, - args: []string{"-Command", fmt.Sprintf(". '%s'", filename)}, - scriptFile: filename, - }, nil - } - - // Lookup for CMD executable + // Lookup for CMD executable (Windows) out, err = exec.LookPath("cmd") if err != nil && !errors.Is(err, exec.ErrNotFound) { return nil, err @@ -144,7 +108,7 @@ func wrapInShellCall(command string) (*interpreter, error) { } return &interpreter{ executable: out, - args: []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL "%s"`, filename)}, + args: []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL %s`, filename)}, scriptFile: filename, }, nil } From f1f0497042cd7bc9e5351c1df5326cb777315926 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 19 Dec 2023 15:59:00 +0100 Subject: [PATCH 07/14] fixed test --- bundle/scripts/scripts_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bundle/scripts/scripts_test.go b/bundle/scripts/scripts_test.go index 8b7aa0d1b8..b040264caa 100644 --- a/bundle/scripts/scripts_test.go +++ b/bundle/scripts/scripts_test.go @@ -8,6 +8,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/cmdio" "github.com/stretchr/testify/require" ) @@ -21,7 +22,9 @@ func TestExecutesHook(t *testing.T) { }, }, } - _, out, err := executeHook(context.Background(), b, config.ScriptPreBuild) + + executor := cmdio.NewCommandExecutor(b.Config.Path) + _, out, err := executeHook(context.Background(), executor, b, config.ScriptPreBuild) require.NoError(t, err) reader := bufio.NewReader(out) From 18930f92076a96cb9df44eaebccff8c6eff8ecb4 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 20 Dec 2023 12:38:04 +0100 Subject: [PATCH 08/14] added test for cmd on windows --- libs/cmdio/exec.go | 40 ++++++++++++++++++++++++++++++++------ libs/cmdio/exec_test.go | 43 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/libs/cmdio/exec.go b/libs/cmdio/exec.go index 07c49dc1e9..e00123c119 100644 --- a/libs/cmdio/exec.go +++ b/libs/cmdio/exec.go @@ -28,8 +28,12 @@ func (e *Executor) StartCommand(ctx context.Context, command string) (*exec.Cmd, return nil, nil, err } - e.scriptFiles = append(e.scriptFiles, interpreter.scriptFile) - cmd := exec.CommandContext(ctx, interpreter.executable, interpreter.args...) + return e.start(ctx, interpreter) +} + +func (e *Executor) start(ctx context.Context, i *interpreter) (*exec.Cmd, io.Reader, error) { + e.scriptFiles = append(e.scriptFiles, i.scriptFile) + cmd := exec.CommandContext(ctx, i.executable, i.args...) cmd.Dir = e.dir outPipe, err := cmd.StdoutPipe() @@ -43,7 +47,6 @@ func (e *Executor) StartCommand(ctx context.Context, command string) (*exec.Cmd, } return cmd, io.MultiReader(outPipe, errPipe), cmd.Start() - } func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { @@ -77,6 +80,28 @@ type interpreter struct { } func wrapInShellCall(command string) (*interpreter, error) { + interpreter, err := findBashExecutable(command) + if err != nil { + return nil, err + } + + if interpreter != nil { + return interpreter, nil + } + + interpreter, err = findCmdExecutable(command) + if err != nil { + return nil, err + } + + if interpreter != nil { + return interpreter, nil + } + + return nil, errors.New("no interpreter found") +} + +func findBashExecutable(command string) (*interpreter, error) { // Lookup for bash executable first (Linux, MacOS, maybe Windows) out, err := exec.LookPath("bash") if err != nil && !errors.Is(err, exec.ErrNotFound) { @@ -95,8 +120,12 @@ func wrapInShellCall(command string) (*interpreter, error) { }, nil } + return nil, nil +} + +func findCmdExecutable(command string) (*interpreter, error) { // Lookup for CMD executable (Windows) - out, err = exec.LookPath("cmd") + out, err := exec.LookPath("cmd") if err != nil && !errors.Is(err, exec.ErrNotFound) { return nil, err } @@ -112,8 +141,7 @@ func wrapInShellCall(command string) (*interpreter, error) { scriptFile: filename, }, nil } - - return nil, errors.New("no interpreter found") + return nil, nil } func createTempScript(command string, extension string) (string, error) { diff --git a/libs/cmdio/exec_test.go b/libs/cmdio/exec_test.go index adf3b20589..85c6fd11a2 100644 --- a/libs/cmdio/exec_test.go +++ b/libs/cmdio/exec_test.go @@ -2,6 +2,7 @@ package cmdio import ( "context" + "io" "runtime" "testing" @@ -41,3 +42,45 @@ func TestExecutorWithInvalidCommandWithWindowsLikePath(t *testing.T) { assert.Error(t, err) assert.Contains(t, string(out), "C:\\Program Files\\invalid-command.exe: No such file or directory") } + +func TestFindBashExecutableNonWindows(t *testing.T) { + if runtime.GOOS == "windows" { + t.SkipNow() + } + + executable, err := findBashExecutable(`echo "Hello from bash"`) + assert.NoError(t, err) + assert.NotEmpty(t, executable) + + e := NewCommandExecutor(".") + cmd, reader, err := e.start(context.Background(), executable) + assert.NoError(t, err) + assert.NotNil(t, cmd) + assert.NotNil(t, reader) + + out, err := io.ReadAll(reader) + assert.NoError(t, err) + assert.Equal(t, "Hello from bash\n", string(out)) + cmd.Wait() +} + +func TestFindCmdExecutable(t *testing.T) { + if runtime.GOOS != "windows" { + t.SkipNow() + } + + executable, err := findCmdExecutable(`echo "Hello from cmd"`) + assert.NoError(t, err) + assert.NotEmpty(t, executable) + + e := NewCommandExecutor(".") + cmd, reader, err := e.start(context.Background(), executable) + assert.NoError(t, err) + assert.NotNil(t, cmd) + assert.NotNil(t, reader) + + out, err := io.ReadAll(reader) + assert.NoError(t, err) + assert.Equal(t, "Hello from cmd\n", string(out)) + cmd.Wait() +} From a265971fd5a93a0b138fd0b093e89ae34a267af9 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 20 Dec 2023 12:45:31 +0100 Subject: [PATCH 09/14] fixed test --- libs/cmdio/exec_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/cmdio/exec_test.go b/libs/cmdio/exec_test.go index 85c6fd11a2..5abc40e625 100644 --- a/libs/cmdio/exec_test.go +++ b/libs/cmdio/exec_test.go @@ -81,6 +81,6 @@ func TestFindCmdExecutable(t *testing.T) { out, err := io.ReadAll(reader) assert.NoError(t, err) - assert.Equal(t, "Hello from cmd\n", string(out)) + assert.Contains(t, string(out), "Hello from cmd") cmd.Wait() } From 67e6f0210a6c9c8912ad7f31b2ced0b00ed2d2ce Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 20 Dec 2023 16:31:53 +0100 Subject: [PATCH 10/14] addressed feedback --- bundle/config/artifact.go | 9 +- bundle/scripts/scripts.go | 17 ++- bundle/scripts/scripts_test.go | 5 +- libs/cmdio/exec.go | 162 ----------------------- libs/exec/exec.go | 212 ++++++++++++++++++++++++++++++ libs/{cmdio => exec}/exec_test.go | 41 +++--- 6 files changed, 252 insertions(+), 194 deletions(-) delete mode 100644 libs/cmdio/exec.go create mode 100644 libs/exec/exec.go rename libs/{cmdio => exec}/exec_test.go (70%) diff --git a/bundle/config/artifact.go b/bundle/config/artifact.go index f0fb49a538..2a1a92a150 100644 --- a/bundle/config/artifact.go +++ b/bundle/config/artifact.go @@ -6,7 +6,7 @@ import ( "path" "github.com/databricks/cli/bundle/config/paths" - "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/exec" "github.com/databricks/databricks-sdk-go/service/compute" ) @@ -50,8 +50,11 @@ func (a *Artifact) Build(ctx context.Context) ([]byte, error) { return nil, fmt.Errorf("no build property defined") } - exec := cmdio.NewCommandExecutor(a.Path) - return exec.Exec(ctx, a.BuildCommand) + e, err := exec.NewCommandExecutor(a.Path) + if err != nil { + return nil, err + } + return e.Exec(ctx, a.BuildCommand) } func (a *Artifact) NormalisePaths() { diff --git a/bundle/scripts/scripts.go b/bundle/scripts/scripts.go index af73c0eb62..a1e5781966 100644 --- a/bundle/scripts/scripts.go +++ b/bundle/scripts/scripts.go @@ -5,12 +5,12 @@ import ( "context" "fmt" "io" - "os/exec" "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/exec" "github.com/databricks/cli/libs/log" ) @@ -29,12 +29,16 @@ func (m *script) Name() string { } func (m *script) Apply(ctx context.Context, b *bundle.Bundle) error { - executor := cmdio.NewCommandExecutor(b.Config.Path) - cmd, out, err := executeHook(ctx, executor, b, m.scriptHook) + executor, err := exec.NewCommandExecutor(b.Config.Path) if err != nil { return err } - if cmd == nil { + defer executor.Cleanup() + wait, out, err := executeHook(ctx, executor, b, m.scriptHook) + if err != nil { + return err + } + if wait == nil { log.Debugf(ctx, "No script defined for %s, skipping", m.scriptHook) return nil } @@ -48,11 +52,10 @@ func (m *script) Apply(ctx context.Context, b *bundle.Bundle) error { line, err = reader.ReadString('\n') } - defer executor.Cleanup() - return cmd.Wait() + return wait() } -func executeHook(ctx context.Context, executor *cmdio.Executor, b *bundle.Bundle, hook config.ScriptHook) (*exec.Cmd, io.Reader, error) { +func executeHook(ctx context.Context, executor *exec.Executor, b *bundle.Bundle, hook config.ScriptHook) (func() error, io.Reader, error) { command := getCommmand(b, hook) if command == "" { return nil, nil, nil diff --git a/bundle/scripts/scripts_test.go b/bundle/scripts/scripts_test.go index b040264caa..a8835b5998 100644 --- a/bundle/scripts/scripts_test.go +++ b/bundle/scripts/scripts_test.go @@ -8,7 +8,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" - "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/exec" "github.com/stretchr/testify/require" ) @@ -23,7 +23,8 @@ func TestExecutesHook(t *testing.T) { }, } - executor := cmdio.NewCommandExecutor(b.Config.Path) + executor, err := exec.NewCommandExecutor(b.Config.Path) + require.NoError(t, err) _, out, err := executeHook(context.Background(), executor, b, config.ScriptPreBuild) require.NoError(t, err) diff --git a/libs/cmdio/exec.go b/libs/cmdio/exec.go deleted file mode 100644 index e00123c119..0000000000 --- a/libs/cmdio/exec.go +++ /dev/null @@ -1,162 +0,0 @@ -package cmdio - -import ( - "context" - "errors" - "fmt" - "io" - "os" - "os/exec" -) - -type Executor struct { - dir string - scriptFiles []string -} - -func NewCommandExecutor(dir string) *Executor { - return &Executor{ - dir: dir, - scriptFiles: nil, - } -} - -func (e *Executor) StartCommand(ctx context.Context, command string) (*exec.Cmd, io.Reader, error) { - interpreter, err := wrapInShellCall(command) - - if err != nil { - return nil, nil, err - } - - return e.start(ctx, interpreter) -} - -func (e *Executor) start(ctx context.Context, i *interpreter) (*exec.Cmd, io.Reader, error) { - e.scriptFiles = append(e.scriptFiles, i.scriptFile) - cmd := exec.CommandContext(ctx, i.executable, i.args...) - cmd.Dir = e.dir - - outPipe, err := cmd.StdoutPipe() - if err != nil { - return nil, nil, err - } - - errPipe, err := cmd.StderrPipe() - if err != nil { - return nil, nil, err - } - - return cmd, io.MultiReader(outPipe, errPipe), cmd.Start() -} - -func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { - cmd, out, err := e.StartCommand(ctx, command) - if err != nil { - return nil, err - } - - res, err := io.ReadAll(out) - if err != nil { - return nil, err - } - - defer e.Cleanup() - return res, cmd.Wait() -} - -func (e *Executor) Cleanup() { - if e.scriptFiles != nil { - for _, file := range e.scriptFiles { - os.Remove(file) - } - } - e.scriptFiles = nil -} - -type interpreter struct { - executable string - args []string - scriptFile string -} - -func wrapInShellCall(command string) (*interpreter, error) { - interpreter, err := findBashExecutable(command) - if err != nil { - return nil, err - } - - if interpreter != nil { - return interpreter, nil - } - - interpreter, err = findCmdExecutable(command) - if err != nil { - return nil, err - } - - if interpreter != nil { - return interpreter, nil - } - - return nil, errors.New("no interpreter found") -} - -func findBashExecutable(command string) (*interpreter, error) { - // Lookup for bash executable first (Linux, MacOS, maybe Windows) - out, err := exec.LookPath("bash") - if err != nil && !errors.Is(err, exec.ErrNotFound) { - return nil, err - } - - if out != "" { - filename, err := createTempScript(command, ".sh") - if err != nil { - return nil, err - } - return &interpreter{ - executable: out, - args: []string{"-e", filename}, - scriptFile: filename, - }, nil - } - - return nil, nil -} - -func findCmdExecutable(command string) (*interpreter, error) { - // Lookup for CMD executable (Windows) - out, err := exec.LookPath("cmd") - if err != nil && !errors.Is(err, exec.ErrNotFound) { - return nil, err - } - - if out != "" { - filename, err := createTempScript(command, ".cmd") - if err != nil { - return nil, err - } - return &interpreter{ - executable: out, - args: []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL %s`, filename)}, - scriptFile: filename, - }, nil - } - return nil, 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) - file.Close() - if err != nil { - return "", err - } - - return file.Name(), nil -} diff --git a/libs/exec/exec.go b/libs/exec/exec.go new file mode 100644 index 0000000000..5f2fd73d67 --- /dev/null +++ b/libs/exec/exec.go @@ -0,0 +1,212 @@ +package exec + +import ( + "context" + "errors" + "fmt" + "io" + "os" + osexec "os/exec" +) + +type Executor struct { + interpreter interpreter + dir string + scriptFiles []string +} + +func NewCommandExecutor(dir string) (*Executor, error) { + interpreter, err := findInterpreter() + if err != nil { + return nil, err + } + return &Executor{ + interpreter: interpreter, + dir: dir, + scriptFiles: nil, + }, nil +} + +func (e *Executor) StartCommand(ctx context.Context, command string) (func() error, io.Reader, error) { + e.interpreter.prepare(command) + return e.start(ctx) +} + +func (e *Executor) start(ctx context.Context) (func() error, io.Reader, error) { + e.scriptFiles = append(e.scriptFiles, e.interpreter.getScriptFile()) + cmd := osexec.CommandContext(ctx, e.interpreter.getExecutable(), e.interpreter.getArgs()...) + cmd.Dir = e.dir + + outPipe, err := cmd.StdoutPipe() + if err != nil { + return nil, nil, err + } + + errPipe, err := cmd.StderrPipe() + if err != nil { + return nil, nil, err + } + + return cmd.Wait, io.MultiReader(outPipe, errPipe), cmd.Start() +} + +func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { + wait, out, err := e.StartCommand(ctx, command) + if err != nil { + return nil, err + } + + res, err := io.ReadAll(out) + if err != nil { + return nil, err + } + + defer e.Cleanup() + return res, wait() +} + +func (e *Executor) Cleanup() { + if e.scriptFiles != nil { + for _, file := range e.scriptFiles { + os.Remove(file) + } + } + e.scriptFiles = nil +} + +func findInterpreter() (interpreter, error) { + interpreter, err := findBashExecutable() + if err != nil { + return nil, err + } + + if interpreter != nil { + return interpreter, nil + } + + interpreter, err = findCmdExecutable() + if err != nil { + return nil, err + } + + if interpreter != nil { + return interpreter, nil + } + + return nil, errors.New("no interpreter found") +} + +type interpreter interface { + prepare(command string) error + getExecutable() string + getArgs() []string + getScriptFile() string +} + +type bashInterpreter struct { + executable string + args []string + scriptFile string +} + +func (b *bashInterpreter) prepare(command string) error { + filename, err := createTempScript(command, ".sh") + if err != nil { + return err + } + + b.args = []string{"-e", filename} + b.scriptFile = filename + + return nil +} + +func (b *bashInterpreter) getExecutable() string { + return b.executable +} + +func (b *bashInterpreter) getArgs() []string { + return b.args +} + +func (b *bashInterpreter) getScriptFile() string { + return b.scriptFile +} + +type cmdInterpreter struct { + executable string + args []string + scriptFile string +} + +func (c *cmdInterpreter) prepare(command string) error { + filename, err := createTempScript(command, ".cmd") + if err != nil { + return err + } + + c.args = []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL %s`, filename)} + c.scriptFile = filename + + return nil +} + +func (c *cmdInterpreter) getExecutable() string { + return c.executable +} + +func (c *cmdInterpreter) getArgs() []string { + return c.args +} + +func (c *cmdInterpreter) getScriptFile() string { + return c.scriptFile +} + +func findBashExecutable() (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 findCmdExecutable() (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 +} diff --git a/libs/cmdio/exec_test.go b/libs/exec/exec_test.go similarity index 70% rename from libs/cmdio/exec_test.go rename to libs/exec/exec_test.go index 5abc40e625..eabe759bae 100644 --- a/libs/cmdio/exec_test.go +++ b/libs/exec/exec_test.go @@ -1,8 +1,7 @@ -package cmdio +package exec import ( "context" - "io" "runtime" "testing" @@ -10,7 +9,8 @@ import ( ) func TestExecutorWithSimpleInput(t *testing.T) { - executor := NewCommandExecutor(".") + executor, err := NewCommandExecutor(".") + assert.NoError(t, err) out, err := executor.Exec(context.Background(), "echo 'Hello'") assert.NoError(t, err) assert.NotNil(t, out) @@ -18,7 +18,8 @@ func TestExecutorWithSimpleInput(t *testing.T) { } func TestExecutorWithComplexInput(t *testing.T) { - executor := NewCommandExecutor(".") + executor, err := NewCommandExecutor(".") + assert.NoError(t, err) out, err := executor.Exec(context.Background(), "echo 'Hello' && echo 'World'") assert.NoError(t, err) assert.NotNil(t, out) @@ -26,7 +27,8 @@ func TestExecutorWithComplexInput(t *testing.T) { } func TestExecutorWithInvalidCommand(t *testing.T) { - executor := NewCommandExecutor(".") + executor, err := NewCommandExecutor(".") + assert.NoError(t, err) out, err := executor.Exec(context.Background(), "invalid-command") assert.Error(t, err) assert.Contains(t, string(out), "invalid-command: command not found") @@ -37,7 +39,8 @@ func TestExecutorWithInvalidCommandWithWindowsLikePath(t *testing.T) { t.SkipNow() } - executor := NewCommandExecutor(".") + executor, err := NewCommandExecutor(".") + assert.NoError(t, err) out, err := executor.Exec(context.Background(), `"C:\Program Files\invalid-command.exe"`) assert.Error(t, err) assert.Contains(t, string(out), "C:\\Program Files\\invalid-command.exe: No such file or directory") @@ -48,20 +51,19 @@ func TestFindBashExecutableNonWindows(t *testing.T) { t.SkipNow() } - executable, err := findBashExecutable(`echo "Hello from bash"`) + executable, err := findBashExecutable() assert.NoError(t, err) assert.NotEmpty(t, executable) - e := NewCommandExecutor(".") - cmd, reader, err := e.start(context.Background(), executable) + e, err := NewCommandExecutor(".") assert.NoError(t, err) - assert.NotNil(t, cmd) - assert.NotNil(t, reader) + e.interpreter = executable - out, err := io.ReadAll(reader) 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)) - cmd.Wait() } func TestFindCmdExecutable(t *testing.T) { @@ -69,18 +71,17 @@ func TestFindCmdExecutable(t *testing.T) { t.SkipNow() } - executable, err := findCmdExecutable(`echo "Hello from cmd"`) + executable, err := findCmdExecutable() assert.NoError(t, err) assert.NotEmpty(t, executable) - e := NewCommandExecutor(".") - cmd, reader, err := e.start(context.Background(), executable) + e, err := NewCommandExecutor(".") assert.NoError(t, err) - assert.NotNil(t, cmd) - assert.NotNil(t, reader) + e.interpreter = executable - out, err := io.ReadAll(reader) 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") - cmd.Wait() } From 36780494365002e00039eb70d65f26d7429e0647 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 21 Dec 2023 12:33:10 +0100 Subject: [PATCH 11/14] cleanup in goroutine --- bundle/scripts/scripts.go | 17 ++++--- libs/exec/exec.go | 101 ++++++++++++++++++++++++++------------ libs/exec/exec_test.go | 32 +++++++++--- 3 files changed, 104 insertions(+), 46 deletions(-) diff --git a/bundle/scripts/scripts.go b/bundle/scripts/scripts.go index a1e5781966..2f13bc19fc 100644 --- a/bundle/scripts/scripts.go +++ b/bundle/scripts/scripts.go @@ -33,12 +33,12 @@ func (m *script) Apply(ctx context.Context, b *bundle.Bundle) error { if err != nil { return err } - defer executor.Cleanup() - wait, out, err := executeHook(ctx, executor, b, m.scriptHook) + + cmd, out, err := executeHook(ctx, executor, b, m.scriptHook) if err != nil { return err } - if wait == nil { + if cmd == nil { log.Debugf(ctx, "No script defined for %s, skipping", m.scriptHook) return nil } @@ -52,16 +52,21 @@ func (m *script) Apply(ctx context.Context, b *bundle.Bundle) error { line, err = reader.ReadString('\n') } - return wait() + return cmd.Wait() } -func executeHook(ctx context.Context, executor *exec.Executor, b *bundle.Bundle, hook config.ScriptHook) (func() error, io.Reader, error) { +func executeHook(ctx context.Context, executor *exec.Executor, b *bundle.Bundle, hook config.ScriptHook) (exec.Command, io.Reader, error) { command := getCommmand(b, hook) if command == "" { return nil, nil, nil } - return executor.StartCommand(ctx, string(command)) + cmd, err := executor.StartCommand(ctx, string(command)) + if err != nil { + return nil, nil, err + } + + return cmd, io.MultiReader(cmd.Stdout(), cmd.Stderr()), nil } func getCommmand(b *bundle.Bundle, hook config.ScriptHook) config.Command { diff --git a/libs/exec/exec.go b/libs/exec/exec.go index 5f2fd73d67..e769ce6819 100644 --- a/libs/exec/exec.go +++ b/libs/exec/exec.go @@ -9,10 +9,44 @@ import ( osexec "os/exec" ) +type Command interface { + // Wait for command to terminate. It must have been previously started. + Wait() error + + // StdinPipe returns a pipe that will be connected to the command's standard input when the command starts. + Stdout() io.ReadCloser + + // StderrPipe returns a pipe that will be connected to the command's standard error when the command starts. + Stderr() io.ReadCloser +} + +type command struct { + done chan bool + err chan error + stdout io.ReadCloser + stderr io.ReadCloser +} + +func (c *command) Wait() error { + select { + case <-c.done: + return nil + case err := <-c.err: + return err + } +} + +func (c *command) Stdout() io.ReadCloser { + return c.stdout +} + +func (c *command) Stderr() io.ReadCloser { + return c.stderr +} + type Executor struct { interpreter interpreter dir string - scriptFiles []string } func NewCommandExecutor(dir string) (*Executor, error) { @@ -23,59 +57,62 @@ func NewCommandExecutor(dir string) (*Executor, error) { return &Executor{ interpreter: interpreter, dir: dir, - scriptFiles: nil, }, nil } -func (e *Executor) StartCommand(ctx context.Context, command string) (func() error, io.Reader, error) { +func (e *Executor) StartCommand(ctx context.Context, command string) (Command, error) { e.interpreter.prepare(command) return e.start(ctx) } -func (e *Executor) start(ctx context.Context) (func() error, io.Reader, error) { - e.scriptFiles = append(e.scriptFiles, e.interpreter.getScriptFile()) +func (e *Executor) start(ctx context.Context) (Command, error) { cmd := osexec.CommandContext(ctx, e.interpreter.getExecutable(), e.interpreter.getArgs()...) cmd.Dir = e.dir - outPipe, err := cmd.StdoutPipe() + stdout, err := cmd.StdoutPipe() if err != nil { - return nil, nil, err + return nil, err } - errPipe, err := cmd.StderrPipe() + stderr, err := cmd.StderrPipe() if err != nil { - return nil, nil, err + return nil, err } - return cmd.Wait, io.MultiReader(outPipe, errPipe), cmd.Start() + err = cmd.Start() + + done := make(chan bool) + errCh := make(chan error) + + go func() { + err := cmd.Wait() + e.interpreter.cleanup() + if err != nil { + errCh <- err + } + close(done) + close(errCh) + }() + + return &command{done, errCh, stdout, stderr}, err } func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { - wait, out, err := e.StartCommand(ctx, command) + cmd, err := e.StartCommand(ctx, command) if err != nil { return nil, err } - res, err := io.ReadAll(out) + res, err := io.ReadAll(io.MultiReader(cmd.Stdout(), cmd.Stderr())) if err != nil { return nil, err } - defer e.Cleanup() - return res, wait() -} - -func (e *Executor) Cleanup() { - if e.scriptFiles != nil { - for _, file := range e.scriptFiles { - os.Remove(file) - } - } - e.scriptFiles = nil + return res, cmd.Wait() } func findInterpreter() (interpreter, error) { - interpreter, err := findBashExecutable() + interpreter, err := findBashInterpreter() if err != nil { return nil, err } @@ -84,7 +121,7 @@ func findInterpreter() (interpreter, error) { return interpreter, nil } - interpreter, err = findCmdExecutable() + interpreter, err = findCmdInterpreter() if err != nil { return nil, err } @@ -100,7 +137,7 @@ type interpreter interface { prepare(command string) error getExecutable() string getArgs() []string - getScriptFile() string + cleanup() error } type bashInterpreter struct { @@ -129,8 +166,8 @@ func (b *bashInterpreter) getArgs() []string { return b.args } -func (b *bashInterpreter) getScriptFile() string { - return b.scriptFile +func (b *bashInterpreter) cleanup() error { + return os.Remove(b.scriptFile) } type cmdInterpreter struct { @@ -159,11 +196,11 @@ func (c *cmdInterpreter) getArgs() []string { return c.args } -func (c *cmdInterpreter) getScriptFile() string { - return c.scriptFile +func (c *cmdInterpreter) cleanup() error { + return os.Remove(c.scriptFile) } -func findBashExecutable() (interpreter, error) { +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) { @@ -178,7 +215,7 @@ func findBashExecutable() (interpreter, error) { return &bashInterpreter{executable: out}, nil } -func findCmdExecutable() (interpreter, error) { +func findCmdInterpreter() (interpreter, error) { // Lookup for CMD executable (Windows) out, err := osexec.LookPath("cmd") if err != nil && !errors.Is(err, osexec.ErrNotFound) { diff --git a/libs/exec/exec_test.go b/libs/exec/exec_test.go index eabe759bae..68534698c8 100644 --- a/libs/exec/exec_test.go +++ b/libs/exec/exec_test.go @@ -46,18 +46,18 @@ func TestExecutorWithInvalidCommandWithWindowsLikePath(t *testing.T) { assert.Contains(t, string(out), "C:\\Program Files\\invalid-command.exe: No such file or directory") } -func TestFindBashExecutableNonWindows(t *testing.T) { +func TestFindBashInterpreterNonWindows(t *testing.T) { if runtime.GOOS == "windows" { t.SkipNow() } - executable, err := findBashExecutable() + interpreter, err := findBashInterpreter() assert.NoError(t, err) - assert.NotEmpty(t, executable) + assert.NotEmpty(t, interpreter) e, err := NewCommandExecutor(".") assert.NoError(t, err) - e.interpreter = executable + e.interpreter = interpreter assert.NoError(t, err) out, err := e.Exec(context.Background(), `echo "Hello from bash"`) @@ -66,18 +66,18 @@ func TestFindBashExecutableNonWindows(t *testing.T) { assert.Equal(t, "Hello from bash\n", string(out)) } -func TestFindCmdExecutable(t *testing.T) { +func TestFindCmdInterpreter(t *testing.T) { if runtime.GOOS != "windows" { t.SkipNow() } - executable, err := findCmdExecutable() + interpreter, err := findCmdInterpreter() assert.NoError(t, err) - assert.NotEmpty(t, executable) + assert.NotEmpty(t, interpreter) e, err := NewCommandExecutor(".") assert.NoError(t, err) - e.interpreter = executable + e.interpreter = interpreter assert.NoError(t, err) out, err := e.Exec(context.Background(), `echo "Hello from cmd"`) @@ -85,3 +85,19 @@ func TestFindCmdExecutable(t *testing.T) { assert.Contains(t, string(out), "Hello from cmd") } + +func TestExecutorCleanupsTempFiles(t *testing.T) { + executor, err := NewCommandExecutor(".") + assert.NoError(t, err) + + cmd, err := executor.StartCommand(context.Background(), "echo 'Hello'") + args := executor.interpreter.getArgs() + assert.NoError(t, err) + + fileName := args[1] + assert.FileExists(t, fileName) + + err = cmd.Wait() + assert.NoError(t, err) + assert.NoFileExists(t, fileName) +} From 2b75ed0d1fa19ff2daf5277cea36ba79a0c89971 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 21 Dec 2023 15:34:26 +0100 Subject: [PATCH 12/14] safe async runs --- libs/exec/exec.go | 171 ++++----------------------------------- libs/exec/exec_test.go | 39 ++++++++- libs/exec/interpreter.go | 132 ++++++++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 159 deletions(-) create mode 100644 libs/exec/interpreter.go diff --git a/libs/exec/exec.go b/libs/exec/exec.go index e769ce6819..21b02b3934 100644 --- a/libs/exec/exec.go +++ b/libs/exec/exec.go @@ -2,10 +2,7 @@ package exec import ( "context" - "errors" - "fmt" "io" - "os" osexec "os/exec" ) @@ -21,19 +18,18 @@ type Command interface { } type command struct { - done chan bool err chan error stdout io.ReadCloser stderr io.ReadCloser } func (c *command) Wait() error { - select { - case <-c.done: - return nil - case err := <-c.err: + err, ok := <-c.err + // If there's a value in the channel, it means that the command finished with an error + if ok { return err } + return nil } func (c *command) Stdout() io.ReadCloser { @@ -61,12 +57,15 @@ func NewCommandExecutor(dir string) (*Executor, error) { } func (e *Executor) StartCommand(ctx context.Context, command string) (Command, error) { - e.interpreter.prepare(command) - return e.start(ctx) + ec, err := e.interpreter.prepare(command) + if err != nil { + return nil, err + } + return e.start(ctx, ec) } -func (e *Executor) start(ctx context.Context) (Command, error) { - cmd := osexec.CommandContext(ctx, e.interpreter.getExecutable(), e.interpreter.getArgs()...) +func (e *Executor) start(ctx context.Context, ec *execContext) (Command, error) { + cmd := osexec.CommandContext(ctx, ec.executable, ec.args...) cmd.Dir = e.dir stdout, err := cmd.StdoutPipe() @@ -80,21 +79,18 @@ func (e *Executor) start(ctx context.Context) (Command, error) { } err = cmd.Start() - - done := make(chan bool) errCh := make(chan error) - go func() { + go func(cmd *osexec.Cmd, errCh chan error) { err := cmd.Wait() - e.interpreter.cleanup() + e.interpreter.cleanup(ec) if err != nil { errCh <- err } - close(done) close(errCh) - }() + }(cmd, errCh) - return &command{done, errCh, stdout, stderr}, err + return &command{errCh, stdout, stderr}, err } func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { @@ -110,140 +106,3 @@ func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { return res, cmd.Wait() } - -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") -} - -type interpreter interface { - prepare(command string) error - getExecutable() string - getArgs() []string - cleanup() error -} - -type bashInterpreter struct { - executable string - args []string - scriptFile string -} - -func (b *bashInterpreter) prepare(command string) error { - filename, err := createTempScript(command, ".sh") - if err != nil { - return err - } - - b.args = []string{"-e", filename} - b.scriptFile = filename - - return nil -} - -func (b *bashInterpreter) getExecutable() string { - return b.executable -} - -func (b *bashInterpreter) getArgs() []string { - return b.args -} - -func (b *bashInterpreter) cleanup() error { - return os.Remove(b.scriptFile) -} - -type cmdInterpreter struct { - executable string - args []string - scriptFile string -} - -func (c *cmdInterpreter) prepare(command string) error { - filename, err := createTempScript(command, ".cmd") - if err != nil { - return err - } - - c.args = []string{"/D", "/E:ON", "/V:OFF", "/S", "/C", fmt.Sprintf(`CALL %s`, filename)} - c.scriptFile = filename - - return nil -} - -func (c *cmdInterpreter) getExecutable() string { - return c.executable -} - -func (c *cmdInterpreter) getArgs() []string { - return c.args -} - -func (c *cmdInterpreter) cleanup() error { - return os.Remove(c.scriptFile) -} - -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 -} diff --git a/libs/exec/exec_test.go b/libs/exec/exec_test.go index 68534698c8..a1d8d6ffe1 100644 --- a/libs/exec/exec_test.go +++ b/libs/exec/exec_test.go @@ -2,7 +2,10 @@ package exec import ( "context" + "fmt" + "io" "runtime" + "sync" "testing" "github.com/stretchr/testify/assert" @@ -90,14 +93,44 @@ func TestExecutorCleanupsTempFiles(t *testing.T) { executor, err := NewCommandExecutor(".") assert.NoError(t, err) - cmd, err := executor.StartCommand(context.Background(), "echo 'Hello'") - args := executor.interpreter.getArgs() + ec, err := executor.interpreter.prepare("echo 'Hello'") assert.NoError(t, err) - fileName := args[1] + cmd, err := executor.start(context.Background(), ec) + assert.NoError(t, err) + + fileName := ec.args[1] assert.FileExists(t, fileName) err = cmd.Wait() assert.NoError(t, err) assert.NoFileExists(t, fileName) } + +func TestMultipleCommandsRunInParrallel(t *testing.T) { + executor, err := NewCommandExecutor(".") + assert.NoError(t, err) + + const count = 5 + var wg sync.WaitGroup + + for i := 0; i < count; i++ { + wg.Add(1) + cmd, err := executor.StartCommand(context.Background(), fmt.Sprintf("echo 'Hello %d'", i)) + go func(cmd Command, i int) { + defer wg.Done() + + stdout := cmd.Stdout() + out, err := io.ReadAll(stdout) + assert.NoError(t, err) + + err = cmd.Wait() + assert.NoError(t, err) + + assert.Equal(t, fmt.Sprintf("Hello %d\n", i), string(out)) + }(cmd, i) + assert.NoError(t, err) + } + + wg.Wait() +} diff --git a/libs/exec/interpreter.go b/libs/exec/interpreter.go new file mode 100644 index 0000000000..bb0f385799 --- /dev/null +++ b/libs/exec/interpreter.go @@ -0,0 +1,132 @@ +package exec + +import ( + "errors" + "fmt" + "io" + "os" + osexec "os/exec" +) + +type interpreter interface { + prepare(string) (*execContext, error) + cleanup(*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 +} + +func (b *bashInterpreter) cleanup(ec *execContext) error { + return os.Remove(ec.scriptFile) +} + +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 (c *cmdInterpreter) cleanup(ec *execContext) error { + return os.Remove(ec.scriptFile) +} + +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 +} From 35c718fdf93f19e1ad1126555e3654c8f1c6b028 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 21 Dec 2023 15:40:13 +0100 Subject: [PATCH 13/14] fix cmd.Wait --- libs/exec/exec_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/exec/exec_test.go b/libs/exec/exec_test.go index a1d8d6ffe1..edffcae913 100644 --- a/libs/exec/exec_test.go +++ b/libs/exec/exec_test.go @@ -117,6 +117,9 @@ func TestMultipleCommandsRunInParrallel(t *testing.T) { for i := 0; i < count; i++ { wg.Add(1) cmd, err := executor.StartCommand(context.Background(), fmt.Sprintf("echo 'Hello %d'", i)) + // Execute cmd.Wait only when all goroutines are done + defer cmd.Wait() + go func(cmd Command, i int) { defer wg.Done() @@ -124,9 +127,6 @@ func TestMultipleCommandsRunInParrallel(t *testing.T) { out, err := io.ReadAll(stdout) assert.NoError(t, err) - err = cmd.Wait() - assert.NoError(t, err) - assert.Equal(t, fmt.Sprintf("Hello %d\n", i), string(out)) }(cmd, i) assert.NoError(t, err) From a101ef54f001de06e2cb7c4c2b3de9df9b45997e Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 21 Dec 2023 15:58:05 +0100 Subject: [PATCH 14/14] removed sync logic from command --- libs/exec/exec.go | 31 ++++++++++++------------------- libs/exec/exec_test.go | 6 +++--- libs/exec/interpreter.go | 9 --------- 3 files changed, 15 insertions(+), 31 deletions(-) diff --git a/libs/exec/exec.go b/libs/exec/exec.go index 21b02b3934..7ef6762b8a 100644 --- a/libs/exec/exec.go +++ b/libs/exec/exec.go @@ -3,6 +3,7 @@ package exec import ( "context" "io" + "os" osexec "os/exec" ) @@ -18,17 +19,21 @@ type Command interface { } type command struct { - err chan error - stdout io.ReadCloser - stderr io.ReadCloser + cmd *osexec.Cmd + execContext *execContext + stdout io.ReadCloser + stderr io.ReadCloser } func (c *command) Wait() error { - err, ok := <-c.err - // If there's a value in the channel, it means that the command finished with an error - if ok { + // After the command has finished (cmd.Wait call), remove the temporary script file + defer os.Remove(c.execContext.scriptFile) + + err := c.cmd.Wait() + if err != nil { return err } + return nil } @@ -78,19 +83,7 @@ func (e *Executor) start(ctx context.Context, ec *execContext) (Command, error) return nil, err } - err = cmd.Start() - errCh := make(chan error) - - go func(cmd *osexec.Cmd, errCh chan error) { - err := cmd.Wait() - e.interpreter.cleanup(ec) - if err != nil { - errCh <- err - } - close(errCh) - }(cmd, errCh) - - return &command{errCh, stdout, stderr}, err + return &command{cmd, ec, stdout, stderr}, cmd.Start() } func (e *Executor) Exec(ctx context.Context, command string) ([]byte, error) { diff --git a/libs/exec/exec_test.go b/libs/exec/exec_test.go index edffcae913..a1d8d6ffe1 100644 --- a/libs/exec/exec_test.go +++ b/libs/exec/exec_test.go @@ -117,9 +117,6 @@ func TestMultipleCommandsRunInParrallel(t *testing.T) { for i := 0; i < count; i++ { wg.Add(1) cmd, err := executor.StartCommand(context.Background(), fmt.Sprintf("echo 'Hello %d'", i)) - // Execute cmd.Wait only when all goroutines are done - defer cmd.Wait() - go func(cmd Command, i int) { defer wg.Done() @@ -127,6 +124,9 @@ func TestMultipleCommandsRunInParrallel(t *testing.T) { out, err := io.ReadAll(stdout) assert.NoError(t, err) + err = cmd.Wait() + assert.NoError(t, err) + assert.Equal(t, fmt.Sprintf("Hello %d\n", i), string(out)) }(cmd, i) assert.NoError(t, err) diff --git a/libs/exec/interpreter.go b/libs/exec/interpreter.go index bb0f385799..e600e47f6d 100644 --- a/libs/exec/interpreter.go +++ b/libs/exec/interpreter.go @@ -10,7 +10,6 @@ import ( type interpreter interface { prepare(string) (*execContext, error) - cleanup(*execContext) error } type execContext struct { @@ -36,10 +35,6 @@ func (b *bashInterpreter) prepare(command string) (*execContext, error) { }, nil } -func (b *bashInterpreter) cleanup(ec *execContext) error { - return os.Remove(ec.scriptFile) -} - type cmdInterpreter struct { executable string } @@ -57,10 +52,6 @@ func (c *cmdInterpreter) prepare(command string) (*execContext, error) { }, nil } -func (c *cmdInterpreter) cleanup(ec *execContext) error { - return os.Remove(ec.scriptFile) -} - func findInterpreter() (interpreter, error) { interpreter, err := findBashInterpreter() if err != nil {