Fix build output (#2312)

* Fix build output

Multiplexing stderr to a buffer accidentally broke TTY detection in docker. This PR restores it by using a pipe instead of a wrapper.

Also fixed `--progress` not being passed to the second build step.

* fix potential stderr/stdout deadlock

* only use pty if error writer is a tty

* fix deprecation warning

* container stop prints the id, discard it

* out goes to stderr unless a writer is provided

Author: michaeldwan

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/aws/aws-sdk-go-v2 v1.36.3
 	github.com/aws/aws-sdk-go-v2/credentials v1.17.67
 	github.com/aws/aws-sdk-go-v2/service/s3 v1.79.3
+	github.com/creack/pty v1.1.24
 	github.com/docker/cli v28.1.1+incompatible
 	github.com/docker/docker v28.1.1+incompatible
 	github.com/docker/go-connections v0.5.0
@@ -33,6 +34,7 @@ require (
 	golang.org/x/sys v0.32.0
 	golang.org/x/term v0.31.0
 	gopkg.in/yaml.v2 v2.4.0
+	gopkg.in/yaml.v3 v3.0.1
 	sigs.k8s.io/yaml v1.4.0
 )
 
@@ -279,7 +281,6 @@ require (
 	google.golang.org/genproto/googleapis/api v0.0.0-20250218202821-56aae31c358a // indirect
 	google.golang.org/grpc v1.71.0 // indirect
 	google.golang.org/protobuf v1.36.6 // indirect
-	gopkg.in/yaml.v3 v3.0.1 // indirect
 	gotest.tools/gotestsum v1.12.2 // indirect
 	honnef.co/go/tools v0.6.1 // indirect
 	mvdan.cc/gofumpt v0.7.0 // indirect

go.sum

@@ -121,8 +121,8 @@ github.com/containerd/stargz-snapshotter/estargz v0.16.3/go.mod h1:uyr4BfYfOj3G9
 github.com/cpuguy83/dockercfg v0.3.2 h1:DlJTyZGBDlXqUZ2Dk2Q3xHs/FtnooJJVaad2S9GKorA=
 github.com/cpuguy83/dockercfg v0.3.2/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHfjj5/jFyUJc=
 github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
-github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY=
-github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4=
+github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s=
+github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE=
 github.com/curioswitch/go-reassign v0.3.0 h1:dh3kpQHuADL3cobV/sSGETA8DOv457dwl+fbBAhrQPs=
 github.com/curioswitch/go-reassign v0.3.0/go.mod h1:nApPCCTtqLJN/s8HfItCcKV0jIPwluBOvZP+dsJGA88=
 github.com/daixiang0/gci v0.13.5 h1:kThgmH1yBmZSBCh1EJVxQ7JsHpm5Oms0AMed/0LaH4c=

pkg/docker/docker_command.go

@@ -1,6 +1,7 @@
 package docker
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -12,6 +13,7 @@ import (
 	"runtime"
 	"strings"
 
+	"github.com/creack/pty"
 	"github.com/docker/cli/cli/config"
 	"github.com/docker/cli/cli/config/configfile"
 	"github.com/docker/cli/cli/config/types"
@@ -53,7 +55,7 @@ func (c *DockerCommand) Pull(ctx context.Context, image string, force bool) (*im
 		"--platform", "linux/amd64",
 	}
 
-	err := c.exec(ctx, nil, nil, "", args)
+	err := c.exec(ctx, nil, nil, nil, "", args)
 	if err != nil {
 		// A "not found" error message will be different depending on what flavor of engine and
 		// registry version we're hitting. This checks for both docker and OCI lingo.
@@ -74,7 +76,7 @@ func (c *DockerCommand) Pull(ctx context.Context, image string, force bool) (*im
 func (c *DockerCommand) Push(ctx context.Context, image string) error {
 	console.Debugf("=== DockerCommand.Push %s", image)
 
-	return c.exec(ctx, nil, nil, "", []string{"push", image})
+	return c.exec(ctx, nil, nil, nil, "", []string{"push", image})
 }
 
 func (c *DockerCommand) LoadUserInformation(ctx context.Context, registryHost string) (*command.UserInfo, error) {
@@ -118,7 +120,7 @@ func (c *DockerCommand) CreateTarFile(ctx context.Context, image string, tmpDir
 		"/",
 		folder,
 	}
-	if err := c.exec(ctx, nil, nil, "", args); err != nil {
+	if err := c.exec(ctx, nil, nil, nil, "", args); err != nil {
 		return "", err
 	}
 	return filepath.Join(tmpDir, tarFile), nil
@@ -143,7 +145,7 @@ func (c *DockerCommand) CreateAptTarFile(ctx context.Context, tmpDir string, apt
 		"/buildtmp/" + aptTarFile,
 	}
 	args = append(args, packages...)
-	if err := c.exec(ctx, nil, nil, "", args); err != nil {
+	if err := c.exec(ctx, nil, nil, nil, "", args); err != nil {
 		return "", err
 	}
 
@@ -204,7 +206,7 @@ func (c *DockerCommand) ContainerLogs(ctx context.Context, containerID string, w
 		"--follow",
 	}
 
-	return c.exec(ctx, nil, w, "", args)
+	return c.exec(ctx, nil, w, nil, "", args)
 }
 
 func (c *DockerCommand) ContainerInspect(ctx context.Context, id string) (*container.InspectResponse, error) {
@@ -241,11 +243,11 @@ func (c *DockerCommand) ContainerStop(ctx context.Context, containerID string) e
 	args := []string{
 		"container",
 		"stop",
-		"--time", "3",
+		"--timeout", "3",
 		containerID,
 	}
 
-	if err := c.exec(ctx, nil, nil, "", args); err != nil {
+	if err := c.exec(ctx, nil, nil, nil, "", args); err != nil {
 		if strings.Contains(err.Error(), "No such container") {
 			err = &command.NotFoundError{Object: "container", Ref: containerID}
 		}
@@ -330,44 +332,82 @@ func (c *DockerCommand) ImageBuild(ctx context.Context, options command.ImageBui
 
 	in := strings.NewReader(options.DockerfileContents)
 
-	return c.exec(ctx, in, nil, options.WorkingDir, args)
+	return c.exec(ctx, in, nil, nil, options.WorkingDir, args)
 }
 
-func (c *DockerCommand) exec(ctx context.Context, in io.Reader, out io.Writer, dir string, args []string) error {
+func (c *DockerCommand) exec(ctx context.Context, in io.Reader, outw, errw io.Writer, dir string, args []string) error {
+	if outw == nil {
+		outw = os.Stderr
+	}
+	if errw == nil {
+		errw = os.Stderr
+	}
+
 	dockerCmd := DockerCommandFromEnvironment()
 	cmd := exec.CommandContext(ctx, dockerCmd, args...)
+	if dir != "" {
+		cmd.Dir = dir
+	}
+
+	// setup stderr buffer & writer to errw and buffer
+	var stderrBuf bytes.Buffer
+
+	// if errw is a TTY, use a pty for stderr output so that the child process will properly detect an interactive console
+	if f, ok := errw.(*os.File); ok && console.IsTTY(f) {
+		stderrpty, stderrtty, err := pty.Open()
+		if err != nil {
+			return fmt.Errorf("failed to open stderr pty: %w", err)
+		}
+		cmd.Stderr = stderrtty
+
+		go func() {
+			defer stderrpty.Close()
+			defer stderrtty.Close()
+
+			_, err = io.Copy(io.MultiWriter(
+				errw,
+				util.NewRingBufferWriter(&stderrBuf, 1024),
+			), stderrpty)
+			if err != nil {
+				console.Errorf("failed to copy stderr pty to errw: %s", err)
+			}
+		}()
+	} else {
+		cmd.Stderr = io.MultiWriter(errw, util.NewRingBufferWriter(&stderrBuf, 1024))
+	}
 
-	if out == nil {
-		out = os.Stderr
+	// setup stdout pipe
+	outpipe, err := cmd.StdoutPipe()
+	if err != nil {
+		return fmt.Errorf("failed to create stdout pipe: %w", err)
 	}
+	// copy stdout to outw
+	go func() {
+		defer outpipe.Close()
 
-	// the ring buffer captures the last N bytes written to `w` so we have some context to return in an error
-	errbuf := util.NewRingBufferWriter(out, 1024)
-	cmd.Stdout = errbuf
-	cmd.Stderr = errbuf
+		_, err = io.Copy(outw, outpipe)
+		if err != nil {
+			console.Errorf("failed to copy stdout to outw: %s", err)
+		}
+	}()
 
 	if in != nil {
 		cmd.Stdin = in
 	}
 
-	if dir != "" {
-		cmd.Dir = dir
-	}
-
 	console.Debug("$ " + strings.Join(cmd.Args, " "))
-	err := cmd.Run()
-	if err != nil {
+	if err := cmd.Run(); err != nil {
 		if errors.Is(err, context.Canceled) {
 			return err
 		}
-		return fmt.Errorf("command failed: %s: %w", errbuf.String(), err)
+		return fmt.Errorf("command failed: %s: %w", stderrBuf.String(), err)
 	}
 	return nil
 }
 
 func (c *DockerCommand) execCaptured(ctx context.Context, in io.Reader, dir string, args []string) (string, error) {
 	var out strings.Builder
-	err := c.exec(ctx, in, &out, dir, args)
+	err := c.exec(ctx, in, &out, nil, dir, args)
 	if err != nil {
 		return "", err
 	}

pkg/image/build.go

@@ -271,7 +271,7 @@ func Build(ctx context.Context, cfg *config.Config, dir, imageName string, secre
 		labels[key] = val
 	}
 
-	if err := BuildAddLabelsAndSchemaToImage(ctx, dockerCommand, imageName, labels, bundledSchemaFile); err != nil {
+	if err := BuildAddLabelsAndSchemaToImage(ctx, dockerCommand, imageName, labels, bundledSchemaFile, progressOutput); err != nil {
 		return fmt.Errorf("Failed to add labels to image: %w", err)
 	}
 	return nil
@@ -280,14 +280,15 @@ func Build(ctx context.Context, cfg *config.Config, dir, imageName string, secre
 // BuildAddLabelsAndSchemaToImage builds a cog model with labels and schema.
 //
 // The new image is based on the provided image with the labels and schema file appended to it.
-func BuildAddLabelsAndSchemaToImage(ctx context.Context, dockerClient command.Command, image string, labels map[string]string, bundledSchemaFile string) error {
+func BuildAddLabelsAndSchemaToImage(ctx context.Context, dockerClient command.Command, image string, labels map[string]string, bundledSchemaFile string, progressOutput string) error {
 	dockerfile := "FROM " + image + "\n"
 	dockerfile += "COPY " + bundledSchemaFile + " .cog\n"
 
 	buildOpts := command.ImageBuildOptions{
 		DockerfileContents: dockerfile,
 		ImageName:          image,
 		Labels:             labels,
+		ProgressOutput:     progressOutput,
 	}
 
 	if err := dockerClient.ImageBuild(ctx, buildOpts); err != nil {