Refactor interaction between dispatcher.from and dispatchState

Signed-off-by: Daniel Nephin <dnephin@docker.com>

builder/builder.go

@@ -89,5 +89,5 @@ type Image interface {
 // ReleaseableLayer is an image layer that can be mounted and released
 type ReleaseableLayer interface {
 	Release() error
-	Mount() (string, error)
+	Mount(string) (string, error)
 }

builder/dockerfile/dispatchers.go

@@ -23,6 +23,7 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/strslice"
+	"github.com/docker/docker/builder/dockerfile/parser"
 	"github.com/docker/docker/pkg/signal"
 	"github.com/docker/go-connections/nat"
 	"github.com/pkg/errors"
@@ -196,24 +197,21 @@ func from(req dispatchRequest) error {
 	}
 
 	req.builder.resetImageCache()
-	req.state.noBaseImage = false
-	req.state.stageName = stageName
 	image, err := req.builder.getFromImage(req.shlex, req.args[0])
 	if err != nil {
 		return err
 	}
-	if image == nil {
-		req.state.imageID = ""
-		req.state.noBaseImage = true
-		image = newImageMount(nil, nil)
-	}
 	if err := req.builder.imageContexts.add(stageName, image); err != nil {
 		return err
 	}
-	req.state.baseImage = image
-
+	req.state.beginStage(stageName, image)
 	req.builder.buildArgs.ResetAllowed()
-	return req.builder.processImageFrom(req.state, image)
+	if image.ImageID() == "" {
+		// Typically this means they used "FROM scratch"
+		return nil
+	}
+
+	return processOnBuild(req)
 }
 
 func parseBuildStageName(args []string) (string, error) {
@@ -243,11 +241,7 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error
 	}
 
 	if im, ok := b.imageContexts.byName[name]; ok {
-		if len(im.ImageID()) > 0 {
-			return im, nil
-		}
-		// FROM scratch does not have an ImageID
-		return nil, nil
+		return im, nil
 	}
 
 	// Windows cannot support a container with no base image.
@@ -255,7 +249,7 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error
 		if runtime.GOOS == "windows" {
 			return nil, errors.New("Windows does not support FROM scratch")
 		}
-		return nil, nil
+		return newImageMount(nil, nil), nil
 	}
 	return b.getImage(name)
 }
@@ -272,6 +266,53 @@ func (b *Builder) getImage(name string) (*imageMount, error) {
 	return newImageMount(image, layer), nil
 }
 
+func processOnBuild(req dispatchRequest) error {
+	dispatchState := req.state
+	// Process ONBUILD triggers if they exist
+	if nTriggers := len(dispatchState.runConfig.OnBuild); nTriggers != 0 {
+		word := "trigger"
+		if nTriggers > 1 {
+			word = "triggers"
+		}
+		fmt.Fprintf(req.builder.Stderr, "# Executing %d build %s...\n", nTriggers, word)
+	}
+
+	// Copy the ONBUILD triggers, and remove them from the config, since the config will be committed.
+	onBuildTriggers := dispatchState.runConfig.OnBuild
+	dispatchState.runConfig.OnBuild = []string{}
+
+	// Reset stdin settings as all build actions run without stdin
+	dispatchState.runConfig.OpenStdin = false
+	dispatchState.runConfig.StdinOnce = false
+
+	// parse the ONBUILD triggers by invoking the parser
+	for _, step := range onBuildTriggers {
+		dockerfile, err := parser.Parse(strings.NewReader(step))
+		if err != nil {
+			return err
+		}
+
+		for _, n := range dockerfile.AST.Children {
+			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)
+			}
+		}
+
+		if _, err := dispatchFromDockerfile(req.builder, dockerfile, dispatchState); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // ONBUILD RUN echo yo
 //
 // ONBUILD triggers run when the image is used in a FROM statement.

builder/dockerfile/dispatchers_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/docker/docker/api/types/strslice"
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/dockerfile/parser"
+	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/pkg/testutil"
 	"github.com/docker/go-connections/nat"
 	"github.com/stretchr/testify/assert"
@@ -192,8 +193,9 @@ func TestFromScratch(t *testing.T) {
 	}
 
 	require.NoError(t, err)
+	assert.True(t, req.state.hasFromImage())
 	assert.Equal(t, "", req.state.imageID)
-	assert.Equal(t, true, req.state.noBaseImage)
+	assert.Equal(t, []string{"PATH=" + system.DefaultPathEnv}, req.state.runConfig.Env)
 }
 
 func TestFromWithArg(t *testing.T) {

builder/dockerfile/evaluator.go

@@ -28,6 +28,7 @@ import (
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/dockerfile/command"
 	"github.com/docker/docker/builder/dockerfile/parser"
+	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/runconfig/opts"
 	"github.com/pkg/errors"
 )
@@ -184,37 +185,55 @@ type dispatchOptions struct {
 
 // dispatchState is a data object which is modified by dispatchers
 type dispatchState struct {
-	runConfig   *container.Config
-	maintainer  string
-	cmdSet      bool
-	noBaseImage bool
-	imageID     string
-	baseImage   builder.Image
-	stageName   string
+	runConfig  *container.Config
+	maintainer string
+	cmdSet     bool
+	imageID    string
+	baseImage  builder.Image
+	stageName  string
 }
 
 func newDispatchState() *dispatchState {
 	return &dispatchState{runConfig: &container.Config{}}
 }
 
-func (r *dispatchState) updateRunConfig() {
-	r.runConfig.Image = r.imageID
+func (s *dispatchState) updateRunConfig() {
+	s.runConfig.Image = s.imageID
 }
 
 // hasFromImage returns true if the builder has processed a `FROM <image>` line
-func (r *dispatchState) hasFromImage() bool {
-	return r.imageID != "" || r.noBaseImage
+func (s *dispatchState) hasFromImage() bool {
+	return s.imageID != "" || (s.baseImage != nil && s.baseImage.ImageID() == "")
 }
 
-func (r *dispatchState) runConfigEnvMapping() map[string]string {
-	return opts.ConvertKVStringsToMap(r.runConfig.Env)
-}
-
-func (r *dispatchState) isCurrentStage(target string) bool {
+func (s *dispatchState) isCurrentStage(target string) bool {
 	if target == "" {
 		return false
 	}
-	return strings.EqualFold(r.stageName, target)
+	return strings.EqualFold(s.stageName, target)
+}
+
+func (s *dispatchState) beginStage(stageName string, image builder.Image) {
+	s.stageName = stageName
+	s.imageID = image.ImageID()
+
+	if image.RunConfig() != nil {
+		s.runConfig = image.RunConfig()
+	}
+	s.baseImage = image
+	s.setDefaultPath()
+}
+
+// Add the default PATH to runConfig.ENV if one exists for the platform and there
+// is no PATH set. Note that windows won't have one as it's set by HCS
+func (s *dispatchState) setDefaultPath() {
+	if system.DefaultPathEnv == "" {
+		return
+	}
+	envMap := opts.ConvertKVStringsToMap(s.runConfig.Env)
+	if _, ok := envMap["PATH"]; !ok {
+		s.runConfig.Env = append(s.runConfig.Env, "PATH="+system.DefaultPathEnv)
+	}
 }
 
 func handleOnBuildNode(ast *parser.Node, msg *bytes.Buffer) (*parser.Node, []string, error) {

builder/dockerfile/imagecontext.go

@@ -45,9 +45,9 @@ func (ic *imageContexts) update(imageID string, runConfig *container.Config) {
 func (ic *imageContexts) validate(i int) error {
 	if i < 0 || i >= len(ic.list)-1 {
 		if i == len(ic.list)-1 {
-			return errors.Errorf("%d refers to current build stage", i)
+			return errors.New("refers to current build stage")
 		}
-		return errors.Errorf("index out of bounds")
+		return errors.New("index out of bounds")
 	}
 	return nil
 }
@@ -122,7 +122,7 @@ func (im *imageMount) context() (builder.Source, error) {
 		if im.id == "" || im.layer == nil {
 			return nil, errors.Errorf("empty context")
 		}
-		mountPath, err := im.layer.Mount()
+		mountPath, err := im.layer.Mount(im.id)
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed to mount %s", im.id)
 		}
@@ -136,6 +136,9 @@ func (im *imageMount) context() (builder.Source, error) {
 }
 
 func (im *imageMount) unmount() error {
+	if im.layer == nil {
+		return nil
+	}
 	if err := im.layer.Release(); err != nil {
 		return errors.Wrapf(err, "failed to unmount previous build image %s", im.id)
 	}

builder/dockerfile/internals.go

@@ -22,7 +22,6 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/builder"
-	"github.com/docker/docker/builder/dockerfile/parser"
 	"github.com/docker/docker/builder/remotecontext"
 	"github.com/docker/docker/pkg/httputils"
 	"github.com/docker/docker/pkg/ioutils"
@@ -480,74 +479,6 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression
 	return copyInfos, nil
 }
 
-func (b *Builder) processImageFrom(dispatchState *dispatchState, img builder.Image) error {
-	if img != nil {
-		dispatchState.imageID = img.ImageID()
-
-		if img.RunConfig() != nil {
-			dispatchState.runConfig = img.RunConfig()
-		}
-	}
-
-	// 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 != "" {
-		if _, ok := dispatchState.runConfigEnvMapping()["PATH"]; !ok {
-			dispatchState.runConfig.Env = append(dispatchState.runConfig.Env,
-				"PATH="+system.DefaultPathEnv)
-		}
-	}
-
-	if img == nil {
-		// Typically this means they used "FROM scratch"
-		return nil
-	}
-
-	// Process ONBUILD triggers if they exist
-	if nTriggers := len(dispatchState.runConfig.OnBuild); nTriggers != 0 {
-		word := "trigger"
-		if nTriggers > 1 {
-			word = "triggers"
-		}
-		fmt.Fprintf(b.Stderr, "# Executing %d build %s...\n", nTriggers, word)
-	}
-
-	// Copy the ONBUILD triggers, and remove them from the config, since the config will be committed.
-	onBuildTriggers := dispatchState.runConfig.OnBuild
-	dispatchState.runConfig.OnBuild = []string{}
-
-	// Reset stdin settings as all build actions run without stdin
-	dispatchState.runConfig.OpenStdin = false
-	dispatchState.runConfig.StdinOnce = false
-
-	// parse the ONBUILD triggers by invoking the parser
-	for _, step := range onBuildTriggers {
-		dockerfile, err := parser.Parse(strings.NewReader(step))
-		if err != nil {
-			return err
-		}
-
-		for _, n := range dockerfile.AST.Children {
-			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)
-			}
-		}
-
-		if _, err := dispatchFromDockerfile(b, dockerfile, dispatchState); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 // probeCache checks if cache match can be found for current build instruction.
 // If an image is found, probeCache returns `(true, nil)`.
 // If no image is found, it returns `(false, nil)`.

builder/dockerfile/mockbackend_test.go

@@ -112,6 +112,6 @@ func (l *mockLayer) Release() error {
 	return nil
 }
 
-func (l *mockLayer) Mount() (string, error) {
+func (l *mockLayer) Mount(_ string) (string, error) {
 	return "mountPath", nil
 }

daemon/build.go

@@ -17,7 +17,7 @@ import (
 type releaseableLayer struct {
 	rwLayer layer.RWLayer
 	release func(layer.RWLayer) error
-	mount   func() (layer.RWLayer, error)
+	mount   func(string) (layer.RWLayer, error)
 }
 
 func (rl *releaseableLayer) Release() error {
@@ -28,10 +28,9 @@ func (rl *releaseableLayer) Release() error {
 	return rl.release(rl.rwLayer)
 }
 
-func (rl *releaseableLayer) Mount() (string, error) {
+func (rl *releaseableLayer) Mount(imageID string) (string, error) {
 	var err error
-	// daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil)
-	rl.rwLayer, err = rl.mount()
+	rl.rwLayer, err = rl.mount(imageID)
 	if err != nil {
 		return "", errors.Wrap(err, "failed to create rwlayer")
 	}
@@ -47,8 +46,12 @@ func (rl *releaseableLayer) Mount() (string, error) {
 	return mountPath, err
 }
 
-func (daemon *Daemon) getReleasableLayerForImage(img *image.Image) (*releaseableLayer, error) {
-	mountFunc := func() (layer.RWLayer, error) {
+func (daemon *Daemon) getReleasableLayerForImage() *releaseableLayer {
+	mountFunc := func(imageID string) (layer.RWLayer, error) {
+		img, err := daemon.GetImage(imageID)
+		if err != nil {
+			return nil, err
+		}
 		mountID := stringid.GenerateRandomID()
 		return daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil)
 	}
@@ -59,7 +62,7 @@ func (daemon *Daemon) getReleasableLayerForImage(img *image.Image) (*releaseable
 		return err
 	}
 
-	return &releaseableLayer{mount: mountFunc, release: releaseFunc}, nil
+	return &releaseableLayer{mount: mountFunc, release: releaseFunc}
 }
 
 // TODO: could this use the regular daemon PullImage ?
@@ -94,15 +97,10 @@ func (daemon *Daemon) GetImageAndLayer(ctx context.Context, refOrID string, opts
 		image, _ := daemon.GetImage(refOrID)
 		// TODO: shouldn't we error out if error is different from "not found" ?
 		if image != nil {
-			layer, err := daemon.getReleasableLayerForImage(image)
-			return image, layer, err
+			return image, daemon.getReleasableLayerForImage(), nil
 		}
 	}
 
 	image, err := daemon.pullForBuilder(ctx, refOrID, opts.AuthConfig, opts.Output)
-	if err != nil {
-		return nil, nil, err
-	}
-	layer, err := daemon.getReleasableLayerForImage(image)
-	return image, layer, err
+	return image, daemon.getReleasableLayerForImage(), err
 }

integration-cli/docker_cli_build_test.go

@@ -5946,7 +5946,7 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFSErrors(c *check.C) {
 			dockerfile: `
 		FROM busybox
 		COPY --from=0 foo bar`,
-			expectedError: "invalid from flag value 0 refers current build block",
+			expectedError: "invalid from flag value 0: refers to current build stage",
 		},
 		{
 			dockerfile: `