diff --git a/builder/builder.go b/builder/builder.go index 785f0eb3c3..ccc43f6cc2 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -54,8 +54,6 @@ type Backend interface { ContainerStart(containerID string, hostConfig *container.HostConfig, checkpoint string, checkpointDir string) error // ContainerWait stops processing until the given container is stopped. ContainerWait(containerID string, timeout time.Duration) (int, error) - // ContainerUpdateCmdOnBuild updates container.Path and container.Args - ContainerUpdateCmdOnBuild(containerID string, cmd []string) error // ContainerCreateWorkdir creates the workdir ContainerCreateWorkdir(containerID string) error diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 715161270d..0b76f2ff8e 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -315,10 +315,6 @@ func workdir(req dispatchRequest) error { return nil } - // TODO: why is this done here. This seems to be done at random places all over - // the builder - req.runConfig.Image = req.builder.image - comment := "WORKDIR " + req.runConfig.WorkingDir runConfigWithCommentCmd := copyRunConfig(req.runConfig, withCmdCommentString(comment)) if hit, err := req.builder.probeCache(req.builder.image, runConfigWithCommentCmd); err != nil || hit { @@ -372,10 +368,9 @@ func run(req dispatchRequest) error { saveCmd = prependEnvOnCmd(req.builder.buildArgs, buildArgs, cmdFromArgs) } - // TODO: this was previously in b.create(), why is it necessary? - req.runConfig.Image = req.builder.image - - runConfigForCacheProbe := copyRunConfig(req.runConfig, withCmd(saveCmd)) + runConfigForCacheProbe := copyRunConfig(req.runConfig, + withCmd(saveCmd), + withEntrypointOverride(saveCmd, nil)) hit, err := req.builder.probeCache(req.builder.image, runConfigForCacheProbe) if err != nil || hit { return err @@ -383,13 +378,13 @@ func run(req dispatchRequest) error { runConfig := copyRunConfig(req.runConfig, withCmd(cmdFromArgs), - withEnv(append(req.runConfig.Env, buildArgs...))) + withEnv(append(req.runConfig.Env, buildArgs...)), + withEntrypointOverride(saveCmd, strslice.StrSlice{""})) // set config as already being escaped, this prevents double escaping on windows runConfig.ArgsEscaped = true logrus.Debugf("[BUILDER] Command to be executed: %v", runConfig.Cmd) - cID, err := req.builder.create(runConfig) if err != nil { return err diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index 72a82dbd91..0dd3894348 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -448,6 +448,7 @@ func TestRunWithBuildArgs(t *testing.T) { getCacheFunc: func(parentID string, cfg *container.Config) (string, error) { // Check the runConfig.Cmd sent to probeCache() assert.Equal(t, cachedCmd, cfg.Cmd) + assert.Equal(t, strslice.StrSlice(nil), cfg.Entrypoint) return "", nil }, } @@ -462,12 +463,14 @@ func TestRunWithBuildArgs(t *testing.T) { // Check the runConfig.Cmd sent to create() assert.Equal(t, cmdWithShell, config.Config.Cmd) assert.Contains(t, config.Config.Env, "one=two") + assert.Equal(t, strslice.StrSlice{""}, config.Config.Entrypoint) return container.ContainerCreateCreatedBody{ID: "12345"}, nil } mockBackend.commitFunc = func(cID string, cfg *backend.ContainerCommitConfig) (string, error) { // Check the runConfig.Cmd sent to commit() assert.Equal(t, origCmd, cfg.Config.Cmd) assert.Equal(t, cachedCmd, cfg.ContainerConfig.Cmd) + assert.Equal(t, strslice.StrSlice(nil), cfg.Config.Entrypoint) return "", nil } diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index abad15e096..e98a4e2f0d 100644 --- a/builder/dockerfile/evaluator.go +++ b/builder/dockerfile/evaluator.go @@ -173,7 +173,14 @@ func (b *Builder) dispatch(stepN int, stepTotal int, node *parser.Node, shlex *S // XXX yes, we skip any cmds that are not valid; the parser should have // picked these out already. if f, ok := evaluateTable[cmd]; ok { - return f(newDispatchRequestFromNode(node, b, strList, shlex)) + if err := f(newDispatchRequestFromNode(node, b, strList, shlex)); err != nil { + return err + } + // TODO: return an object instead of setting things on builder + // If the step created a new image set it as the imageID for the + // current runConfig + b.runConfig.Image = b.image + return nil } return fmt.Errorf("Unknown instruction: %s", upperCasedCmd) diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 3ee3ab30a8..bfe8c8ff89 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -42,8 +42,6 @@ func (b *Builder) commit(comment string) error { if !b.hasFromImage() { return errors.New("Please provide a source image with `from` prior to commit") } - // TODO: why is this set here? - b.runConfig.Image = b.image runConfigWithCommentCmd := copyRunConfig(b.runConfig, withCmdComment(comment)) hit, err := b.probeCache(b.image, runConfigWithCommentCmd) @@ -67,7 +65,8 @@ func (b *Builder) commitContainer(id string, containerConfig *container.Config) ContainerCommitConfig: types.ContainerCommitConfig{ Author: b.maintainer, Pause: true, - Config: b.runConfig, + // TODO: this should be done by Commit() + Config: copyRunConfig(b.runConfig), }, ContainerConfig: containerConfig, } @@ -100,10 +99,6 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD // Work in daemon-specific filepath semantics dest := filepath.FromSlash(args[len(args)-1]) // last one is always the dest - // TODO: why is this done here. This seems to be done at random places all over - // the builder - b.runConfig.Image = b.image - var infos []copyInfo // Loop through each src file and calculate the info we need to @@ -239,6 +234,21 @@ func withEnv(env []string) runConfigModifier { } } +// withEntrypointOverride sets an entrypoint on runConfig if the command is +// not empty. The entrypoint is left unmodified if command is empty. +// +// The dockerfile RUN instruction expect to run without an entrypoint +// so the runConfig entrypoint needs to be modified accordingly. ContainerCreate +// will change a []string{""} entrypoint to nil, so we probe the cache with the +// nil entrypoint. +func withEntrypointOverride(cmd []string, entrypoint []string) runConfigModifier { + return func(runConfig *container.Config) { + if len(cmd) > 0 { + runConfig.Entrypoint = entrypoint + } + } +} + // getShell is a helper function which gets the right shell for prefixing the // shell-form of RUN, ENTRYPOINT and CMD instructions func getShell(c *container.Config) []string { @@ -541,12 +551,12 @@ func (b *Builder) processImageFrom(img builder.Image) error { // If an image is found, probeCache returns `(true, nil)`. // If no image is found, it returns `(false, nil)`. // If there is any error, it returns `(false, err)`. -func (b *Builder) probeCache(imageID string, runConfig *container.Config) (bool, error) { +func (b *Builder) probeCache(parentID string, runConfig *container.Config) (bool, error) { c := b.imageCache if c == nil || b.options.NoCache || b.cacheBusted { return false, nil } - cache, err := c.GetCache(imageID, runConfig) + cache, err := c.GetCache(parentID, runConfig) if err != nil { return false, err } @@ -606,12 +616,6 @@ func (b *Builder) create(runConfig *container.Config) (string, error) { b.tmpContainers[c.ID] = struct{}{} fmt.Fprintf(b.Stdout, " ---> Running in %s\n", stringid.TruncateID(c.ID)) - - // override the entry point that may have been picked up from the base image - if err := b.docker.ContainerUpdateCmdOnBuild(c.ID, runConfig.Cmd); err != nil { - return "", err - } - return c.ID, nil } diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index e8647f426c..bdd198ccfe 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -73,10 +73,6 @@ func (m *MockBackend) ContainerWait(containerID string, timeout time.Duration) ( return 0, nil } -func (m *MockBackend) ContainerUpdateCmdOnBuild(containerID string, cmd []string) error { - return nil -} - func (m *MockBackend) ContainerCreateWorkdir(containerID string) error { return nil } diff --git a/daemon/update.go b/daemon/update.go index 6e26eeb96a..76e4a3f93f 100644 --- a/daemon/update.go +++ b/daemon/update.go @@ -22,20 +22,6 @@ func (daemon *Daemon) ContainerUpdate(name string, hostConfig *container.HostCon return container.ContainerUpdateOKBody{Warnings: warnings}, nil } -// ContainerUpdateCmdOnBuild updates Path and Args for the container with ID cID. -func (daemon *Daemon) ContainerUpdateCmdOnBuild(cID string, cmd []string) error { - if len(cmd) == 0 { - return nil - } - c, err := daemon.GetContainer(cID) - if err != nil { - return err - } - c.Path = cmd[0] - c.Args = cmd[1:] - return nil -} - func (daemon *Daemon) update(name string, hostConfig *container.HostConfig) error { if hostConfig == nil { return nil diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 5ee64cbe4b..701572908b 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -337,13 +337,13 @@ func (s *DockerSuite) TestBuildOnBuildCmdEntrypointJSON(c *check.C) { name1 := "onbuildcmd" name2 := "onbuildgenerated" - buildImageSuccessfully(c, name1, build.WithDockerfile(` + cli.BuildCmd(c, name1, build.WithDockerfile(` FROM busybox ONBUILD CMD ["hello world"] ONBUILD ENTRYPOINT ["echo"] ONBUILD RUN ["true"]`)) - buildImageSuccessfully(c, name2, build.WithDockerfile(fmt.Sprintf(`FROM %s`, name1))) + cli.BuildCmd(c, name2, build.WithDockerfile(fmt.Sprintf(`FROM %s`, name1))) result := cli.DockerCmd(c, "run", name2) result.Assert(c, icmd.Expected{Out: "hello world"}) @@ -1785,11 +1785,17 @@ func (s *DockerSuite) TestBuildConditionalCache(c *check.C) { } } -// FIXME(vdemeester) this really seems to test the same thing as before func (s *DockerSuite) TestBuildAddMultipleLocalFileWithAndWithoutCache(c *check.C) { name := "testbuildaddmultiplelocalfilewithcache" - dockerfile := ` + baseName := name + "-base" + + cli.BuildCmd(c, baseName, build.WithDockerfile(` FROM busybox + ENTRYPOINT ["/bin/sh"] + `)) + + dockerfile := ` + FROM testbuildaddmultiplelocalfilewithcache-base MAINTAINER dockerio ADD foo Dockerfile /usr/lib/bla/ RUN sh -c "[ $(cat /usr/lib/bla/foo) = "hello" ]"` @@ -1799,15 +1805,15 @@ func (s *DockerSuite) TestBuildAddMultipleLocalFileWithAndWithoutCache(c *check. defer ctx.Close() cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx)) id1 := getIDByName(c, name) - cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx)) + result2 := cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx)) id2 := getIDByName(c, name) - cli.BuildCmd(c, name, build.WithoutCache, build.WithExternalBuildContext(ctx)) + result3 := cli.BuildCmd(c, name, build.WithoutCache, build.WithExternalBuildContext(ctx)) id3 := getIDByName(c, name) if id1 != id2 { - c.Fatal("The cache should have been used but hasn't.") + c.Fatalf("The cache should have been used but hasn't: %s", result2.Stdout()) } if id1 == id3 { - c.Fatal("The cache should have been invalided but hasn't.") + c.Fatalf("The cache should have been invalided but hasn't: %s", result3.Stdout()) } }