From 05b8a1eb363ce03a9dfa3315fbac59c42af2df54 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Tue, 16 Sep 2014 09:58:20 -0700 Subject: [PATCH] Add support for copy/add with multiple src files Part one of solution for issue #6820 Signed-off-by: Doug Davis --- builder/dispatchers.go | 8 +- builder/internals.go | 166 ++++++++++++------ docs/man/Dockerfile.5.md | 13 +- docs/sources/reference/builder.md | 30 ++-- .../TestCopy/MultipleFiles/Dockerfile | 17 ++ .../TestCopy/MultipleFiles/test_file1 | 0 .../TestCopy/MultipleFiles/test_file2 | 0 .../TestCopy/MultipleFiles/test_file3 | 0 .../TestCopy/MultipleFiles/test_file4 | 0 .../TestCopy/MultipleFilesToFile/Dockerfile | 7 + .../TestCopy/MultipleFilesToFile/test_file1 | 0 .../TestCopy/MultipleFilesToFile/test_file2 | 0 .../TestCopy/MultipleFilesToFile/test_file3 | 0 integration-cli/docker_cli_build_test.go | 89 ++++++++++ 14 files changed, 253 insertions(+), 77 deletions(-) create mode 100644 integration-cli/build_tests/TestCopy/MultipleFiles/Dockerfile create mode 100644 integration-cli/build_tests/TestCopy/MultipleFiles/test_file1 create mode 100644 integration-cli/build_tests/TestCopy/MultipleFiles/test_file2 create mode 100644 integration-cli/build_tests/TestCopy/MultipleFiles/test_file3 create mode 100644 integration-cli/build_tests/TestCopy/MultipleFiles/test_file4 create mode 100644 integration-cli/build_tests/TestCopy/MultipleFilesToFile/Dockerfile create mode 100644 integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file1 create mode 100644 integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file2 create mode 100644 integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file3 diff --git a/builder/dispatchers.go b/builder/dispatchers.go index baaccf5204..54dbaa3c74 100644 --- a/builder/dispatchers.go +++ b/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 { - return fmt.Errorf("ADD requires two arguments") + if len(args) < 2 { + 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 { - return fmt.Errorf("COPY requires two arguments") + if len(args) < 2 { + return fmt.Errorf("COPY requires at least two arguments") } return b.runContextCommand(args, false, false, "COPY") diff --git a/builder/internals.go b/builder/internals.go index 3c3ad534c2..8d9a5a222d 100644 --- a/builder/internals.go +++ b/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 { - return fmt.Errorf("Invalid %s format", cmdName) + if len(args) < 2 { + return fmt.Errorf("Invalid %s format - at least two arguments required", cmdName) } - orig := args[0] - dest := args[1] + dest := args[len(args)-1] // last one is always the dest + + 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 := ©Infos[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, "/") { - u, err := url.Parse(orig) + if strings.HasSuffix(ci.destPath, "/") { + 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 - } else if fi, err := os.Stat(path.Join(b.contextPath, origPath)); err != nil { + ci.hashPath = remoteHash + } 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 { - origPath = origPath[1:] + if ci.origPath[0] == '/' && len(ci.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 { - hash = "file:" + fis.Sum() + if fis := sums.GetFile(ci.origPath); fis != nil { + 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 - } - 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 + ci.decompress = false } return nil } diff --git a/docs/man/Dockerfile.5.md b/docs/man/Dockerfile.5.md index c6a4e32fcf..4104dc2321 100644 --- a/docs/man/Dockerfile.5.md +++ b/docs/man/Dockerfile.5.md @@ -131,12 +131,13 @@ or interactively, as with the following command: **docker run -t -i image bash** **ADD** - --**ADD ** The ADD instruction copies new files from and adds them - to the filesystem of the container at path . must be the path to a - file or directory relative to the source directory that is being built (the - context of the build) or a remote file URL. is the absolute path to - which the source is copied inside the target container. All new files and - directories are created with mode 0755, with uid and gid 0. + --**ADD ... ** The ADD instruction copies new files, directories + or remote file URLs to the filesystem of the container at path . + Mutliple resources may be specified but if they are files or directories + then they must be relative to the source directory that is being built + (the context of the build). is the absolute path to + 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"] diff --git a/docs/sources/reference/builder.md b/docs/sources/reference/builder.md index 4b16e0365c..1f5bd4dada 100644 --- a/docs/sources/reference/builder.md +++ b/docs/sources/reference/builder.md @@ -284,13 +284,15 @@ change them using `docker run --env =`. ## ADD - ADD + ADD ... -The `ADD` instruction will copy new files from `` and add them to the -container's filesystem at path ``. +The `ADD` instruction copies new files,directories or remote file URLs to +the filesystem of the container from `` and add them to the at +path ``. -`` must be the path to a file or directory relative to the source directory -being built (also called the *context* of the build) or a remote file URL. +Multiple resource may be specified but if they are files or +directories then they must be relative to the source directory that is +being built (the context of the build). `` 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 `` will be written at `/base()`. +- If multiple `` resources are specified then `` must be a + directory, and it must end with a slash `/`. + - If `` does not end with a trailing slash, it will be considered a regular file and the contents of `` will be written at ``. @@ -361,13 +366,15 @@ The copy obeys the following rules: ## COPY - COPY + COPY ... -The `COPY` instruction will copy new files from `` and add them to the -container's filesystem at path ``. +The `COPY` instruction copies new files,directories or remote file URLs to +the filesystem of the container from `` and add them to the at +path ``. -`` must be the path to a file or directory relative to the source directory -being built (also called the *context* of the build). +Multiple resource may be specified but if they are files or +directories then they must be relative to the source directory that is being +built (the context of the build). `` 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 `` will be written at `/base()`. +- If multiple `` resources are specified then `` must be a + directory, and it must end with a slash `/`. + - If `` does not end with a trailing slash, it will be considered a regular file and the contents of `` will be written at ``. diff --git a/integration-cli/build_tests/TestCopy/MultipleFiles/Dockerfile b/integration-cli/build_tests/TestCopy/MultipleFiles/Dockerfile new file mode 100644 index 0000000000..4143e65962 --- /dev/null +++ b/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' ] diff --git a/integration-cli/build_tests/TestCopy/MultipleFiles/test_file1 b/integration-cli/build_tests/TestCopy/MultipleFiles/test_file1 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integration-cli/build_tests/TestCopy/MultipleFiles/test_file2 b/integration-cli/build_tests/TestCopy/MultipleFiles/test_file2 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integration-cli/build_tests/TestCopy/MultipleFiles/test_file3 b/integration-cli/build_tests/TestCopy/MultipleFiles/test_file3 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integration-cli/build_tests/TestCopy/MultipleFiles/test_file4 b/integration-cli/build_tests/TestCopy/MultipleFiles/test_file4 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integration-cli/build_tests/TestCopy/MultipleFilesToFile/Dockerfile b/integration-cli/build_tests/TestCopy/MultipleFilesToFile/Dockerfile new file mode 100644 index 0000000000..520d356c72 --- /dev/null +++ b/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 diff --git a/integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file1 b/integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file1 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file2 b/integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file2 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file3 b/integration-cli/build_tests/TestCopy/MultipleFilesToFile/test_file3 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index d9885aad0f..1c25de6e48 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/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)