From 6fb208126e8223a2bd0e574143c514fac9f28ffb Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@docker.com>
Date: Tue, 4 Apr 2017 12:28:59 -0400
Subject: [PATCH 1/4] Cleanup in dispatcher.env

Remove commented code blocks
Remove some duplication in comparing and restructuring env

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Upstream-commit: c7fad9b750f8f143a22cc5a85a1dc26573025414
Component: engine
---
 .../engine/builder/dockerfile/builder.go      |  9 ++--
 .../engine/builder/dockerfile/dispatchers.go  | 42 +++++--------------
 .../builder/dockerfile/dispatchers_test.go    | 37 +++++++++-------
 .../builder/dockerfile/dispatchers_unix.go    |  6 +++
 .../builder/dockerfile/dispatchers_windows.go |  6 +++
 .../engine/builder/dockerfile/evaluator.go    | 12 ++++--
 .../engine/builder/dockerfile/internals.go    |  7 +---
 .../engine/builder/dockerfile/shell_parser.go | 15 +------
 .../integration-cli/docker_cli_build_test.go  | 20 ++++-----
 9 files changed, 69 insertions(+), 85 deletions(-)

diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go
index b62e7e220d..03abfafcf3 100644
--- a/components/engine/builder/dockerfile/builder.go
+++ b/components/engine/builder/dockerfile/builder.go
@@ -2,7 +2,6 @@ package dockerfile
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -20,7 +19,7 @@ import (
 	"github.com/docker/docker/builder/dockerfile/parser"
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/stringid"
-	perrors "github.com/pkg/errors"
+	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 )
 
@@ -220,7 +219,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 	total := len(dockerfile.Children)
 	for _, n := range dockerfile.Children {
 		if err := b.checkDispatch(n, false); err != nil {
-			return "", perrors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
+			return "", errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
 		}
 	}
 
@@ -253,7 +252,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 	}
 
 	if b.options.Target != "" && !b.imageContexts.isCurrentTarget(b.options.Target) {
-		return "", perrors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target)
+		return "", errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target)
 	}
 
 	b.warnOnUnusedBuildArgs()
@@ -269,7 +268,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 		}
 		b.image, err = b.docker.SquashImage(b.image, fromID)
 		if err != nil {
-			return "", perrors.Wrap(err, "error squashing image")
+			return "", errors.Wrap(err, "error squashing image")
 		}
 	}
 
diff --git a/components/engine/builder/dockerfile/dispatchers.go b/components/engine/builder/dockerfile/dispatchers.go
index fb064e5971..18fcdaf55d 100644
--- a/components/engine/builder/dockerfile/dispatchers.go
+++ b/components/engine/builder/dockerfile/dispatchers.go
@@ -16,6 +16,7 @@ import (
 	"strings"
 	"time"
 
+	"bytes"
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/api"
 	"github.com/docker/docker/api/types"
@@ -46,45 +47,22 @@ func env(b *Builder, args []string, attributes map[string]bool, original string)
 		return err
 	}
 
-	// TODO/FIXME/NOT USED
-	// Just here to show how to use the builder flags stuff within the
-	// context of a builder command. Will remove once we actually add
-	// a builder command to something!
-	/*
-		flBool1 := b.flags.AddBool("bool1", false)
-		flStr1 := b.flags.AddString("str1", "HI")
-
-		if err := b.flags.Parse(); err != nil {
-			return err
-		}
-
-		fmt.Printf("Bool1:%v\n", flBool1)
-		fmt.Printf("Str1:%v\n", flStr1)
-	*/
-
-	commitStr := "ENV"
-
-	for j := 0; j < len(args); j++ {
-		// name  ==> args[j]
-		// value ==> args[j+1]
+	commitMessage := bytes.NewBufferString("ENV")
 
+	for j := 0; j < len(args); j += 2 {
 		if len(args[j]) == 0 {
 			return errBlankCommandNames("ENV")
 		}
-		newVar := args[j] + "=" + args[j+1] + ""
-		commitStr += " " + newVar
+		name := args[j]
+		value := args[j+1]
+		newVar := name + "=" + value
+		commitMessage.WriteString(" " + newVar)
 
 		gotOne := false
 		for i, envVar := range b.runConfig.Env {
 			envParts := strings.SplitN(envVar, "=", 2)
 			compareFrom := envParts[0]
-			compareTo := args[j]
-			if runtime.GOOS == "windows" {
-				// Case insensitive environment variables on Windows
-				compareFrom = strings.ToUpper(compareFrom)
-				compareTo = strings.ToUpper(compareTo)
-			}
-			if compareFrom == compareTo {
+			if equalEnvKeys(compareFrom, name) {
 				b.runConfig.Env[i] = newVar
 				gotOne = true
 				break
@@ -93,10 +71,9 @@ func env(b *Builder, args []string, attributes map[string]bool, original string)
 		if !gotOne {
 			b.runConfig.Env = append(b.runConfig.Env, newVar)
 		}
-		j++
 	}
 
-	return b.commit("", b.runConfig.Cmd, commitStr)
+	return b.commit("", b.runConfig.Cmd, commitMessage.String())
 }
 
 // MAINTAINER some text <maybe@an.email.address>
@@ -440,6 +417,7 @@ func run(b *Builder, args []string, attributes map[string]bool, original string)
 		return err
 	}
 
+	// FIXME: this is duplicated with the defer above in this function (i think?)
 	// revert to original config environment and set the command string to
 	// have the build-time env vars in it (if any) so that future cache look-ups
 	// properly match it.
diff --git a/components/engine/builder/dockerfile/dispatchers_test.go b/components/engine/builder/dockerfile/dispatchers_test.go
index 77506d712a..40178b1cc7 100644
--- a/components/engine/builder/dockerfile/dispatchers_test.go
+++ b/components/engine/builder/dockerfile/dispatchers_test.go
@@ -133,27 +133,34 @@ func TestCommandseBlankNames(t *testing.T) {
 }
 
 func TestEnv2Variables(t *testing.T) {
-	variables := []string{"var1", "val1", "var2", "val2"}
+	b := newBuilderWithMockBackend()
+	b.disableCommit = true
 
-	bflags := &BFlags{}
-	config := &container.Config{}
+	args := []string{"var1", "val1", "var2", "val2"}
+	err := env(b, args, nil, "")
+	assert.NilError(t, err)
 
-	b := &Builder{flags: bflags, runConfig: config, disableCommit: true}
-
-	if err := env(b, variables, nil, ""); err != nil {
-		t.Fatalf("Error when executing env: %s", err.Error())
+	expected := []string{
+		fmt.Sprintf("%s=%s", args[0], args[1]),
+		fmt.Sprintf("%s=%s", args[2], args[3]),
 	}
+	assert.DeepEqual(t, b.runConfig.Env, expected)
+}
 
-	expectedVar1 := fmt.Sprintf("%s=%s", variables[0], variables[1])
-	expectedVar2 := fmt.Sprintf("%s=%s", variables[2], variables[3])
+func TestEnvValueWithExistingRunConfigEnv(t *testing.T) {
+	b := newBuilderWithMockBackend()
+	b.disableCommit = true
+	b.runConfig.Env = []string{"var1=old", "var2=fromenv"}
 
-	if b.runConfig.Env[0] != expectedVar1 {
-		t.Fatalf("Wrong env output for first variable. Got: %s. Should be: %s", b.runConfig.Env[0], expectedVar1)
-	}
-
-	if b.runConfig.Env[1] != expectedVar2 {
-		t.Fatalf("Wrong env output for second variable. Got: %s, Should be: %s", b.runConfig.Env[1], expectedVar2)
+	args := []string{"var1", "val1"}
+	err := env(b, args, nil, "")
+	assert.NilError(t, err)
+
+	expected := []string{
+		fmt.Sprintf("%s=%s", args[0], args[1]),
+		"var2=fromenv",
 	}
+	assert.DeepEqual(t, b.runConfig.Env, expected)
 }
 
 func TestMaintainer(t *testing.T) {
diff --git a/components/engine/builder/dockerfile/dispatchers_unix.go b/components/engine/builder/dockerfile/dispatchers_unix.go
index 29eb2fb008..62ee371dfe 100644
--- a/components/engine/builder/dockerfile/dispatchers_unix.go
+++ b/components/engine/builder/dockerfile/dispatchers_unix.go
@@ -26,3 +26,9 @@ func normaliseWorkdir(current string, requested string) (string, error) {
 func errNotJSON(command, _ string) error {
 	return fmt.Errorf("%s requires the arguments to be in JSON form", command)
 }
+
+// equalEnvKeys compare two strings and returns true if they are equal. On
+// Windows this comparison is case insensitive.
+func equalEnvKeys(from, to string) bool {
+	return from == to
+}
diff --git a/components/engine/builder/dockerfile/dispatchers_windows.go b/components/engine/builder/dockerfile/dispatchers_windows.go
index 471232f3c7..71f7c9288f 100644
--- a/components/engine/builder/dockerfile/dispatchers_windows.go
+++ b/components/engine/builder/dockerfile/dispatchers_windows.go
@@ -85,3 +85,9 @@ func errNotJSON(command, original string) error {
 	}
 	return fmt.Errorf("%s requires the arguments to be in JSON form%s", command, extra)
 }
+
+// equalEnvKeys compare two strings and returns true if they are equal. On
+// Windows this comparison is case insensitive.
+func equalEnvKeys(from, to string) bool {
+	return strings.ToUpper(from) == strings.ToUpper(to)
+}
diff --git a/components/engine/builder/dockerfile/evaluator.go b/components/engine/builder/dockerfile/evaluator.go
index d29a18c7b1..5be23c8cda 100644
--- a/components/engine/builder/dockerfile/evaluator.go
+++ b/components/engine/builder/dockerfile/evaluator.go
@@ -20,13 +20,13 @@
 package dockerfile
 
 import (
-	"errors"
 	"fmt"
 	"strings"
 
 	"github.com/docker/docker/builder/dockerfile/command"
 	"github.com/docker/docker/builder/dockerfile/parser"
-	runconfigopts "github.com/docker/docker/runconfig/opts"
+	"github.com/docker/docker/runconfig/opts"
+	"github.com/pkg/errors"
 )
 
 // Environment variable interpolation will happen on these statements only.
@@ -187,7 +187,7 @@ func (b *Builder) evaluateEnv(cmd string, str string, envs []string) ([]string,
 // args that are not overriden by runConfig environment variables.
 func (b *Builder) buildArgsWithoutConfigEnv() []string {
 	envs := []string{}
-	configEnv := runconfigopts.ConvertKVStringsToMap(b.runConfig.Env)
+	configEnv := b.runConfigEnvMapping()
 
 	for key, val := range b.buildArgs.GetAllAllowed() {
 		if _, ok := configEnv[key]; !ok {
@@ -197,6 +197,10 @@ func (b *Builder) buildArgsWithoutConfigEnv() []string {
 	return envs
 }
 
+func (b *Builder) runConfigEnvMapping() map[string]string {
+	return opts.ConvertKVStringsToMap(b.runConfig.Env)
+}
+
 // checkDispatch does a simple check for syntax errors of the Dockerfile.
 // Because some of the instructions can only be validated through runtime,
 // arg, env, etc., this syntax check will not be complete and could not replace
@@ -235,5 +239,5 @@ func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error {
 		return nil
 	}
 
-	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)
+	return errors.Errorf("unknown instruction: %s", upperCasedCmd)
 }
diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go
index eecd47fdea..122bb2019e 100644
--- a/components/engine/builder/dockerfile/internals.go
+++ b/components/engine/builder/dockerfile/internals.go
@@ -35,7 +35,6 @@ import (
 	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/pkg/tarsum"
 	"github.com/docker/docker/pkg/urlutil"
-	"github.com/docker/docker/runconfig/opts"
 	"github.com/pkg/errors"
 )
 
@@ -433,11 +432,7 @@ func (b *Builder) processImageFrom(img builder.Image) error {
 	// Check to see if we have a default PATH, note that windows won't
 	// have one as it's set by HCS
 	if system.DefaultPathEnv != "" {
-		// Convert the slice of strings that represent the current list
-		// of env vars into a map so we can see if PATH is already set.
-		// If it's not set then go ahead and give it our default value
-		configEnv := opts.ConvertKVStringsToMap(b.runConfig.Env)
-		if _, ok := configEnv["PATH"]; !ok {
+		if _, ok := b.runConfigEnvMapping()["PATH"]; !ok {
 			b.runConfig.Env = append(b.runConfig.Env,
 				"PATH="+system.DefaultPathEnv)
 		}
diff --git a/components/engine/builder/dockerfile/shell_parser.go b/components/engine/builder/dockerfile/shell_parser.go
index 8e210ad6a9..7c3cf66e83 100644
--- a/components/engine/builder/dockerfile/shell_parser.go
+++ b/components/engine/builder/dockerfile/shell_parser.go
@@ -8,7 +8,6 @@ package dockerfile
 
 import (
 	"fmt"
-	"runtime"
 	"strings"
 	"text/scanner"
 	"unicode"
@@ -296,17 +295,10 @@ func (sw *shellWord) processName() string {
 }
 
 func (sw *shellWord) getEnv(name string) string {
-	if runtime.GOOS == "windows" {
-		// Case-insensitive environment variables on Windows
-		name = strings.ToUpper(name)
-	}
 	for _, env := range sw.envs {
 		i := strings.Index(env, "=")
 		if i < 0 {
-			if runtime.GOOS == "windows" {
-				env = strings.ToUpper(env)
-			}
-			if name == env {
+			if equalEnvKeys(name, env) {
 				// Should probably never get here, but just in case treat
 				// it like "var" and "var=" are the same
 				return ""
@@ -314,10 +306,7 @@ func (sw *shellWord) getEnv(name string) string {
 			continue
 		}
 		compareName := env[:i]
-		if runtime.GOOS == "windows" {
-			compareName = strings.ToUpper(compareName)
-		}
-		if name != compareName {
+		if !equalEnvKeys(name, compareName) {
 			continue
 		}
 		return env[i+1:]
diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go
index ea25959e44..df48bede77 100644
--- a/components/engine/integration-cli/docker_cli_build_test.go
+++ b/components/engine/integration-cli/docker_cli_build_test.go
@@ -6156,7 +6156,7 @@ func (s *DockerSuite) TestBuildCopyFromPreviousFromWindows(c *check.C) {
 func (s *DockerSuite) TestBuildCopyFromForbidWindowsSystemPaths(c *check.C) {
 	testRequires(c, DaemonIsWindows)
 	dockerfile := `
-		FROM ` + testEnv.MinimalBaseImage() + `		
+		FROM ` + testEnv.MinimalBaseImage() + `
 		FROM ` + testEnv.MinimalBaseImage() + `
 		COPY --from=0 %s c:\\oscopy
 		`
@@ -6173,7 +6173,7 @@ func (s *DockerSuite) TestBuildCopyFromForbidWindowsSystemPaths(c *check.C) {
 func (s *DockerSuite) TestBuildCopyFromForbidWindowsRelativePaths(c *check.C) {
 	testRequires(c, DaemonIsWindows)
 	dockerfile := `
-		FROM ` + testEnv.MinimalBaseImage() + `		
+		FROM ` + testEnv.MinimalBaseImage() + `
 		FROM ` + testEnv.MinimalBaseImage() + `
 		COPY --from=0 %s c:\\oscopy
 		`
@@ -6192,7 +6192,7 @@ func (s *DockerSuite) TestBuildCopyFromWindowsIsCaseInsensitive(c *check.C) {
 	testRequires(c, DaemonIsWindows)
 	dockerfile := `
 		FROM ` + testEnv.MinimalBaseImage() + `
-		COPY foo /	
+		COPY foo /
 		FROM ` + testEnv.MinimalBaseImage() + `
 		COPY --from=0 c:\\fOo c:\\copied
 		RUN type c:\\copied
@@ -6351,23 +6351,23 @@ func (s *DockerSuite) TestBuildLineErrorOnBuild(c *check.C) {
 }
 
 // FIXME(vdemeester) should be a unit test
-func (s *DockerSuite) TestBuildLineErrorUknownInstruction(c *check.C) {
+func (s *DockerSuite) TestBuildLineErrorUnknownInstruction(c *check.C) {
 	name := "test_build_line_error_unknown_instruction"
-	buildImage(name, build.WithDockerfile(`FROM busybox
+	cli.Docker(cli.Build(name), build.WithDockerfile(`FROM busybox
   RUN echo hello world
   NOINSTRUCTION echo ba
   RUN echo hello
   ERROR
   `)).Assert(c, icmd.Expected{
 		ExitCode: 1,
-		Err:      "Dockerfile parse error line 3: Unknown instruction: NOINSTRUCTION",
+		Err:      "Dockerfile parse error line 3: unknown instruction: NOINSTRUCTION",
 	})
 }
 
 // FIXME(vdemeester) should be a unit test
 func (s *DockerSuite) TestBuildLineErrorWithEmptyLines(c *check.C) {
 	name := "test_build_line_error_with_empty_lines"
-	buildImage(name, build.WithDockerfile(`
+	cli.Docker(cli.Build(name), build.WithDockerfile(`
   FROM busybox
 
   RUN echo hello world
@@ -6377,21 +6377,21 @@ func (s *DockerSuite) TestBuildLineErrorWithEmptyLines(c *check.C) {
   CMD ["/bin/init"]
   `)).Assert(c, icmd.Expected{
 		ExitCode: 1,
-		Err:      "Dockerfile parse error line 6: Unknown instruction: NOINSTRUCTION",
+		Err:      "Dockerfile parse error line 6: unknown instruction: NOINSTRUCTION",
 	})
 }
 
 // FIXME(vdemeester) should be a unit test
 func (s *DockerSuite) TestBuildLineErrorWithComments(c *check.C) {
 	name := "test_build_line_error_with_comments"
-	buildImage(name, build.WithDockerfile(`FROM busybox
+	cli.Docker(cli.Build(name), build.WithDockerfile(`FROM busybox
   # This will print hello world
   # and then ba
   RUN echo hello world
   NOINSTRUCTION echo ba
   `)).Assert(c, icmd.Expected{
 		ExitCode: 1,
-		Err:      "Dockerfile parse error line 5: Unknown instruction: NOINSTRUCTION",
+		Err:      "Dockerfile parse error line 5: unknown instruction: NOINSTRUCTION",
 	})
 }
 

From a5b8a0845ef1519f77f7de28c827d3d2447eb10b Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@docker.com>
Date: Tue, 4 Apr 2017 16:34:19 -0400
Subject: [PATCH 2/4] Factor out functions from
 builder/dockerfile/builder.go:Builder.build()

Remove the block comment which is stale, and redundant now that the
function is just as readable as the comment.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Upstream-commit: bfcd95817afaedb078022fc2f335ead64afee55c
Component: engine
---
 .../engine/builder/dockerfile/builder.go      | 126 ++++++++++--------
 .../engine/builder/dockerfile/evaluator.go    |  13 +-
 .../engine/builder/dockerfile/internals.go    |  23 ++--
 .../builder/dockerfile/internals_test.go      |   2 +-
 4 files changed, 90 insertions(+), 74 deletions(-)

diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go
index 03abfafcf3..d655fff7fb 100644
--- a/components/engine/builder/dockerfile/builder.go
+++ b/components/engine/builder/dockerfile/builder.go
@@ -185,17 +185,6 @@ func sanitizeRepoAndTags(names []string) ([]reference.Named, error) {
 
 // build runs the Dockerfile builder from a context and a docker object that allows to make calls
 // to Docker.
-//
-// This will (barring errors):
-//
-// * read the dockerfile from context
-// * parse the dockerfile if not already parsed
-// * walk the AST and execute it by dispatching to handlers. If Remove
-//   or ForceRemove is set, additional cleanup around containers happens after
-//   processing.
-// * Tag image, if applicable.
-// * Print a happy message and return the image ID.
-//
 func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (string, error) {
 	defer b.imageContexts.unmount()
 
@@ -203,7 +192,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 	b.Stderr = stderr
 	b.Output = out
 
-	dockerfile, err := b.readDockerfile()
+	dockerfile, err := b.readAndParseDockerfile()
 	if err != nil {
 		return "", err
 	}
@@ -215,14 +204,37 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 
 	addNodesForLabelOption(dockerfile, b.options.Labels)
 
-	var shortImgID string
-	total := len(dockerfile.Children)
-	for _, n := range dockerfile.Children {
-		if err := b.checkDispatch(n, false); err != nil {
-			return "", errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
+	if err := checkDispatchDockerfile(dockerfile); err != nil {
+		return "", err
+	}
+
+	shortImageID, err := b.dispatchDockerfileWithCancellation(dockerfile)
+	if err != nil {
+		return "", err
+	}
+
+	b.warnOnUnusedBuildArgs()
+
+	if b.image == "" {
+		return "", errors.New("No image was generated. Is your Dockerfile empty?")
+	}
+
+	if b.options.Squash {
+		if err := b.squashBuild(); err != nil {
+			return "", err
 		}
 	}
 
+	fmt.Fprintf(b.Stdout, "Successfully built %s\n", shortImageID)
+	if err := b.tagImages(repoAndTags); err != nil {
+		return "", err
+	}
+	return b.image, nil
+}
+
+func (b *Builder) dispatchDockerfileWithCancellation(dockerfile *parser.Node) (string, error) {
+	total := len(dockerfile.Children)
+	var shortImgID string
 	for i, n := range dockerfile.Children {
 		select {
 		case <-b.clientCtx.Done():
@@ -255,34 +267,20 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 		return "", errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target)
 	}
 
-	b.warnOnUnusedBuildArgs()
+	return shortImgID, nil
+}
 
-	if b.image == "" {
-		return "", errors.New("No image was generated. Is your Dockerfile empty?")
+func (b *Builder) squashBuild() error {
+	var fromID string
+	var err error
+	if b.from != nil {
+		fromID = b.from.ImageID()
 	}
-
-	if b.options.Squash {
-		var fromID string
-		if b.from != nil {
-			fromID = b.from.ImageID()
-		}
-		b.image, err = b.docker.SquashImage(b.image, fromID)
-		if err != nil {
-			return "", errors.Wrap(err, "error squashing image")
-		}
+	b.image, err = b.docker.SquashImage(b.image, fromID)
+	if err != nil {
+		return errors.Wrap(err, "error squashing image")
 	}
-
-	fmt.Fprintf(b.Stdout, "Successfully built %s\n", shortImgID)
-
-	imageID := image.ID(b.image)
-	for _, rt := range repoAndTags {
-		if err := b.docker.TagImageWithReference(imageID, rt); err != nil {
-			return "", err
-		}
-		fmt.Fprintf(b.Stdout, "Successfully tagged %s\n", reference.FamiliarString(rt))
-	}
-
-	return b.image, nil
+	return nil
 }
 
 func addNodesForLabelOption(dockerfile *parser.Node, labels map[string]string) {
@@ -303,6 +301,17 @@ func (b *Builder) warnOnUnusedBuildArgs() {
 	}
 }
 
+func (b *Builder) tagImages(repoAndTags []reference.Named) error {
+	imageID := image.ID(b.image)
+	for _, rt := range repoAndTags {
+		if err := b.docker.TagImageWithReference(imageID, rt); err != nil {
+			return err
+		}
+		fmt.Fprintf(b.Stdout, "Successfully tagged %s\n", reference.FamiliarString(rt))
+	}
+	return nil
+}
+
 // hasFromImage returns true if the builder has processed a `FROM <image>` line
 func (b *Builder) hasFromImage() bool {
 	return b.image != "" || b.noBaseImage
@@ -345,18 +354,31 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con
 	b.Stderr = ioutil.Discard
 	b.disableCommit = true
 
-	total := len(ast.Children)
-	for _, n := range ast.Children {
-		if err := b.checkDispatch(n, false); err != nil {
-			return nil, err
-		}
+	if err := checkDispatchDockerfile(ast); err != nil {
+		return nil, err
 	}
 
-	for i, n := range ast.Children {
-		if err := b.dispatch(i, total, n); err != nil {
-			return nil, err
-		}
+	if err := dispatchFromDockerfile(b, ast); err != nil {
+		return nil, err
 	}
-
 	return b.runConfig, nil
 }
+
+func checkDispatchDockerfile(dockerfile *parser.Node) error {
+	for _, n := range dockerfile.Children {
+		if err := checkDispatch(n); err != nil {
+			return errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
+		}
+	}
+	return nil
+}
+
+func dispatchFromDockerfile(b *Builder, ast *parser.Node) error {
+	total := len(ast.Children)
+	for i, n := range ast.Children {
+		if err := b.dispatch(i, total, n); err != nil {
+			return err
+		}
+	}
+	return nil
+}
diff --git a/components/engine/builder/dockerfile/evaluator.go b/components/engine/builder/dockerfile/evaluator.go
index 5be23c8cda..c456ba41f7 100644
--- a/components/engine/builder/dockerfile/evaluator.go
+++ b/components/engine/builder/dockerfile/evaluator.go
@@ -206,8 +206,7 @@ func (b *Builder) runConfigEnvMapping() map[string]string {
 // arg, env, etc., this syntax check will not be complete and could not replace
 // the runtime check. Instead, this function is only a helper that allows
 // user to find out the obvious error in Dockerfile earlier on.
-// onbuild bool: indicate if instruction XXX is part of `ONBUILD XXX` trigger
-func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error {
+func checkDispatch(ast *parser.Node) error {
 	cmd := ast.Value
 	upperCasedCmd := strings.ToUpper(cmd)
 
@@ -225,16 +224,6 @@ func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error {
 		}
 	}
 
-	// The instruction is part of ONBUILD trigger (not the instruction itself)
-	if onbuild {
-		switch upperCasedCmd {
-		case "ONBUILD":
-			return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
-		case "MAINTAINER", "FROM":
-			return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd)
-		}
-	}
-
 	if _, ok := evaluateTable[cmd]; ok {
 		return nil
 	}
diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go
index 122bb2019e..32698775d1 100644
--- a/components/engine/builder/dockerfile/internals.go
+++ b/components/engine/builder/dockerfile/internals.go
@@ -467,19 +467,24 @@ func (b *Builder) processImageFrom(img builder.Image) error {
 			return err
 		}
 
-		total := len(ast.Children)
 		for _, n := range ast.Children {
-			if err := b.checkDispatch(n, true); err != nil {
+			if err := checkDispatch(n); err != nil {
 				return err
 			}
+
+			upperCasedCmd := strings.ToUpper(n.Value)
+			switch upperCasedCmd {
+			case "ONBUILD":
+				return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
+			case "MAINTAINER", "FROM":
+				return errors.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd)
+			}
 		}
-		for i, n := range ast.Children {
-			if err := b.dispatch(i, total, n); err != nil {
-				return err
-			}
+
+		if err := dispatchFromDockerfile(b, ast); err != nil {
+			return err
 		}
 	}
-
 	return nil
 }
 
@@ -644,8 +649,8 @@ func (b *Builder) clearTmp() {
 	}
 }
 
-// readDockerfile reads a Dockerfile from the current context.
-func (b *Builder) readDockerfile() (*parser.Node, error) {
+// readAndParseDockerfile reads a Dockerfile from the current context.
+func (b *Builder) readAndParseDockerfile() (*parser.Node, error) {
 	// If no -f was specified then look for 'Dockerfile'. If we can't find
 	// that then look for 'dockerfile'.  If neither are found then default
 	// back to 'Dockerfile' and use that in the error message.
diff --git a/components/engine/builder/dockerfile/internals_test.go b/components/engine/builder/dockerfile/internals_test.go
index 56d5e4f182..1bffecfa75 100644
--- a/components/engine/builder/dockerfile/internals_test.go
+++ b/components/engine/builder/dockerfile/internals_test.go
@@ -77,6 +77,6 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath,
 
 	b := &Builder{options: options, context: context}
 
-	_, err = b.readDockerfile()
+	_, err = b.readAndParseDockerfile()
 	assert.Error(t, err, expectedError)
 }

From a00fca972da150e1c4562ce6cc89f8f1542f140d Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@docker.com>
Date: Tue, 11 Apr 2017 13:58:29 -0400
Subject: [PATCH 3/4] Remove unused id field from Builder.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Upstream-commit: a6abd57b83dc0aaf0cedeeb488c8a41262e46b7d
Component: engine
---
 components/engine/api/types/backend/backend.go  | 3 +--
 components/engine/builder/dockerfile/builder.go | 4 ----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/components/engine/api/types/backend/backend.go b/components/engine/api/types/backend/backend.go
index 83efae300b..de7e24f208 100644
--- a/components/engine/api/types/backend/backend.go
+++ b/components/engine/api/types/backend/backend.go
@@ -100,8 +100,7 @@ type ContainerCommitConfig struct {
 	Changes []string
 }
 
-// ProgressWriter is an interface
-// to transport progress streams.
+// ProgressWriter is a data object to transport progress streams to the client
 type ProgressWriter struct {
 	Output             io.Writer
 	StdoutFormatter    *streamformatter.StdoutFormatter
diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go
index d655fff7fb..76e0dcf998 100644
--- a/components/engine/builder/dockerfile/builder.go
+++ b/components/engine/builder/dockerfile/builder.go
@@ -65,9 +65,6 @@ type Builder struct {
 	buildArgs     *buildArgs
 	directive     parser.Directive
 
-	// TODO: remove once docker.Commit can receive a tag
-	id string
-
 	imageCache builder.ImageCache
 	from       builder.Image
 }
@@ -127,7 +124,6 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
 		context:       buildContext,
 		runConfig:     new(container.Config),
 		tmpContainers: map[string]struct{}{},
-		id:            stringid.GenerateNonCryptoID(),
 		buildArgs:     newBuildArgs(config.BuildArgs),
 		directive: parser.Directive{
 			EscapeSeen:           false,

From 862dffcf246b153f9d8000f80ded05e33e3b74ea Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@docker.com>
Date: Tue, 11 Apr 2017 14:06:51 -0400
Subject: [PATCH 4/4] Remove unused Builder.Cancel()

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Upstream-commit: 068f344e032ad4489a88665adec683e06ad6f3c7
Component: engine
---
 components/engine/builder/dockerfile/builder.go | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go
index 76e0dcf998..6f7f20d7d1 100644
--- a/components/engine/builder/dockerfile/builder.go
+++ b/components/engine/builder/dockerfile/builder.go
@@ -50,7 +50,6 @@ type Builder struct {
 	docker    builder.Backend
 	context   builder.Context
 	clientCtx context.Context
-	cancel    context.CancelFunc
 
 	runConfig     *container.Config // runconfig for cmd, run, entrypoint etc.
 	flags         *BFlags
@@ -113,10 +112,8 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
 	if config == nil {
 		config = new(types.ImageBuildOptions)
 	}
-	ctx, cancel := context.WithCancel(clientCtx)
 	b = &Builder{
-		clientCtx:     ctx,
-		cancel:        cancel,
+		clientCtx:     clientCtx,
 		options:       config,
 		Stdout:        os.Stdout,
 		Stderr:        os.Stderr,
@@ -313,11 +310,6 @@ func (b *Builder) hasFromImage() bool {
 	return b.image != "" || b.noBaseImage
 }
 
-// Cancel cancels an ongoing Dockerfile build.
-func (b *Builder) Cancel() {
-	b.cancel()
-}
-
 // BuildFromConfig builds directly from `changes`, treating it as if it were the contents of a Dockerfile
 // It will:
 // - Call parse.Parse() to get an AST root for the concatenated Dockerfile entries.