From 92142a6fc399757cdba4021d5259050b2f1205a2 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 5 Jun 2025 16:44:02 +0200 Subject: [PATCH 01/10] Use OS aware runner instead of bash for run-local command --- cmd/workspace/apps/run_local.go | 2 +- libs/apps/apps.go | 6 ++++-- libs/apps/python.go | 20 ++++++++++++-------- libs/apps/python_test.go | 3 ++- 4 files changed, 19 insertions(+), 12 deletions(-) diff --git a/cmd/workspace/apps/run_local.go b/cmd/workspace/apps/run_local.go index cfc4ac8fb5..adc200ef3c 100644 --- a/cmd/workspace/apps/run_local.go +++ b/cmd/workspace/apps/run_local.go @@ -53,7 +53,7 @@ func setupWorkspaceAndConfig(cmd *cobra.Command, entryPoint string, appPort int) func setupApp(cmd *cobra.Command, config *apps.Config, spec *apps.AppSpec, customEnv []string, prepareEnvironment bool) (apps.App, []string, error) { ctx := cmd.Context() cfg := cmdctx.ConfigUsed(ctx) - app := apps.NewApp(config, spec) + app := apps.NewApp(ctx, config, spec) env := auth.ProcessEnv(cfg) if cfg.Profile != "" { env = append(env, "DATABRICKS_CONFIG_PROFILE="+cfg.Profile) diff --git a/libs/apps/apps.go b/libs/apps/apps.go index 7eeaf7f00c..420db61c52 100644 --- a/libs/apps/apps.go +++ b/libs/apps/apps.go @@ -1,12 +1,14 @@ package apps +import "context" + type App interface { PrepareEnvironment() error GetCommand(bool) ([]string, error) } -func NewApp(config *Config, spec *AppSpec) App { +func NewApp(ctx context.Context, config *Config, spec *AppSpec) App { // We only support python apps for now, but later we can add more types // based on AppSpec - return NewPythonApp(config, spec) + return NewPythonApp(ctx, config, spec) } diff --git a/libs/apps/python.go b/libs/apps/python.go index 964b23e6b4..d01b72ba40 100644 --- a/libs/apps/python.go +++ b/libs/apps/python.go @@ -1,13 +1,15 @@ package apps import ( + "context" "errors" "fmt" "os" - "os/exec" "path/filepath" "strconv" "strings" + + "github.com/databricks/cli/libs/exec" ) const DEBUG_PORT = "5678" @@ -37,12 +39,13 @@ var defaultLibraries = []string{ } type PythonApp struct { + ctx context.Context config *Config spec *AppSpec uvArgs []string } -func NewPythonApp(config *Config, spec *AppSpec) *PythonApp { +func NewPythonApp(ctx context.Context, config *Config, spec *AppSpec) *PythonApp { if config.DebugPort == "" { config.DebugPort = DEBUG_PORT } @@ -133,11 +136,12 @@ func (p *PythonApp) enableDebugging() { } } -// runCommand executes the given command as a bash command and returns any error. +// runCommand executes the given command and returns any error. func (p *PythonApp) runCommand(args []string) error { - cmd := exec.Command("bash", "-c", strings.Join(args, " ")) - cmd.Dir = p.spec.config.AppPath - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - return cmd.Run() + e, err := exec.NewCommandExecutor(p.config.AppPath) + if err != nil { + return err + } + _, err = e.Exec(p.ctx, strings.Join(args, " ")) + return err } diff --git a/libs/apps/python_test.go b/libs/apps/python_test.go index e2f9709ce9..dcbdc38aea 100644 --- a/libs/apps/python_test.go +++ b/libs/apps/python_test.go @@ -1,6 +1,7 @@ package apps import ( + "context" "path/filepath" "testing" @@ -88,7 +89,7 @@ func TestPythonAppGetCommand(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { config, spec := tt.setup() - app := NewPythonApp(config, spec) + app := NewPythonApp(context.Background(), config, spec) cmd, err := app.GetCommand(tt.debug) if !tt.wantErr { From f6c821148eec67adf369e9b58c2172c7ac082ff8 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 5 Jun 2025 17:33:22 +0200 Subject: [PATCH 02/10] copy stdout and stderr --- libs/apps/python.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/libs/apps/python.go b/libs/apps/python.go index d01b72ba40..5da6771712 100644 --- a/libs/apps/python.go +++ b/libs/apps/python.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "os" "path/filepath" "strconv" @@ -49,7 +50,7 @@ func NewPythonApp(ctx context.Context, config *Config, spec *AppSpec) *PythonApp if config.DebugPort == "" { config.DebugPort = DEBUG_PORT } - return &PythonApp{config: config, spec: spec} + return &PythonApp{ctx: ctx, config: config, spec: spec} } // PrepareEnvironment creates a Python virtual environment using uv and installs required dependencies. @@ -142,6 +143,13 @@ func (p *PythonApp) runCommand(args []string) error { if err != nil { return err } - _, err = e.Exec(p.ctx, strings.Join(args, " ")) - return err + + cmd, err := e.StartCommand(p.ctx, strings.Join(args, " ")) + if err != nil { + return err + } + go io.Copy(os.Stdout, cmd.Stdout()) + go io.Copy(os.Stderr, cmd.Stderr()) + + return cmd.Wait() } From 4914017e08012acc2c235ff1fcb5006826093fdd Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 5 Jun 2025 17:40:17 +0200 Subject: [PATCH 03/10] handle copy errors --- libs/apps/python.go | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/libs/apps/python.go b/libs/apps/python.go index 5da6771712..94fca7a47c 100644 --- a/libs/apps/python.go +++ b/libs/apps/python.go @@ -148,8 +148,30 @@ func (p *PythonApp) runCommand(args []string) error { if err != nil { return err } - go io.Copy(os.Stdout, cmd.Stdout()) - go io.Copy(os.Stderr, cmd.Stderr()) - return cmd.Wait() + // Create error channels to capture io.Copy errors + stdoutErr := make(chan error, 1) + stderrErr := make(chan error, 1) + + go func() { + _, err := io.Copy(os.Stdout, cmd.Stdout()) + stdoutErr <- err + }() + go func() { + _, err := io.Copy(os.Stderr, cmd.Stderr()) + stderrErr <- err + }() + + // Wait for command completion + cmdErr := cmd.Wait() + + // Check for io.Copy errors + if err := <-stdoutErr; err != nil { + return fmt.Errorf("error copying stdout: %w", err) + } + if err := <-stderrErr; err != nil { + return fmt.Errorf("error copying stderr: %w", err) + } + + return cmdErr } From 36ba29debb4b0c57d99947f9235f51e4919c1565 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 6 Jun 2025 11:02:31 +0200 Subject: [PATCH 04/10] add requirements.txt to acc test --- acceptance/cmd/workspace/apps/run-local/app/requirements.txt | 1 + 1 file changed, 1 insertion(+) create mode 100644 acceptance/cmd/workspace/apps/run-local/app/requirements.txt diff --git a/acceptance/cmd/workspace/apps/run-local/app/requirements.txt b/acceptance/cmd/workspace/apps/run-local/app/requirements.txt new file mode 100644 index 0000000000..aa4b1291cf --- /dev/null +++ b/acceptance/cmd/workspace/apps/run-local/app/requirements.txt @@ -0,0 +1 @@ +Flask==3.1.1 From ba19aa4ed0fab94c50298712c16b2dcc43c03f83 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 6 Jun 2025 11:22:37 +0200 Subject: [PATCH 05/10] enable test output --- acceptance/cmd/workspace/apps/run-local/script | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/cmd/workspace/apps/run-local/script b/acceptance/cmd/workspace/apps/run-local/script index 29990012bd..411ce8c469 100755 --- a/acceptance/cmd/workspace/apps/run-local/script +++ b/acceptance/cmd/workspace/apps/run-local/script @@ -4,7 +4,7 @@ trace errcode $CLI apps run-local --entry-point value-from.yml 2>&1 # We first run the command with different entry point which starts unblocking script # so we don't need to start it in background. It will install the dependencies as part of the command -trace $CLI apps run-local --prepare-environment --entry-point test.yml 2>&1 | grep -w "Hello, world" +trace $CLI apps run-local --prepare-environment --entry-point test.yml 2>&1 # Get 3 unique ports sequentially to avoid conflicts PORTS=$(allocate_ports.py 3 | tr -d '\r') From 064b645dd60fda79115645027a6a1c2bfd080a12 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 6 Jun 2025 11:50:00 +0200 Subject: [PATCH 06/10] fix path to requirements.txt --- acceptance/cmd/workspace/apps/run-local/script | 2 +- libs/apps/python.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/acceptance/cmd/workspace/apps/run-local/script b/acceptance/cmd/workspace/apps/run-local/script index 411ce8c469..29990012bd 100755 --- a/acceptance/cmd/workspace/apps/run-local/script +++ b/acceptance/cmd/workspace/apps/run-local/script @@ -4,7 +4,7 @@ trace errcode $CLI apps run-local --entry-point value-from.yml 2>&1 # We first run the command with different entry point which starts unblocking script # so we don't need to start it in background. It will install the dependencies as part of the command -trace $CLI apps run-local --prepare-environment --entry-point test.yml 2>&1 +trace $CLI apps run-local --prepare-environment --entry-point test.yml 2>&1 | grep -w "Hello, world" # Get 3 unique ports sequentially to avoid conflicts PORTS=$(allocate_ports.py 3 | tr -d '\r') diff --git a/libs/apps/python.go b/libs/apps/python.go index 94fca7a47c..e64941ff90 100644 --- a/libs/apps/python.go +++ b/libs/apps/python.go @@ -72,7 +72,9 @@ func (p *PythonApp) PrepareEnvironment() error { // Install requirements if they exist if _, err := os.Stat(filepath.Join(p.config.AppPath, "requirements.txt")); err == nil { - reqArgs := []string{"uv", "pip", "install", "-r", filepath.Join(p.config.AppPath, "requirements.txt")} + // We also execute command with CWD set at p.config.AppPath + // so we can just path local path to requirements.txt here + reqArgs := []string{"uv", "pip", "install", "-r", "requirements.txt"} if err := p.runCommand(reqArgs); err != nil { return err } From 8c57032561b001a9c0e528b7c457c9471704dad7 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 6 Jun 2025 13:02:58 +0200 Subject: [PATCH 07/10] next changelog --- NEXT_CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 71fec803b4..db0efc7d8f 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -7,6 +7,7 @@ ### Dependency updates ### CLI +* Use OS aware runner instead of bash for run-local command ([#2996](https://github.com/databricks/cli/pull/2996)) ### Bundles * Fix "bundle summary -o json" to render null values properly ([#2990](https://github.com/databricks/cli/pull/2990)) From ac90de1208b235e077246ca2397044c929f421f8 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 6 Jun 2025 13:35:30 +0200 Subject: [PATCH 08/10] inherit output --- libs/apps/python.go | 30 ++---------------------------- libs/exec/exec.go | 27 +++++++++++++++++++-------- 2 files changed, 21 insertions(+), 36 deletions(-) diff --git a/libs/apps/python.go b/libs/apps/python.go index e64941ff90..41c069a81c 100644 --- a/libs/apps/python.go +++ b/libs/apps/python.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "io" "os" "path/filepath" "strconv" @@ -145,35 +144,10 @@ func (p *PythonApp) runCommand(args []string) error { if err != nil { return err } - + e.WithInheritOutput() cmd, err := e.StartCommand(p.ctx, strings.Join(args, " ")) if err != nil { return err } - - // Create error channels to capture io.Copy errors - stdoutErr := make(chan error, 1) - stderrErr := make(chan error, 1) - - go func() { - _, err := io.Copy(os.Stdout, cmd.Stdout()) - stdoutErr <- err - }() - go func() { - _, err := io.Copy(os.Stderr, cmd.Stderr()) - stderrErr <- err - }() - - // Wait for command completion - cmdErr := cmd.Wait() - - // Check for io.Copy errors - if err := <-stdoutErr; err != nil { - return fmt.Errorf("error copying stdout: %w", err) - } - if err := <-stderrErr; err != nil { - return fmt.Errorf("error copying stderr: %w", err) - } - - return cmdErr + return cmd.Wait() } diff --git a/libs/exec/exec.go b/libs/exec/exec.go index 466117e601..f3e777a423 100644 --- a/libs/exec/exec.go +++ b/libs/exec/exec.go @@ -61,21 +61,28 @@ func (c *command) Stderr() io.ReadCloser { } type Executor struct { - shell shell - dir string + shell shell + dir string + inheritOutput bool } +// NewCommandExecutor creates an Executor with default output behavior (no inheritance) func NewCommandExecutor(dir string) (*Executor, error) { shell, err := findShell() if err != nil { return nil, err } return &Executor{ - shell: shell, - dir: dir, + shell: shell, + dir: dir, + inheritOutput: false, }, nil } +func (e *Executor) WithInheritOutput() { + e.inheritOutput = true +} + func NewCommandExecutorWithExecutable(dir string, execType ExecutableType) (*Executor, error) { f, ok := finders[execType] if !ok { @@ -107,20 +114,24 @@ func (e *Executor) StartCommand(ctx context.Context, command string) (Command, e if err != nil { return nil, err } - return e.start(ctx, cmd, ec) + return e.start(cmd, ec) } -func (e *Executor) start(ctx context.Context, cmd *osexec.Cmd, ec *execContext) (Command, error) { +func (e *Executor) start(cmd *osexec.Cmd, ec *execContext) (Command, error) { + if e.inheritOutput { + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + return &command{cmd, ec, nil, nil}, cmd.Start() + } stdout, err := cmd.StdoutPipe() if err != nil { return nil, err } - stderr, err := cmd.StderrPipe() if err != nil { return nil, err } - return &command{cmd, ec, stdout, stderr}, cmd.Start() } From 2b9db15ef09b796f322d17179dcd847bc8cd6d2e Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 6 Jun 2025 13:39:19 +0200 Subject: [PATCH 09/10] comment --- libs/apps/python.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/apps/python.go b/libs/apps/python.go index 41c069a81c..9360505909 100644 --- a/libs/apps/python.go +++ b/libs/apps/python.go @@ -145,6 +145,9 @@ func (p *PythonApp) runCommand(args []string) error { return err } e.WithInheritOutput() + + // Safe to join args with spaces here since args are passed directly inside PrepareEnvironment() and GetCommand() + // and don't contain user input. cmd, err := e.StartCommand(p.ctx, strings.Join(args, " ")) if err != nil { return err From 05cbffb73189cc92ba081cd0214b29fd9d04dd10 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 6 Jun 2025 13:40:09 +0200 Subject: [PATCH 10/10] fix test --- libs/exec/exec_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/exec/exec_test.go b/libs/exec/exec_test.go index f245f9dd1e..49ab2eba66 100644 --- a/libs/exec/exec_test.go +++ b/libs/exec/exec_test.go @@ -123,7 +123,7 @@ func TestExecutorCleanupsTempFiles(t *testing.T) { cmd, ec, err := executor.prepareCommand(context.Background(), "echo 'Hello'") assert.NoError(t, err) - command, err := executor.start(context.Background(), cmd, ec) + command, err := executor.start(cmd, ec) assert.NoError(t, err) fileName := ec.args[1]