builder: conditional warning for wcow

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

cmd/docker/builder.go

@@ -34,43 +34,50 @@ func newBuilderError(warn bool, err error) error {
 	if pluginmanager.IsNotFound(err) {
 		return errors.New(errorMsg)
 	}
-	return fmt.Errorf("%w\n\n%s", err, errorMsg)
+	if err != nil {
+		return fmt.Errorf("%w\n\n%s", err, errorMsg)
+	}
+	return fmt.Errorf("%s", errorMsg)
 }
 
 func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []string) ([]string, []string, error) {
+	var useLegacy bool
+	var useBuilder bool
+
 	// check DOCKER_BUILDKIT env var is present and
-	// if not assume we want to use a builder
-	var enforcedBuilder bool
+	// if not assume we want to use the builder component
 	if v, ok := os.LookupEnv("DOCKER_BUILDKIT"); ok {
 		enabled, err := strconv.ParseBool(v)
 		if err != nil {
 			return args, osargs, errors.Wrap(err, "DOCKER_BUILDKIT environment variable expects boolean value")
 		}
 		if !enabled {
-			return args, osargs, nil
+			useLegacy = true
+		} else {
+			useBuilder = true
 		}
-		enforcedBuilder = true
 	}
 
 	// if a builder alias is defined, use it instead
 	// of the default one
-	isAlias := false
 	builderAlias := builderDefaultPlugin
 	aliasMap := dockerCli.ConfigFile().Aliases
 	if v, ok := aliasMap[keyBuilderAlias]; ok {
-		isAlias = true
+		useBuilder = true
 		builderAlias = v
 	}
 
-	// wcow build command must use the legacy builder for buildx
-	// if not opt-in through a builder alias
-	if !isAlias && dockerCli.ServerInfo().OSType == "windows" {
+	// is this a build that should be forwarded to the builder?
+	fwargs, fwosargs, forwarded := forwardBuilder(builderAlias, args, osargs)
+	if !forwarded {
 		return args, osargs, nil
 	}
 
-	// are we using a cmd that should be forwarded to the builder?
-	fwargs, fwosargs, forwarded := forwardBuilder(builderAlias, args, osargs)
-	if !forwarded {
+	if useLegacy {
+		// display warning if not wcow and continue
+		if dockerCli.ServerInfo().OSType != "windows" {
+			_, _ = fmt.Fprintln(dockerCli.Err(), newBuilderError(true, nil))
+		}
 		return args, osargs, nil
 	}
 
@@ -80,9 +87,9 @@ func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []st
 		perr = plugin.Err
 	}
 	if perr != nil {
-		// if builder enforced with DOCKER_BUILDKIT=1, cmd fails if plugin missing or broken
-		if enforcedBuilder {
-			return fwargs, fwosargs, newBuilderError(false, perr)
+		// if builder enforced with DOCKER_BUILDKIT=1, cmd must fail if plugin missing or broken
+		if useBuilder {
+			return args, osargs, newBuilderError(false, perr)
 		}
 		// otherwise, display warning and continue
 		_, _ = fmt.Fprintln(dockerCli.Err(), newBuilderError(true, perr))

cmd/docker/builder_test.go

@@ -3,17 +3,27 @@ package main
 import (
 	"bytes"
 	"os"
+	"runtime"
 	"testing"
 
 	"github.com/docker/cli/cli/command"
+	"github.com/docker/cli/internal/test/output"
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/env"
 	"gotest.tools/v3/fs"
 )
 
-func TestBuild(t *testing.T) {
+var pluginFilename = "docker-buildx"
+
+func init() {
+	if runtime.GOOS == "windows" {
+		pluginFilename = pluginFilename + ".exe"
+	}
+}
+
+func TestBuildWithBuilder(t *testing.T) {
 	dir := fs.NewDir(t, t.Name(),
-		fs.WithFile("docker-buildx", `#!/bin/sh
+		fs.WithFile(pluginFilename, `#!/bin/sh
 echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortDescription":"Build with BuildKit"}'`, fs.WithMode(0777)),
 	)
 	defer dir.Remove()
@@ -36,10 +46,44 @@ echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortD
 
 func TestBuildkitDisabled(t *testing.T) {
 	defer env.Patch(t, "DOCKER_BUILDKIT", "0")()
-	var b bytes.Buffer
 
-	dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(&b))
+	dir := fs.NewDir(t, t.Name(),
+		fs.WithFile(pluginFilename, `#!/bin/sh exit 1`, fs.WithMode(0777)),
+	)
+	defer dir.Remove()
+
+	b := bytes.NewBuffer(nil)
+
+	dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b))
+	assert.NilError(t, err)
+	dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()}
+
+	tcmd := newDockerCommand(dockerCli)
+	tcmd.SetArgs([]string{"build", "."})
+
+	cmd, args, err := tcmd.HandleGlobalFlags()
+	assert.NilError(t, err)
+
+	args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args)
+	assert.NilError(t, err)
+	assert.DeepEqual(t, []string{"build", "."}, args)
+
+	output.Assert(t, b.String(), map[int]func(string) error{
+		0: output.Suffix("DEPRECATED: The legacy builder is deprecated and will be removed in a future release."),
+	})
+}
+
+func TestBuilderBroken(t *testing.T) {
+	dir := fs.NewDir(t, t.Name(),
+		fs.WithFile(pluginFilename, `#!/bin/sh exit 1`, fs.WithMode(0777)),
+	)
+	defer dir.Remove()
+
+	b := bytes.NewBuffer(nil)
+
+	dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b))
 	assert.NilError(t, err)
+	dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()}
 
 	tcmd := newDockerCommand(dockerCli)
 	tcmd.SetArgs([]string{"build", "."})
@@ -50,4 +94,38 @@ func TestBuildkitDisabled(t *testing.T) {
 	args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args)
 	assert.NilError(t, err)
 	assert.DeepEqual(t, []string{"build", "."}, args)
+
+	output.Assert(t, b.String(), map[int]func(string) error{
+		0: output.Prefix("failed to fetch metadata:"),
+		2: output.Suffix("DEPRECATED: The legacy builder is deprecated and will be removed in a future release."),
+	})
+}
+
+func TestBuilderBrokenEnforced(t *testing.T) {
+	defer env.Patch(t, "DOCKER_BUILDKIT", "1")()
+
+	dir := fs.NewDir(t, t.Name(),
+		fs.WithFile(pluginFilename, `#!/bin/sh exit 1`, fs.WithMode(0777)),
+	)
+	defer dir.Remove()
+
+	b := bytes.NewBuffer(nil)
+
+	dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b))
+	assert.NilError(t, err)
+	dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()}
+
+	tcmd := newDockerCommand(dockerCli)
+	tcmd.SetArgs([]string{"build", "."})
+
+	cmd, args, err := tcmd.HandleGlobalFlags()
+	assert.NilError(t, err)
+
+	args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args)
+	assert.DeepEqual(t, []string{"build", "."}, args)
+
+	output.Assert(t, err.Error(), map[int]func(string) error{
+		0: output.Prefix("failed to fetch metadata:"),
+		2: output.Suffix("ERROR: BuildKit is enabled but the buildx component is missing or broken."),
+	})
 }