From 13d2adae3f0f304448a1e46a11088a21202e475b Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@docker.com>
Date: Thu, 25 May 2017 17:03:29 -0400
Subject: [PATCH] Fix copy when used with scratch and images with empty RootFS

Commit the rwLayer to get the correct DiffID
Refacator copy in thebuilder
move more code into exportImage
cleanup some windows tests
Release the newly commited layer.
Set the imageID on the buildStage after exporting a new image.
Move archiver to BuildManager.
Have ReleaseableLayer.Commit return a layer
and store the Image from exportImage in the local imageSources cache
Remove NewChild from image interface.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Upstream-commit: 51360965206b0db49cc0365dabb590063a17a9df
Component: engine
---
 .../api/server/backend/build/backend.go       |  5 +-
 components/engine/builder/builder.go          |  8 +-
 .../engine/builder/dockerfile/builder.go      |  9 +-
 components/engine/builder/dockerfile/copy.go  | 67 +++++++++-----
 .../engine/builder/dockerfile/copy_test.go    | 45 ++++++++++
 .../engine/builder/dockerfile/copy_unix.go    | 36 +-------
 .../engine/builder/dockerfile/imagecontext.go | 30 +++++--
 .../engine/builder/dockerfile/internals.go    | 87 ++++++++++++-------
 .../builder/dockerfile/internals_unix.go      |  2 +-
 .../builder/dockerfile/internals_windows.go   |  6 +-
 .../dockerfile/internals_windows_test.go      | 36 ++++----
 .../builder/dockerfile/mockbackend_test.go    | 20 ++---
 components/engine/cmd/dockerd/daemon.go       |  2 +-
 components/engine/daemon/archive.go           |  2 +-
 components/engine/daemon/build.go             | 64 ++++++++++----
 components/engine/daemon/commit.go            |  2 +-
 components/engine/image/image.go              | 11 ++-
 components/engine/image/store.go              |  4 +-
 .../integration-cli/docker_cli_build_test.go  | 41 ++++-----
 .../engine/pkg/testutil/tempfile/tempfile.go  | 20 +++++
 20 files changed, 318 insertions(+), 179 deletions(-)
 create mode 100644 components/engine/builder/dockerfile/copy_test.go

diff --git a/components/engine/api/server/backend/build/backend.go b/components/engine/api/server/backend/build/backend.go
index bf171441ca..00aa7c3126 100644
--- a/components/engine/api/server/backend/build/backend.go
+++ b/components/engine/api/server/backend/build/backend.go
@@ -8,6 +8,7 @@ import (
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/dockerfile"
 	"github.com/docker/docker/image"
+	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
@@ -26,8 +27,8 @@ type Backend struct {
 }
 
 // NewBackend creates a new build backend from components
-func NewBackend(components ImageComponent, builderBackend builder.Backend) *Backend {
-	manager := dockerfile.NewBuildManager(builderBackend)
+func NewBackend(components ImageComponent, builderBackend builder.Backend, idMappings *idtools.IDMappings) *Backend {
+	manager := dockerfile.NewBuildManager(builderBackend, idMappings)
 	return &Backend{imageComponent: components, manager: manager}
 }
 
diff --git a/components/engine/builder/builder.go b/components/engine/builder/builder.go
index 07a5500adb..5cd6c838f9 100644
--- a/components/engine/builder/builder.go
+++ b/components/engine/builder/builder.go
@@ -11,9 +11,7 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
 	containerpkg "github.com/docker/docker/container"
-	"github.com/docker/docker/image"
 	"github.com/docker/docker/layer"
-	"github.com/docker/docker/pkg/idtools"
 	"golang.org/x/net/context"
 )
 
@@ -45,9 +43,7 @@ type Backend interface {
 	// ContainerCreateWorkdir creates the workdir
 	ContainerCreateWorkdir(containerID string) error
 
-	CreateImage(config []byte, parent string) (string, error)
-
-	IDMappings() *idtools.IDMappings
+	CreateImage(config []byte, parent string) (Image, error)
 
 	ImageCacheBuilder
 }
@@ -98,12 +94,12 @@ type Image interface {
 	ImageID() string
 	RunConfig() *container.Config
 	MarshalJSON() ([]byte, error)
-	NewChild(child image.ChildConfig) *image.Image
 }
 
 // ReleaseableLayer is an image layer that can be mounted and released
 type ReleaseableLayer interface {
 	Release() error
 	Mount() (string, error)
+	Commit() (ReleaseableLayer, error)
 	DiffID() layer.DiffID
 }
diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go
index c50570a6fd..d35ed9b91a 100644
--- a/components/engine/builder/dockerfile/builder.go
+++ b/components/engine/builder/dockerfile/builder.go
@@ -17,6 +17,7 @@ import (
 	"github.com/docker/docker/builder/remotecontext"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/chrootarchive"
+	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/streamformatter"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/pkg/errors"
@@ -39,15 +40,17 @@ var validCommitCommands = map[string]bool{
 
 // BuildManager is shared across all Builder objects
 type BuildManager struct {
+	archiver  *archive.Archiver
 	backend   builder.Backend
 	pathCache pathCache // TODO: make this persistent
 }
 
 // NewBuildManager creates a BuildManager
-func NewBuildManager(b builder.Backend) *BuildManager {
+func NewBuildManager(b builder.Backend, idMappings *idtools.IDMappings) *BuildManager {
 	return &BuildManager{
 		backend:   b,
 		pathCache: &syncmap.Map{},
+		archiver:  chrootarchive.NewArchiver(idMappings),
 	}
 }
 
@@ -75,6 +78,7 @@ func (bm *BuildManager) Build(ctx context.Context, config backend.BuildConfig) (
 		ProgressWriter: config.ProgressWriter,
 		Backend:        bm.backend,
 		PathCache:      bm.pathCache,
+		Archiver:       bm.archiver,
 	}
 	return newBuilder(ctx, builderOptions).build(source, dockerfile)
 }
@@ -85,6 +89,7 @@ type builderOptions struct {
 	Backend        builder.Backend
 	ProgressWriter backend.ProgressWriter
 	PathCache      pathCache
+	Archiver       *archive.Archiver
 }
 
 // Builder is a Dockerfile builder
@@ -124,7 +129,7 @@ func newBuilder(clientCtx context.Context, options builderOptions) *Builder {
 		Aux:              options.ProgressWriter.AuxFormatter,
 		Output:           options.ProgressWriter.Output,
 		docker:           options.Backend,
-		archiver:         chrootarchive.NewArchiver(options.Backend.IDMappings()),
+		archiver:         options.Archiver,
 		buildArgs:        newBuildArgs(config.BuildArgs),
 		buildStages:      newBuildStages(),
 		imageSources:     newImageSources(clientCtx, options),
diff --git a/components/engine/builder/dockerfile/copy.go b/components/engine/builder/dockerfile/copy.go
index 268036e26c..0026bdc158 100644
--- a/components/engine/builder/dockerfile/copy.go
+++ b/components/engine/builder/dockerfile/copy.go
@@ -368,7 +368,7 @@ type copyFileOptions struct {
 	archiver   *archive.Archiver
 }
 
-func copyFile(dest copyInfo, source copyInfo, options copyFileOptions) error {
+func performCopyForInfo(dest copyInfo, source copyInfo, options copyFileOptions) error {
 	srcPath, err := source.fullPath()
 	if err != nil {
 		return err
@@ -379,36 +379,63 @@ func copyFile(dest copyInfo, source copyInfo, options copyFileOptions) error {
 	}
 
 	archiver := options.archiver
-	rootIDs := archiver.IDMappings.RootPair()
 
 	src, err := os.Stat(srcPath)
 	if err != nil {
 		return errors.Wrapf(err, "source path not found")
 	}
 	if src.IsDir() {
-		if err := archiver.CopyWithTar(srcPath, destPath); err != nil {
-			return err
-		}
-		return fixPermissions(srcPath, destPath, rootIDs)
+		return copyDirectory(archiver, srcPath, destPath)
 	}
-
 	if options.decompress && archive.IsArchivePath(srcPath) {
-		// To support the untar feature we need to clean up the path a little bit
-		// because tar is not very forgiving
-		tarDest := dest.path
-		// TODO: could this be just TrimSuffix()?
-		if strings.HasSuffix(tarDest, string(os.PathSeparator)) {
-			tarDest = filepath.Dir(dest.path)
-		}
-		return archiver.UntarPath(srcPath, tarDest)
+		return archiver.UntarPath(srcPath, destPath)
 	}
 
-	if err := idtools.MkdirAllAndChownNew(filepath.Dir(destPath), 0755, rootIDs); err != nil {
+	destExistsAsDir, err := isExistingDirectory(destPath)
+	if err != nil {
 		return err
 	}
-	if err := archiver.CopyFileWithTar(srcPath, destPath); err != nil {
-		return err
+	// dest.path must be used because destPath has already been cleaned of any
+	// trailing slash
+	if endsInSlash(dest.path) || destExistsAsDir {
+		// source.path must be used to get the correct filename when the source
+		// is a symlink
+		destPath = filepath.Join(destPath, filepath.Base(source.path))
 	}
-	// TODO: do I have to change destPath to the filename?
-	return fixPermissions(srcPath, destPath, rootIDs)
+	return copyFile(archiver, srcPath, destPath)
+}
+
+func copyDirectory(archiver *archive.Archiver, source, dest string) error {
+	if err := archiver.CopyWithTar(source, dest); err != nil {
+		return errors.Wrapf(err, "failed to copy directory")
+	}
+	return fixPermissions(source, dest, archiver.IDMappings.RootPair())
+}
+
+func copyFile(archiver *archive.Archiver, source, dest string) error {
+	rootIDs := archiver.IDMappings.RootPair()
+
+	if err := idtools.MkdirAllAndChownNew(filepath.Dir(dest), 0755, rootIDs); err != nil {
+		return errors.Wrapf(err, "failed to create new directory")
+	}
+	if err := archiver.CopyFileWithTar(source, dest); err != nil {
+		return errors.Wrapf(err, "failed to copy file")
+	}
+	return fixPermissions(source, dest, rootIDs)
+}
+
+func endsInSlash(path string) bool {
+	return strings.HasSuffix(path, string(os.PathSeparator))
+}
+
+// isExistingDirectory returns true if the path exists and is a directory
+func isExistingDirectory(path string) (bool, error) {
+	destStat, err := os.Stat(path)
+	switch {
+	case os.IsNotExist(err):
+		return false, nil
+	case err != nil:
+		return false, err
+	}
+	return destStat.IsDir(), nil
 }
diff --git a/components/engine/builder/dockerfile/copy_test.go b/components/engine/builder/dockerfile/copy_test.go
new file mode 100644
index 0000000000..aee225b5ff
--- /dev/null
+++ b/components/engine/builder/dockerfile/copy_test.go
@@ -0,0 +1,45 @@
+package dockerfile
+
+import (
+	"testing"
+
+	"github.com/docker/docker/pkg/testutil/tempfile"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIsExistingDirectory(t *testing.T) {
+	tmpfile := tempfile.NewTempFile(t, "file-exists-test", "something")
+	defer tmpfile.Remove()
+	tmpdir := tempfile.NewTempDir(t, "dir-exists-test")
+	defer tmpdir.Remove()
+
+	var testcases = []struct {
+		doc      string
+		path     string
+		expected bool
+	}{
+		{
+			doc:      "directory exists",
+			path:     tmpdir.Path,
+			expected: true,
+		},
+		{
+			doc:      "path doesn't exist",
+			path:     "/bogus/path/does/not/exist",
+			expected: false,
+		},
+		{
+			doc:      "file exists",
+			path:     tmpfile.Name(),
+			expected: false,
+		},
+	}
+
+	for _, testcase := range testcases {
+		result, err := isExistingDirectory(testcase.path)
+		if !assert.NoError(t, err) {
+			continue
+		}
+		assert.Equal(t, testcase.expected, result, testcase.doc)
+	}
+}
diff --git a/components/engine/builder/dockerfile/copy_unix.go b/components/engine/builder/dockerfile/copy_unix.go
index ecbbd33368..326d95bb39 100644
--- a/components/engine/builder/dockerfile/copy_unix.go
+++ b/components/engine/builder/dockerfile/copy_unix.go
@@ -1,3 +1,5 @@
+// +build !windows
+
 package dockerfile
 
 import (
@@ -7,20 +9,8 @@ import (
 	"github.com/docker/docker/pkg/idtools"
 )
 
-func pathExists(path string) (bool, error) {
-	_, err := os.Stat(path)
-	switch {
-	case err == nil:
-		return true, nil
-	case os.IsNotExist(err):
-		return false, nil
-	}
-	return false, err
-}
-
-// TODO: review this
 func fixPermissions(source, destination string, rootIDs idtools.IDPair) error {
-	doChownDestination, err := chownDestinationRoot(destination)
+	skipChownRoot, err := isExistingDirectory(destination)
 	if err != nil {
 		return err
 	}
@@ -30,7 +20,7 @@ func fixPermissions(source, destination string, rootIDs idtools.IDPair) error {
 	return filepath.Walk(source, func(fullpath string, info os.FileInfo, err error) error {
 		// Do not alter the walk root iff. it existed before, as it doesn't fall under
 		// the domain of "things we should chown".
-		if !doChownDestination && (source == fullpath) {
+		if skipChownRoot && source == fullpath {
 			return nil
 		}
 
@@ -44,21 +34,3 @@ func fixPermissions(source, destination string, rootIDs idtools.IDPair) error {
 		return os.Lchown(fullpath, rootIDs.UID, rootIDs.GID)
 	})
 }
-
-// If the destination didn't already exist, or the destination isn't a
-// directory, then we should Lchown the destination. Otherwise, we shouldn't
-// Lchown the destination.
-func chownDestinationRoot(destination string) (bool, error) {
-	destExists, err := pathExists(destination)
-	if err != nil {
-		return false, err
-	}
-	destStat, err := os.Stat(destination)
-	if err != nil {
-		// This should *never* be reached, because the destination must've already
-		// been created while untar-ing the context.
-		return false, err
-	}
-
-	return !destExists || !destStat.IsDir(), nil
-}
diff --git a/components/engine/builder/dockerfile/imagecontext.go b/components/engine/builder/dockerfile/imagecontext.go
index 4b47862ce6..2a5b6e2579 100644
--- a/components/engine/builder/dockerfile/imagecontext.go
+++ b/components/engine/builder/dockerfile/imagecontext.go
@@ -8,7 +8,7 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/remotecontext"
-	"github.com/docker/docker/layer"
+	dockerimage "github.com/docker/docker/image"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 )
@@ -92,6 +92,7 @@ type getAndMountFunc func(string) (builder.Image, builder.ReleaseableLayer, erro
 // all images so they can be unmounted at the end of the build.
 type imageSources struct {
 	byImageID map[string]*imageMount
+	withoutID []*imageMount
 	getImage  getAndMountFunc
 	cache     pathCache // TODO: remove
 }
@@ -121,7 +122,7 @@ func (m *imageSources) Get(idOrRef string) (*imageMount, error) {
 		return nil, err
 	}
 	im := newImageMount(image, layer)
-	m.byImageID[image.ImageID()] = im
+	m.Add(im)
 	return im, nil
 }
 
@@ -132,9 +133,25 @@ func (m *imageSources) Unmount() (retErr error) {
 			retErr = err
 		}
 	}
+	for _, im := range m.withoutID {
+		if err := im.unmount(); err != nil {
+			logrus.Error(err)
+			retErr = err
+		}
+	}
 	return
 }
 
+func (m *imageSources) Add(im *imageMount) {
+	switch im.image {
+	case nil:
+		im.image = &dockerimage.Image{}
+		m.withoutID = append(m.withoutID, im)
+	default:
+		m.byImageID[im.image.ImageID()] = im
+	}
+}
+
 // imageMount is a reference to an image that can be used as a builder.Source
 type imageMount struct {
 	image  builder.Image
@@ -172,6 +189,7 @@ func (im *imageMount) unmount() error {
 	if err := im.layer.Release(); err != nil {
 		return errors.Wrapf(err, "failed to unmount previous build image %s", im.image.ImageID())
 	}
+	im.layer = nil
 	return nil
 }
 
@@ -179,10 +197,10 @@ func (im *imageMount) Image() builder.Image {
 	return im.image
 }
 
+func (im *imageMount) Layer() builder.ReleaseableLayer {
+	return im.layer
+}
+
 func (im *imageMount) ImageID() string {
 	return im.image.ImageID()
 }
-
-func (im *imageMount) DiffID() layer.DiffID {
-	return im.layer.DiffID()
-}
diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go
index 095b6069c4..213e64a52e 100644
--- a/components/engine/builder/dockerfile/internals.go
+++ b/components/engine/builder/dockerfile/internals.go
@@ -12,7 +12,6 @@ import (
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
-	"github.com/docker/docker/builder"
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/pkg/errors"
@@ -65,14 +64,44 @@ func (b *Builder) commitContainer(dispatchState *dispatchState, id string, conta
 	return nil
 }
 
-func (b *Builder) exportImage(state *dispatchState, image builder.Image) error {
-	config, err := image.MarshalJSON()
+func (b *Builder) exportImage(state *dispatchState, imageMount *imageMount, runConfig *container.Config) error {
+	newLayer, err := imageMount.Layer().Commit()
+	if err != nil {
+		return err
+	}
+
+	// add an image mount without an image so the layer is properly unmounted
+	// if there is an error before we can add the full mount with image
+	b.imageSources.Add(newImageMount(nil, newLayer))
+
+	parentImage, ok := imageMount.Image().(*image.Image)
+	if !ok {
+		return errors.Errorf("unexpected image type")
+	}
+
+	newImage := image.NewChildImage(parentImage, image.ChildConfig{
+		Author:          state.maintainer,
+		ContainerConfig: runConfig,
+		DiffID:          newLayer.DiffID(),
+		Config:          copyRunConfig(state.runConfig),
+	})
+
+	// TODO: it seems strange to marshal this here instead of just passing in the
+	// image struct
+	config, err := newImage.MarshalJSON()
 	if err != nil {
 		return errors.Wrap(err, "failed to encode image config")
 	}
 
-	state.imageID, err = b.docker.CreateImage(config, state.imageID)
-	return err
+	exportedImage, err := b.docker.CreateImage(config, state.imageID)
+	if err != nil {
+		return errors.Wrapf(err, "failed to export image")
+	}
+
+	state.imageID = exportedImage.ImageID()
+	b.imageSources.Add(newImageMount(exportedImage, newLayer))
+	b.buildStages.update(state.imageID)
+	return nil
 }
 
 func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error {
@@ -82,46 +111,46 @@ func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error
 	runConfigWithCommentCmd := copyRunConfig(
 		state.runConfig,
 		withCmdCommentString(fmt.Sprintf("%s %s in %s ", inst.cmdName, srcHash, inst.dest)))
-	containerID, err := b.probeAndCreate(state, runConfigWithCommentCmd)
-	if err != nil || containerID == "" {
-		return err
-	}
-
-	// Twiddle the destination when it's a relative path - meaning, make it
-	// relative to the WORKINGDIR
-	dest, err := normaliseDest(inst.cmdName, state.runConfig.WorkingDir, inst.dest)
-	if err != nil {
+	hit, err := b.probeCache(state, runConfigWithCommentCmd)
+	if err != nil || hit {
 		return err
 	}
 
 	imageMount, err := b.imageSources.Get(state.imageID)
+	if err != nil {
+		return errors.Wrapf(err, "failed to get destination image %q", state.imageID)
+	}
+	destInfo, err := createDestInfo(state.runConfig.WorkingDir, inst, imageMount)
 	if err != nil {
 		return err
 	}
-	destSource, err := imageMount.Source()
-	if err != nil {
-		return errors.Wrapf(err, "failed to mount copy source")
-	}
 
-	destInfo := newCopyInfoFromSource(destSource, dest, "")
 	opts := copyFileOptions{
 		decompress: inst.allowLocalDecompression,
 		archiver:   b.archiver,
 	}
 	for _, info := range inst.infos {
-		if err := copyFile(destInfo, info, opts); err != nil {
-			return err
+		if err := performCopyForInfo(destInfo, info, opts); err != nil {
+			return errors.Wrapf(err, "failed to copy files")
 		}
 	}
+	return b.exportImage(state, imageMount, runConfigWithCommentCmd)
+}
 
-	newImage := imageMount.Image().NewChild(image.ChildConfig{
-		Author:          state.maintainer,
-		DiffID:          imageMount.DiffID(),
-		ContainerConfig: runConfigWithCommentCmd,
-		// TODO: ContainerID?
-		// TODO: Config?
-	})
-	return b.exportImage(state, newImage)
+func createDestInfo(workingDir string, inst copyInstruction, imageMount *imageMount) (copyInfo, error) {
+	// Twiddle the destination when it's a relative path - meaning, make it
+	// relative to the WORKINGDIR
+	dest, err := normaliseDest(workingDir, inst.dest)
+	if err != nil {
+		return copyInfo{}, errors.Wrapf(err, "invalid %s", inst.cmdName)
+	}
+
+	destMount, err := imageMount.Source()
+	if err != nil {
+		return copyInfo{}, errors.Wrapf(err, "failed to mount copy source")
+	}
+
+	return newCopyInfoFromSource(destMount, dest, ""), nil
 }
 
 // For backwards compat, if there's just one info then use it as the
diff --git a/components/engine/builder/dockerfile/internals_unix.go b/components/engine/builder/dockerfile/internals_unix.go
index 17102cb443..f4784e1cca 100644
--- a/components/engine/builder/dockerfile/internals_unix.go
+++ b/components/engine/builder/dockerfile/internals_unix.go
@@ -12,7 +12,7 @@ import (
 
 // normaliseDest normalises the destination of a COPY/ADD command in a
 // platform semantically consistent way.
-func normaliseDest(cmdName, workingDir, requested string) (string, error) {
+func normaliseDest(workingDir, requested string) (string, error) {
 	dest := filepath.FromSlash(requested)
 	endsInSlash := strings.HasSuffix(requested, string(os.PathSeparator))
 	if !system.IsAbs(requested) {
diff --git a/components/engine/builder/dockerfile/internals_windows.go b/components/engine/builder/dockerfile/internals_windows.go
index 81a0386251..bb3285925f 100644
--- a/components/engine/builder/dockerfile/internals_windows.go
+++ b/components/engine/builder/dockerfile/internals_windows.go
@@ -12,7 +12,7 @@ import (
 
 // normaliseDest normalises the destination of a COPY/ADD command in a
 // platform semantically consistent way.
-func normaliseDest(cmdName, workingDir, requested string) (string, error) {
+func normaliseDest(workingDir, requested string) (string, error) {
 	dest := filepath.FromSlash(requested)
 	endsInSlash := strings.HasSuffix(dest, string(os.PathSeparator))
 
@@ -32,7 +32,7 @@ func normaliseDest(cmdName, workingDir, requested string) (string, error) {
 	// we only want to validate where the DriveColon part has been supplied.
 	if filepath.IsAbs(dest) {
 		if strings.ToUpper(string(dest[0])) != "C" {
-			return "", fmt.Errorf("Windows does not support %s with a destinations not on the system drive (C:)", cmdName)
+			return "", fmt.Errorf("Windows does not support destinations not on the system drive (C:)")
 		}
 		dest = dest[2:] // Strip the drive letter
 	}
@@ -44,7 +44,7 @@ func normaliseDest(cmdName, workingDir, requested string) (string, error) {
 		}
 		if !system.IsAbs(dest) {
 			if string(workingDir[0]) != "C" {
-				return "", fmt.Errorf("Windows does not support %s with relative paths when WORKDIR is not the system drive", cmdName)
+				return "", fmt.Errorf("Windows does not support relative paths when WORKDIR is not the system drive")
 			}
 			dest = filepath.Join(string(os.PathSeparator), workingDir[2:], dest)
 			// Make sure we preserve any trailing slash
diff --git a/components/engine/builder/dockerfile/internals_windows_test.go b/components/engine/builder/dockerfile/internals_windows_test.go
index 868a6671a3..b4c8d4b3cb 100644
--- a/components/engine/builder/dockerfile/internals_windows_test.go
+++ b/components/engine/builder/dockerfile/internals_windows_test.go
@@ -2,16 +2,22 @@
 
 package dockerfile
 
-import "testing"
+import (
+	"fmt"
+	"testing"
+
+	"github.com/docker/docker/pkg/testutil"
+	"github.com/stretchr/testify/assert"
+)
 
 func TestNormaliseDest(t *testing.T) {
 	tests := []struct{ current, requested, expected, etext string }{
-		{``, `D:\`, ``, `Windows does not support TEST with a destinations not on the system drive (C:)`},
-		{``, `e:/`, ``, `Windows does not support TEST with a destinations not on the system drive (C:)`},
+		{``, `D:\`, ``, `Windows does not support destinations not on the system drive (C:)`},
+		{``, `e:/`, ``, `Windows does not support destinations not on the system drive (C:)`},
 		{`invalid`, `./c1`, ``, `Current WorkingDir invalid is not platform consistent`},
 		{`C:`, ``, ``, `Current WorkingDir C: is not platform consistent`},
 		{`C`, ``, ``, `Current WorkingDir C is not platform consistent`},
-		{`D:\`, `.`, ``, "Windows does not support TEST with relative paths when WORKDIR is not the system drive"},
+		{`D:\`, `.`, ``, "Windows does not support relative paths when WORKDIR is not the system drive"},
 		{``, `D`, `D`, ``},
 		{``, `./a1`, `.\a1`, ``},
 		{``, `.\b1`, `.\b1`, ``},
@@ -32,20 +38,16 @@ func TestNormaliseDest(t *testing.T) {
 		{`C:\wdm`, `foo/bar/`, `\wdm\foo\bar\`, ``},
 		{`C:\wdn`, `foo\bar/`, `\wdn\foo\bar\`, ``},
 	}
-	for _, i := range tests {
-		got, err := normaliseDest("TEST", i.current, i.requested)
-		if err != nil && i.etext == "" {
-			t.Fatalf("TestNormaliseDest Got unexpected error %q for %s %s. ", err.Error(), i.current, i.requested)
-		}
-		if i.etext != "" && ((err == nil) || (err != nil && err.Error() != i.etext)) {
-			if err == nil {
-				t.Fatalf("TestNormaliseDest Expected an error for %s %s but didn't get one", i.current, i.requested)
-			} else {
-				t.Fatalf("TestNormaliseDest Wrong error text for %s %s - %s", i.current, i.requested, err.Error())
+	for _, testcase := range tests {
+		msg := fmt.Sprintf("Input: %s, %s", testcase.current, testcase.requested)
+		actual, err := normaliseDest(testcase.current, testcase.requested)
+		if testcase.etext == "" {
+			if !assert.NoError(t, err, msg) {
+				continue
 			}
-		}
-		if i.etext == "" && got != i.expected {
-			t.Fatalf("TestNormaliseDest Expected %q for %q and %q. Got %q", i.expected, i.current, i.requested, got)
+			assert.Equal(t, testcase.expected, actual, msg)
+		} else {
+			testutil.ErrorContains(t, err, testcase.etext)
 		}
 	}
 }
diff --git a/components/engine/builder/dockerfile/mockbackend_test.go b/components/engine/builder/dockerfile/mockbackend_test.go
index cf4c83edd8..876bb91f1b 100644
--- a/components/engine/builder/dockerfile/mockbackend_test.go
+++ b/components/engine/builder/dockerfile/mockbackend_test.go
@@ -9,9 +9,7 @@ import (
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/builder"
 	containerpkg "github.com/docker/docker/container"
-	"github.com/docker/docker/image"
 	"github.com/docker/docker/layer"
-	"github.com/docker/docker/pkg/idtools"
 	"golang.org/x/net/context"
 )
 
@@ -80,12 +78,8 @@ func (m *MockBackend) MakeImageCache(cacheFrom []string) builder.ImageCache {
 	return nil
 }
 
-func (m *MockBackend) CreateImage(config []byte, parent string) (string, error) {
-	return "c411d1d", nil
-}
-
-func (m *MockBackend) IDMappings() *idtools.IDMappings {
-	return &idtools.IDMappings{}
+func (m *MockBackend) CreateImage(config []byte, parent string) (builder.Image, error) {
+	return nil, nil
 }
 
 type mockImage struct {
@@ -106,10 +100,6 @@ func (i *mockImage) MarshalJSON() ([]byte, error) {
 	return json.Marshal(rawImage(*i))
 }
 
-func (i *mockImage) NewChild(child image.ChildConfig) *image.Image {
-	return nil
-}
-
 type mockImageCache struct {
 	getCacheFunc func(parentID string, cfg *container.Config) (string, error)
 }
@@ -131,6 +121,10 @@ func (l *mockLayer) Mount() (string, error) {
 	return "mountPath", nil
 }
 
+func (l *mockLayer) Commit() (builder.ReleaseableLayer, error) {
+	return nil, nil
+}
+
 func (l *mockLayer) DiffID() layer.DiffID {
-	return layer.DiffID("abcdef12345")
+	return layer.DiffID("abcdef")
 }
diff --git a/components/engine/cmd/dockerd/daemon.go b/components/engine/cmd/dockerd/daemon.go
index 428a877dc9..d5c411f06d 100644
--- a/components/engine/cmd/dockerd/daemon.go
+++ b/components/engine/cmd/dockerd/daemon.go
@@ -449,7 +449,7 @@ func initRouter(s *apiserver.Server, d *daemon.Daemon, c *cluster.Cluster) {
 		image.NewRouter(d, decoder),
 		systemrouter.NewRouter(d, c),
 		volume.NewRouter(d),
-		build.NewRouter(buildbackend.NewBackend(d, d), d),
+		build.NewRouter(buildbackend.NewBackend(d, d, d.IDMappings()), d),
 		swarmrouter.NewRouter(c),
 		pluginrouter.NewRouter(d.PluginManager()),
 		distributionrouter.NewRouter(d),
diff --git a/components/engine/daemon/archive.go b/components/engine/daemon/archive.go
index 5db1ef9826..bd00daca53 100644
--- a/components/engine/daemon/archive.go
+++ b/components/engine/daemon/archive.go
@@ -359,4 +359,4 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str
 	})
 	daemon.LogContainerEvent(container, "copy")
 	return reader, nil
-}
\ No newline at end of file
+}
diff --git a/components/engine/daemon/build.go b/components/engine/daemon/build.go
index 3005ede7d1..3faa2affd4 100644
--- a/components/engine/daemon/build.go
+++ b/components/engine/daemon/build.go
@@ -1,6 +1,8 @@
 package daemon
 
 import (
+	"io"
+
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
@@ -13,7 +15,6 @@ import (
 	"github.com/docker/docker/registry"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
-	"io"
 )
 
 type releaseableLayer struct {
@@ -23,12 +24,14 @@ type releaseableLayer struct {
 }
 
 func (rl *releaseableLayer) Mount() (string, error) {
-	if rl.roLayer == nil {
-		return "", errors.New("can not mount an image with no root FS")
-	}
 	var err error
+	var chainID layer.ChainID
+	if rl.roLayer != nil {
+		chainID = rl.roLayer.ChainID()
+	}
+
 	mountID := stringid.GenerateRandomID()
-	rl.rwLayer, err = rl.layerStore.CreateRWLayer(mountID, rl.roLayer.ChainID(), nil)
+	rl.rwLayer, err = rl.layerStore.CreateRWLayer(mountID, chainID, nil)
 	if err != nil {
 		return "", errors.Wrap(err, "failed to create rwlayer")
 	}
@@ -36,15 +39,41 @@ func (rl *releaseableLayer) Mount() (string, error) {
 	return rl.rwLayer.Mount("")
 }
 
-func (rl *releaseableLayer) Release() error {
-	rl.releaseRWLayer()
-	return rl.releaseROLayer()
+func (rl *releaseableLayer) Commit() (builder.ReleaseableLayer, error) {
+	var chainID layer.ChainID
+	if rl.roLayer != nil {
+		chainID = rl.roLayer.ChainID()
+	}
+
+	stream, err := rl.rwLayer.TarStream()
+	if err != nil {
+		return nil, err
+	}
+
+	newLayer, err := rl.layerStore.Register(stream, chainID)
+	if err != nil {
+		return nil, err
+	}
+
+	if layer.IsEmpty(newLayer.DiffID()) {
+		_, err := rl.layerStore.Release(newLayer)
+		return &releaseableLayer{layerStore: rl.layerStore}, err
+	}
+	return &releaseableLayer{layerStore: rl.layerStore, roLayer: newLayer}, nil
 }
 
 func (rl *releaseableLayer) DiffID() layer.DiffID {
+	if rl.roLayer == nil {
+		return layer.DigestSHA256EmptyTar
+	}
 	return rl.roLayer.DiffID()
 }
 
+func (rl *releaseableLayer) Release() error {
+	rl.releaseRWLayer()
+	return rl.releaseROLayer()
+}
+
 func (rl *releaseableLayer) releaseRWLayer() error {
 	if rl.rwLayer == nil {
 		return nil
@@ -67,8 +96,8 @@ func (rl *releaseableLayer) releaseROLayer() error {
 }
 
 func newReleasableLayerForImage(img *image.Image, layerStore layer.Store) (builder.ReleaseableLayer, error) {
-	if img.RootFS.ChainID() == "" {
-		return nil, nil
+	if img == nil || img.RootFS.ChainID() == "" {
+		return &releaseableLayer{layerStore: layerStore}, nil
 	}
 	// Hold a reference to the image layer so that it can't be removed before
 	// it is released
@@ -109,6 +138,11 @@ func (daemon *Daemon) pullForBuilder(ctx context.Context, name string, authConfi
 // Every call to GetImageAndReleasableLayer MUST call releasableLayer.Release() to prevent
 // leaking of layers.
 func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) {
+	if refOrID == "" {
+		layer, err := newReleasableLayerForImage(nil, daemon.layerStore)
+		return nil, layer, err
+	}
+
 	if !opts.ForcePull {
 		image, _ := daemon.GetImage(refOrID)
 		// TODO: shouldn't we error out if error is different from "not found" ?
@@ -129,19 +163,19 @@ func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID st
 // CreateImage creates a new image by adding a config and ID to the image store.
 // This is similar to LoadImage() except that it receives JSON encoded bytes of
 // an image instead of a tar archive.
-func (daemon *Daemon) CreateImage(config []byte, parent string) (string, error) {
+func (daemon *Daemon) CreateImage(config []byte, parent string) (builder.Image, error) {
 	id, err := daemon.imageStore.Create(config)
 	if err != nil {
-		return "", err
+		return nil, errors.Wrapf(err, "failed to create image")
 	}
 
 	if parent != "" {
 		if err := daemon.imageStore.SetParent(id, image.ID(parent)); err != nil {
-			return "", err
+			return nil, errors.Wrapf(err, "failed to set parent %s", parent)
 		}
 	}
-	// TODO: do we need any daemon.LogContainerEventWithAttributes?
-	return id.String(), nil
+
+	return daemon.imageStore.Get(id)
 }
 
 // IDMappings returns uid/gid mappings for the builder
diff --git a/components/engine/daemon/commit.go b/components/engine/daemon/commit.go
index 8125d28f65..6011628092 100644
--- a/components/engine/daemon/commit.go
+++ b/components/engine/daemon/commit.go
@@ -188,7 +188,7 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str
 		Config:          newConfig,
 		DiffID:          l.DiffID(),
 	}
-	config, err := json.Marshal(parent.NewChild(cc))
+	config, err := json.Marshal(image.NewChildImage(parent, cc))
 	if err != nil {
 		return "", err
 	}
diff --git a/components/engine/image/image.go b/components/engine/image/image.go
index 20650a8429..9f892d298a 100644
--- a/components/engine/image/image.go
+++ b/components/engine/image/image.go
@@ -4,14 +4,14 @@ import (
 	"encoding/json"
 	"errors"
 	"io"
+	"runtime"
+	"strings"
 	"time"
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/dockerversion"
 	"github.com/docker/docker/layer"
 	"github.com/opencontainers/go-digest"
-	"runtime"
-	"strings"
 )
 
 // ID is the content-addressable ID of an image.
@@ -125,10 +125,13 @@ type ChildConfig struct {
 	Config          *container.Config
 }
 
-// NewChild creates a new Image as a child of this image.
-func (img *Image) NewChild(child ChildConfig) *Image {
+// NewChildImage creates a new Image as a child of this image.
+func NewChildImage(img *Image, child ChildConfig) *Image {
 	isEmptyLayer := layer.IsEmpty(child.DiffID)
 	rootFS := img.RootFS
+	if rootFS == nil {
+		rootFS = NewRootFS()
+	}
 	if !isEmptyLayer {
 		rootFS.Append(child.DiffID)
 	}
diff --git a/components/engine/image/store.go b/components/engine/image/store.go
index 26ae109a06..17a3b7880a 100644
--- a/components/engine/image/store.go
+++ b/components/engine/image/store.go
@@ -2,7 +2,6 @@ package image
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"sync"
 
@@ -10,6 +9,7 @@ import (
 	"github.com/docker/distribution/digestset"
 	"github.com/docker/docker/layer"
 	"github.com/opencontainers/go-digest"
+	"github.com/pkg/errors"
 )
 
 // Store is an interface for creating and accessing images
@@ -147,7 +147,7 @@ func (is *store) Create(config []byte) (ID, error) {
 	if layerID != "" {
 		l, err = is.ls.Get(layerID)
 		if err != nil {
-			return "", err
+			return "", errors.Wrapf(err, "failed to get layer %s", layerID)
 		}
 	}
 
diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go
index 482de38b6c..b9a5dff789 100644
--- a/components/engine/integration-cli/docker_cli_build_test.go
+++ b/components/engine/integration-cli/docker_cli_build_test.go
@@ -5870,15 +5870,17 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFS(c *check.C) {
 	dockerfile := `
 		FROM busybox AS first
 		COPY foo bar
+
 		FROM busybox
-    %s
-    COPY baz baz
-    RUN echo mno > baz/cc
+		%s
+		COPY baz baz
+		RUN echo mno > baz/cc
+
 		FROM busybox
-    COPY bar /
-    COPY --from=1 baz sub/
-    COPY --from=0 bar baz
-    COPY --from=first bar bay`
+		COPY bar /
+		COPY --from=1 baz sub/
+		COPY --from=0 bar baz
+		COPY --from=first bar bay`
 
 	ctx := fakecontext.New(c, "",
 		fakecontext.WithDockerfile(fmt.Sprintf(dockerfile, "")),
@@ -5892,32 +5894,25 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFS(c *check.C) {
 
 	cli.BuildCmd(c, "build1", build.WithExternalBuildContext(ctx))
 
-	out := cli.DockerCmd(c, "run", "build1", "cat", "bar").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "def")
-	out = cli.DockerCmd(c, "run", "build1", "cat", "sub/aa").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "ghi")
-	out = cli.DockerCmd(c, "run", "build1", "cat", "sub/cc").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "mno")
-	out = cli.DockerCmd(c, "run", "build1", "cat", "baz").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "abc")
-	out = cli.DockerCmd(c, "run", "build1", "cat", "bay").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "abc")
+	cli.DockerCmd(c, "run", "build1", "cat", "bar").Assert(c, icmd.Expected{Out: "def"})
+	cli.DockerCmd(c, "run", "build1", "cat", "sub/aa").Assert(c, icmd.Expected{Out: "ghi"})
+	cli.DockerCmd(c, "run", "build1", "cat", "sub/cc").Assert(c, icmd.Expected{Out: "mno"})
+	cli.DockerCmd(c, "run", "build1", "cat", "baz").Assert(c, icmd.Expected{Out: "abc"})
+	cli.DockerCmd(c, "run", "build1", "cat", "bay").Assert(c, icmd.Expected{Out: "abc"})
 
 	result := cli.BuildCmd(c, "build2", build.WithExternalBuildContext(ctx))
 
 	// all commands should be cached
 	c.Assert(strings.Count(result.Combined(), "Using cache"), checker.Equals, 7)
+	c.Assert(getIDByName(c, "build1"), checker.Equals, getIDByName(c, "build2"))
 
 	err := ioutil.WriteFile(filepath.Join(ctx.Dir, "Dockerfile"), []byte(fmt.Sprintf(dockerfile, "COPY baz/aa foo")), 0644)
 	c.Assert(err, checker.IsNil)
 
 	// changing file in parent block should not affect last block
 	result = cli.BuildCmd(c, "build3", build.WithExternalBuildContext(ctx))
-
 	c.Assert(strings.Count(result.Combined(), "Using cache"), checker.Equals, 5)
 
-	c.Assert(getIDByName(c, "build1"), checker.Equals, getIDByName(c, "build2"))
-
 	err = ioutil.WriteFile(filepath.Join(ctx.Dir, "foo"), []byte("pqr"), 0644)
 	c.Assert(err, checker.IsNil)
 
@@ -5925,10 +5920,8 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFS(c *check.C) {
 	result = cli.BuildCmd(c, "build4", build.WithExternalBuildContext(ctx))
 	c.Assert(strings.Count(result.Combined(), "Using cache"), checker.Equals, 5)
 
-	out = cli.DockerCmd(c, "run", "build4", "cat", "bay").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "pqr")
-	out = cli.DockerCmd(c, "run", "build4", "cat", "baz").Combined()
-	c.Assert(strings.TrimSpace(out), check.Equals, "pqr")
+	cli.DockerCmd(c, "run", "build4", "cat", "bay").Assert(c, icmd.Expected{Out: "pqr"})
+	cli.DockerCmd(c, "run", "build4", "cat", "baz").Assert(c, icmd.Expected{Out: "pqr"})
 }
 
 func (s *DockerSuite) TestBuildCopyFromPreviousRootFSErrors(c *check.C) {
diff --git a/components/engine/pkg/testutil/tempfile/tempfile.go b/components/engine/pkg/testutil/tempfile/tempfile.go
index b375cb9561..01474babff 100644
--- a/components/engine/pkg/testutil/tempfile/tempfile.go
+++ b/components/engine/pkg/testutil/tempfile/tempfile.go
@@ -34,3 +34,23 @@ func (f *TempFile) Name() string {
 func (f *TempFile) Remove() {
 	os.Remove(f.Name())
 }
+
+// TempDir is a temporary directory that can be used with unit tests. TempDir
+// reduces the boilerplate setup required in each test case by handling
+// setup errors.
+type TempDir struct {
+	Path string
+}
+
+// NewTempDir returns a new temp file with contents
+func NewTempDir(t require.TestingT, prefix string) *TempDir {
+	path, err := ioutil.TempDir("", prefix+"-")
+	require.NoError(t, err)
+
+	return &TempDir{Path: path}
+}
+
+// Remove removes the file
+func (f *TempDir) Remove() {
+	os.Remove(f.Path)
+}