Add support for copy/add with multiple src files

Part one of solution for issue #6820

Signed-off-by: Doug Davis <dug@us.ibm.com>

builder/dispatchers.go

@@ -65,8 +65,8 @@ func maintainer(b *Builder, args []string, attributes map[string]bool) error {
 // exist here. If you do not wish to have this automatic handling, use COPY.
 //
 func add(b *Builder, args []string, attributes map[string]bool) error {
-	if len(args) != 2 {
+	if len(args) < 2 {
-		return fmt.Errorf("ADD requires two arguments")
+		return fmt.Errorf("ADD requires at least two arguments")
 	}
 
 	return b.runContextCommand(args, true, true, "ADD")
@@ -77,8 +77,8 @@ func add(b *Builder, args []string, attributes map[string]bool) error {
 // Same as 'ADD' but without the tar and remote url handling.
 //
 func dispatchCopy(b *Builder, args []string, attributes map[string]bool) error {
-	if len(args) != 2 {
+	if len(args) < 2 {
-		return fmt.Errorf("COPY requires two arguments")
+		return fmt.Errorf("COPY requires at least two arguments")
 	}
 
 	return b.runContextCommand(args, false, false, "COPY")

builder/internals.go

@@ -99,37 +99,117 @@ func (b *Builder) commit(id string, autoCmd []string, comment string) error {
 	return nil
 }
 
+type copyInfo struct {
+	origPath   string
+	destPath   string
+	hashPath   string
+	decompress bool
+	tmpDir     string
+}
+
 func (b *Builder) runContextCommand(args []string, allowRemote bool, allowDecompression bool, cmdName string) error {
 	if b.context == nil {
 		return fmt.Errorf("No context given. Impossible to use %s", cmdName)
 	}
 
-	if len(args) != 2 {
+	if len(args) < 2 {
-		return fmt.Errorf("Invalid %s format", cmdName)
+		return fmt.Errorf("Invalid %s format - at least two arguments required", cmdName)
 	}
 
-	orig := args[0]
+	dest := args[len(args)-1] // last one is always the dest
-	dest := args[1]
+
+	if len(args) > 2 && dest[len(dest)-1] != '/' {
+		return fmt.Errorf("When using %s with more than one source file, the destination must be a directory and end with a /", cmdName)
+	}
+
+	copyInfos := make([]copyInfo, len(args)-1)
+	hasHash := false
+	srcPaths := ""
+	origPaths := ""
 
-	cmd := b.Config.Cmd
-	b.Config.Cmd = []string{"/bin/sh", "-c", fmt.Sprintf("#(nop) %s %s in %s", cmdName, orig, dest)}
-	defer func(cmd []string) { b.Config.Cmd = cmd }(cmd)
 	b.Config.Image = b.image
 
+	defer func() {
+		for _, ci := range copyInfos {
+			if ci.tmpDir != "" {
+				os.RemoveAll(ci.tmpDir)
+			}
+		}
+	}()
+
+	// Loop through each src file and calculate the info we need to
+	// do the copy (e.g. hash value if cached).  Don't actually do
+	// the copy until we've looked at all src files
+	for i, orig := range args[0 : len(args)-1] {
+		ci := &copyInfos[i]
+		ci.origPath = orig
+		ci.destPath = dest
+		ci.decompress = true
+
+		err := calcCopyInfo(b, cmdName, ci, allowRemote, allowDecompression)
+		if err != nil {
+			return err
+		}
+
+		origPaths += " " + ci.origPath // will have leading space
+		if ci.hashPath == "" {
+			srcPaths += " " + ci.origPath // note leading space
+		} else {
+			srcPaths += " " + ci.hashPath // note leading space
+			hasHash = true
+		}
+	}
+
+	cmd := b.Config.Cmd
+	b.Config.Cmd = []string{"/bin/sh", "-c", fmt.Sprintf("#(nop) %s%s in %s", cmdName, srcPaths, dest)}
+	defer func(cmd []string) { b.Config.Cmd = cmd }(cmd)
+
+	hit, err := b.probeCache()
+	if err != nil {
+		return err
+	}
+	// If we do not have at least one hash, never use the cache
+	if hit && hasHash {
+		return nil
+	}
+
+	container, _, err := b.Daemon.Create(b.Config, "")
+	if err != nil {
+		return err
+	}
+	b.TmpContainers[container.ID] = struct{}{}
+
+	if err := container.Mount(); err != nil {
+		return err
+	}
+	defer container.Unmount()
+
+	for _, ci := range copyInfos {
+		if err := b.addContext(container, ci.origPath, ci.destPath, ci.decompress); err != nil {
+			return err
+		}
+	}
+
+	if err := b.commit(container.ID, cmd, fmt.Sprintf("%s%s in %s", cmdName, origPaths, dest)); err != nil {
+		return err
+	}
+	return nil
+}
+
+func calcCopyInfo(b *Builder, cmdName string, ci *copyInfo, allowRemote bool, allowDecompression bool) error {
 	var (
-		origPath   = orig
-		destPath   = dest
 		remoteHash string
 		isRemote   bool
-		decompress = true
 	)
 
-	isRemote = utils.IsURL(orig)
+	saveOrig := ci.origPath
+	isRemote = utils.IsURL(ci.origPath)
+
 	if isRemote && !allowRemote {
 		return fmt.Errorf("Source can't be an URL for %s", cmdName)
-	} else if utils.IsURL(orig) {
+	} else if isRemote {
 		// Initiate the download
-		resp, err := utils.Download(orig)
+		resp, err := utils.Download(ci.origPath)
 		if err != nil {
 			return err
 		}
@@ -139,6 +219,7 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowDecomp
 		if err != nil {
 			return err
 		}
+		ci.tmpDir = tmpDirName
 
 		// Create a tmp file within our tmp dir
 		tmpFileName := path.Join(tmpDirName, "tmp")
@@ -146,7 +227,6 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowDecomp
 		if err != nil {
 			return err
 		}
-		defer os.RemoveAll(tmpDirName)
 
 		// Download and dump result to tmp file
 		if _, err := io.Copy(tmpFile, utils.ProgressReader(resp.Body, int(resp.ContentLength), b.OutOld, b.StreamFormatter, true, "", "Downloading")); err != nil {
@@ -161,7 +241,7 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowDecomp
 			return err
 		}
 
-		origPath = path.Join(filepath.Base(tmpDirName), filepath.Base(tmpFileName))
+		ci.origPath = path.Join(filepath.Base(tmpDirName), filepath.Base(tmpFileName))
 
 		// Process the checksum
 		r, err := archive.Tar(tmpFileName, archive.Uncompressed)
@@ -179,8 +259,8 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowDecomp
 		r.Close()
 
 		// If the destination is a directory, figure out the filename.
-		if strings.HasSuffix(dest, "/") {
+		if strings.HasSuffix(ci.destPath, "/") {
-			u, err := url.Parse(orig)
+			u, err := url.Parse(saveOrig)
 			if err != nil {
 				return err
 			}
@@ -193,30 +273,29 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowDecomp
 			if filename == "" {
 				return fmt.Errorf("cannot determine filename from url: %s", u)
 			}
-			destPath = dest + filename
+			ci.destPath = ci.destPath + filename
 		}
 	}
 
-	if err := b.checkPathForAddition(origPath); err != nil {
+	if err := b.checkPathForAddition(ci.origPath); err != nil {
 		return err
 	}
 
 	// Hash path and check the cache
 	if b.UtilizeCache {
 		var (
-			hash string
 			sums = b.context.GetSums()
 		)
 
 		if remoteHash != "" {
-			hash = remoteHash
+			ci.hashPath = remoteHash
-		} else if fi, err := os.Stat(path.Join(b.contextPath, origPath)); err != nil {
+		} else if fi, err := os.Stat(path.Join(b.contextPath, ci.origPath)); err != nil {
 			return err
 		} else if fi.IsDir() {
 			var subfiles []string
 			for _, fileInfo := range sums {
 				absFile := path.Join(b.contextPath, fileInfo.Name())
-				absOrigPath := path.Join(b.contextPath, origPath)
+				absOrigPath := path.Join(b.contextPath, ci.origPath)
 				if strings.HasPrefix(absFile, absOrigPath) {
 					subfiles = append(subfiles, fileInfo.Sum())
 				}
@@ -224,49 +303,22 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowDecomp
 			sort.Strings(subfiles)
 			hasher := sha256.New()
 			hasher.Write([]byte(strings.Join(subfiles, ",")))
-			hash = "dir:" + hex.EncodeToString(hasher.Sum(nil))
+			ci.hashPath = "dir:" + hex.EncodeToString(hasher.Sum(nil))
 		} else {
-			if origPath[0] == '/' && len(origPath) > 1 {
+			if ci.origPath[0] == '/' && len(ci.origPath) > 1 {
-				origPath = origPath[1:]
+				ci.origPath = ci.origPath[1:]
 			}
-			origPath = strings.TrimPrefix(origPath, "./")
+			ci.origPath = strings.TrimPrefix(ci.origPath, "./")
 			// This will match on the first file in sums of the archive
-			if fis := sums.GetFile(origPath); fis != nil {
+			if fis := sums.GetFile(ci.origPath); fis != nil {
-				hash = "file:" + fis.Sum()
+				ci.hashPath = "file:" + fis.Sum()
 			}
 		}
-		b.Config.Cmd = []string{"/bin/sh", "-c", fmt.Sprintf("#(nop) %s %s in %s", cmdName, hash, dest)}
-		hit, err := b.probeCache()
-		if err != nil {
-			return err
-		}
-		// If we do not have a hash, never use the cache
-		if hit && hash != "" {
-			return nil
-		}
-	}
 
-	// Create the container
-	container, _, err := b.Daemon.Create(b.Config, "")
-	if err != nil {
-		return err
 	}
-	b.TmpContainers[container.ID] = struct{}{}
-
-	if err := container.Mount(); err != nil {
-		return err
-	}
-	defer container.Unmount()
 
 	if !allowDecompression || isRemote {
-		decompress = false
+		ci.decompress = false
-	}
-	if err := b.addContext(container, origPath, destPath, decompress); err != nil {
-		return err
-	}
-
-	if err := b.commit(container.ID, cmd, fmt.Sprintf("%s %s in %s", cmdName, orig, dest)); err != nil {
-		return err
 	}
 	return nil
 }

docs/man/Dockerfile.5.md

@@ -131,12 +131,13 @@ or
  interactively, as with the following command: **docker run -t -i image bash**
 
 **ADD**
- --**ADD <src> <dest>** The ADD instruction copies new files from <src> and adds them
+ --**ADD <src>... <dest>** The ADD instruction copies new files, directories
-  to the filesystem of the container at path <dest>.  <src> must be the path to a
+ or remote file URLs to the filesystem of the container at path <dest>.  
-  file or directory relative to the source directory that is being built (the
+ Mutliple <src> resources may be specified but if they are files or directories
-  context of the build) or a remote file URL.  <dest> is the absolute path to
+ then they must be relative to the source directory that is being built 
-  which the source is copied inside the target container.  All new files and
+ (the context of the build).  <dest> is the absolute path to
-  directories are created with mode 0755, with uid and gid 0.
+ which the source is copied inside the target container.  All new files and
+ directories are created with mode 0755, with uid and gid 0.
 
 **ENTRYPOINT**
  --**ENTRYPOINT** has two forms: ENTRYPOINT ["executable", "param1", "param2"]

docs/sources/reference/builder.md

@@ -284,13 +284,15 @@ change them using `docker run --env <key>=<value>`.
 
 ## ADD
 
-    ADD <src> <dest>
+    ADD <src>... <dest>
 
-The `ADD` instruction will copy new files from `<src>` and add them to the
+The `ADD` instruction copies new files,directories or remote file URLs to 
-container's filesystem at path `<dest>`.
+the filesystem of the container  from `<src>` and add them to the at 
+path `<dest>`.  
 
-`<src>` must be the path to a file or directory relative to the source directory
+Multiple <src> resource may be specified but if they are files or 
-being built (also called the *context* of the build) or a remote file URL.
+directories then they must be relative to the source directory that is 
+being built (the context of the build).
 
 `<dest>` is the absolute path to which the source will be copied inside the
 destination container.
@@ -353,6 +355,9 @@ The copy obeys the following rules:
   will be considered a directory and the contents of `<src>` will be written
   at `<dest>/base(<src>)`.
 
+- If multiple `<src>` resources are specified then `<dest>` must be a
+  directory, and it must end with a slash `/`.
+
 - If `<dest>` does not end with a trailing slash, it will be considered a
   regular file and the contents of `<src>` will be written at `<dest>`.
 
@@ -361,13 +366,15 @@ The copy obeys the following rules:
 
 ## COPY
 
-    COPY <src> <dest>
+    COPY <src>... <dest>
 
-The `COPY` instruction will copy new files from `<src>` and add them to the
+The `COPY` instruction copies new files,directories or remote file URLs to 
-container's filesystem at path `<dest>`.
+the filesystem of the container  from `<src>` and add them to the at 
+path `<dest>`. 
 
-`<src>` must be the path to a file or directory relative to the source directory
+Multiple <src> resource may be specified but if they are files or 
-being built (also called the *context* of the build).
+directories then they must be relative to the source directory that is being 
+built (the context of the build).
 
 `<dest>` is the absolute path to which the source will be copied inside the
 destination container.
@@ -393,6 +400,9 @@ The copy obeys the following rules:
   will be considered a directory and the contents of `<src>` will be written
   at `<dest>/base(<src>)`.
 
+- If multiple `<src>` resources are specified then `<dest>` must be a
+  directory, and it must end with a slash `/`.
+
 - If `<dest>` does not end with a trailing slash, it will be considered a
   regular file and the contents of `<src>` will be written at `<dest>`.
 

integration-cli/build_tests/TestCopy/MultipleFiles/Dockerfile

@@ -0,0 +1,17 @@
+FROM busybox
+RUN echo 'dockerio:x:1001:1001::/bin:/bin/false' >> /etc/passwd
+RUN echo 'dockerio:x:1001:' >> /etc/group
+RUN mkdir /exists
+RUN touch /exists/exists_file
+RUN chown -R dockerio.dockerio /exists
+COPY test_file1 test_file2 /exists/
+ADD test_file3 test_file4 https://docker.com/robots.txt /exists/
+RUN [ $(ls -l / | grep exists | awk '{print $3":"$4}') = 'dockerio:dockerio' ]
+RUN [ $(ls -l /exists/test_file1 | awk '{print $3":"$4}') = 'root:root' ]
+RUN [ $(ls -l /exists/test_file2 | awk '{print $3":"$4}') = 'root:root' ]
+
+RUN [ $(ls -l /exists/test_file3 | awk '{print $3":"$4}') = 'root:root' ]
+RUN [ $(ls -l /exists/test_file4 | awk '{print $3":"$4}') = 'root:root' ]
+RUN [ $(ls -l /exists/robots.txt | awk '{print $3":"$4}') = 'root:root' ]
+
+RUN [ $(ls -l /exists/exists_file | awk '{print $3":"$4}') = 'dockerio:dockerio' ]

integration-cli/build_tests/TestCopy/MultipleFilesToFile/Dockerfile

@@ -0,0 +1,7 @@
+FROM busybox
+RUN echo 'dockerio:x:1001:1001::/bin:/bin/false' >> /etc/passwd
+RUN echo 'dockerio:x:1001:' >> /etc/group
+RUN mkdir /exists
+RUN chown -R dockerio.dockerio /exists
+COPY test_file1 /exists/
+ADD test_file2 test_file3 /exists/test_file1

integration-cli/docker_cli_build_test.go

@@ -148,6 +148,66 @@ func TestAddSingleFileToExistDir(t *testing.T) {
 	logDone("build - add single file to existing dir")
 }
 
+func TestMultipleFiles(t *testing.T) {
+	buildDirectory := filepath.Join(workingDirectory, "build_tests", "TestCopy")
+	out, exitCode, err := dockerCmdInDir(t, buildDirectory, "build", "-t", "testaddimg", "MultipleFiles")
+	errorOut(err, t, fmt.Sprintf("build failed to complete: %v %v", out, err))
+
+	if err != nil || exitCode != 0 {
+		t.Fatal("failed to build the image")
+	}
+
+	deleteImages("testaddimg")
+
+	logDone("build - mulitple file copy/add tests")
+}
+
+func TestAddMultipleFilesToFile(t *testing.T) {
+	name := "testaddmultiplefilestofile"
+	defer deleteImages(name)
+	ctx, err := fakeContext(`FROM scratch
+	ADD file1.txt file2.txt test
+        `,
+		map[string]string{
+			"file1.txt": "test1",
+			"file2.txt": "test1",
+		})
+	defer ctx.Close()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expected := "When using ADD with more than one source file, the destination must be a directory and end with a /"
+	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
+		t.Fatalf("Wrong error: (should contain \"%s\") got:\n%v", expected, err)
+	}
+
+	logDone("build - multiple add files to file")
+}
+
+func TestCopyMultipleFilesToFile(t *testing.T) {
+	name := "testcopymultiplefilestofile"
+	defer deleteImages(name)
+	ctx, err := fakeContext(`FROM scratch
+	COPY file1.txt file2.txt test
+        `,
+		map[string]string{
+			"file1.txt": "test1",
+			"file2.txt": "test1",
+		})
+	defer ctx.Close()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expected := "When using COPY with more than one source file, the destination must be a directory and end with a /"
+	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
+		t.Fatalf("Wrong error: (should contain \"%s\") got:\n%v", expected, err)
+	}
+
+	logDone("build - multiple copy files to file")
+}
+
 func TestAddSingleFileToNonExistDir(t *testing.T) {
 	buildDirectory := filepath.Join(workingDirectory, "build_tests", "TestAdd")
 	out, exitCode, err := dockerCmdInDir(t, buildDirectory, "build", "-t", "testaddimg", "SingleFileToNonExistDir")
@@ -1059,6 +1119,35 @@ func TestBuildADDLocalFileWithCache(t *testing.T) {
 	logDone("build - add local file with cache")
 }
 
+func TestBuildADDMultipleLocalFileWithCache(t *testing.T) {
+	name := "testbuildaddmultiplelocalfilewithcache"
+	defer deleteImages(name)
+	dockerfile := `
+		FROM busybox
+        MAINTAINER dockerio
+        ADD foo Dockerfile /usr/lib/bla/
+		RUN [ "$(cat /usr/lib/bla/foo)" = "hello" ]`
+	ctx, err := fakeContext(dockerfile, map[string]string{
+		"foo": "hello",
+	})
+	defer ctx.Close()
+	if err != nil {
+		t.Fatal(err)
+	}
+	id1, err := buildImageFromContext(name, ctx, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	id2, err := buildImageFromContext(name, ctx, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if id1 != id2 {
+		t.Fatal("The cache should have been used but hasn't.")
+	}
+	logDone("build - add multiple local files with cache")
+}
+
 func TestBuildADDLocalFileWithoutCache(t *testing.T) {
 	name := "testbuildaddlocalfilewithoutcache"
 	defer deleteImages(name)