New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: add multi-stage build support #31257

Merged
merged 4 commits into from Mar 24, 2017

Conversation

@tonistiigi
Member

tonistiigi commented Feb 22, 2017

fixes #31067
fixes #31892
depends on #31236

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Feb 22, 2017

Member

@johnstep @jhowardmsft Do you know what may be the problem with windows test. TestBuildMultiStageArg passes so it shouldn't be completely broken.

Member

tonistiigi commented Feb 22, 2017

@johnstep @jhowardmsft Do you know what may be the problem with windows test. TestBuildMultiStageArg passes so it shouldn't be completely broken.

@tonistiigi tonistiigi changed the title from [wip] build: add multi-stage build support to build: add multi-stage build support Feb 22, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Mar 2, 2017

@anusha-ragunathan

This comment has been minimized.

Show comment
Hide comment
@anusha-ragunathan

anusha-ragunathan Mar 7, 2017

Contributor

I tested the PR while building a lean network plugin and it works quite nicely! Design and testing LGTM.

Contributor

anusha-ragunathan commented Mar 7, 2017

I tested the PR while building a lean network plugin and it works quite nicely! Design and testing LGTM.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Mar 8, 2017

Contributor

A slight variant/alternative mentioned here: #31067 (comment)

Contributor

duglin commented Mar 8, 2017

A slight variant/alternative mentioned here: #31067 (comment)

Show outdated Hide outdated builder/dockerfile/dispatchers.go
return err
}
b.context, err = builder.MakeDockerImageContext(b.docker, b.image, dir)

This comment has been minimized.

@tonistiigi

tonistiigi Mar 8, 2017

Member

note to self: make sure old context is closed

@tonistiigi

tonistiigi Mar 8, 2017

Member

note to self: make sure old context is closed

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 14, 2017

Member

I've not rebased this since #31236 needs to be merged before anyway.

Member

tonistiigi commented Mar 14, 2017

I've not rebased this since #31236 needs to be merged before anyway.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 16, 2017

Member

Based on #31067 (comment) I will reimplement this with COPY --context

Member

tonistiigi commented Mar 16, 2017

Based on #31067 (comment) I will reimplement this with COPY --context

@thaJeztah thaJeztah removed this from backlog in maintainers-session Mar 16, 2017

@simonferquel

This comment has been minimized.

Show comment
Hide comment
@simonferquel

simonferquel Mar 16, 2017

Contributor

I think we need to explicitly forbid building / (or c:\) and /windows (or c:\windows) out of a Windows image. That would make the image very large, and might even fail because Windows has a concept of "exclusive openness" of files, that might make some system files not copyable

Contributor

simonferquel commented Mar 16, 2017

I think we need to explicitly forbid building / (or c:\) and /windows (or c:\windows) out of a Windows image. That would make the image very large, and might even fail because Windows has a concept of "exclusive openness" of files, that might make some system files not copyable

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 18, 2017

Member

I have updated this PR to the new COPY --context=n syntax. Named addressing should be in a separate PR.

PTAL @dnephin @dmcgowan @simonferquel @philtay

Could I move it to code-review now as the design was discussed in the previous implementations and a decision was made in the favor of this?

Member

tonistiigi commented Mar 18, 2017

I have updated this PR to the new COPY --context=n syntax. Named addressing should be in a separate PR.

PTAL @dnephin @dmcgowan @simonferquel @philtay

Could I move it to code-review now as the design was discussed in the previous implementations and a decision was made in the favor of this?

@philtay

This comment has been minimized.

Show comment
Hide comment
@philtay

philtay Mar 18, 2017

Looks good. Thinking about the "zero case", I'm not sure if context=0 actually makes sense. Probably a simple COPY, without the context flag, should imply context=0. If we give to the initial context a number now (i.e. zero), we'll be forced to give it a name later for consistency (following named contexts PR). While it's easy (syntax-wise) to give the user the ability to name contexts from 1 onward, I don't see an elegant way to let them specify a name for the initial context.

Moreover it's debatable if a case like this one should really fail.

FROM busybox
COPY --context=0 foo bar

Even if we're clearly not in a nested build situation, the context=0 flag refers to the initial context. Thus, in theory, it should work. Omitting a number/name for the initial context would solve these little ambiguities and it feels (at least to me) more consistent with the actual behavior (i.e. you can COPY only from the initial context without having to specify a flag).

philtay commented Mar 18, 2017

Looks good. Thinking about the "zero case", I'm not sure if context=0 actually makes sense. Probably a simple COPY, without the context flag, should imply context=0. If we give to the initial context a number now (i.e. zero), we'll be forced to give it a name later for consistency (following named contexts PR). While it's easy (syntax-wise) to give the user the ability to name contexts from 1 onward, I don't see an elegant way to let them specify a name for the initial context.

Moreover it's debatable if a case like this one should really fail.

FROM busybox
COPY --context=0 foo bar

Even if we're clearly not in a nested build situation, the context=0 flag refers to the initial context. Thus, in theory, it should work. Omitting a number/name for the initial context would solve these little ambiguities and it feels (at least to me) more consistent with the actual behavior (i.e. you can COPY only from the initial context without having to specify a flag).

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 18, 2017

Member

@philtay The numbers mean the index of a build block that is defined by FROM. Accessing user files is different and doesn't require any flags. I think there is no confusion understanding that numbers start with 0. I'll change the error message to the case you pointed out so it clearly states that this copy would mean source and destination are already the same. After the naming is implemented in the next step the indexes would be minimally used anyway.

Member

tonistiigi commented Mar 18, 2017

@philtay The numbers mean the index of a build block that is defined by FROM. Accessing user files is different and doesn't require any flags. I think there is no confusion understanding that numbers start with 0. I'll change the error message to the case you pointed out so it clearly states that this copy would mean source and destination are already the same. After the naming is implemented in the next step the indexes would be minimally used anyway.

@philtay

This comment has been minimized.

Show comment
Hide comment
@philtay

philtay Mar 18, 2017

@tonistiigi I misunderstood the meaning of context=0, thanks for the explanation. LGTM.

philtay commented Mar 18, 2017

@tonistiigi I misunderstood the meaning of context=0, thanks for the explanation. LGTM.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Mar 18, 2017

Member

How about calling it --from-context for clarity?

Member

AkihiroSuda commented Mar 18, 2017

How about calling it --from-context for clarity?

tonistiigi added some commits Mar 16, 2017

Fix ARG scoping for Dockerfiles with multiple FROM
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Fix cache for dockerfiles with multiple FROM
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 20, 2017

Member

@AkihiroSuda That is bit too verbose to my taste. I'd rather prefer --from then but it may be confused with FROM.

I have updated this PR so it doesn't depend on any other PRs. PTAL.

Member

tonistiigi commented Mar 20, 2017

@AkihiroSuda That is bit too verbose to my taste. I'd rather prefer --from then but it may be confused with FROM.

I have updated this PR so it doesn't depend on any other PRs. PTAL.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 21, 2017

Member

Named contexts(#31067 (comment)) are implemented in tonistiigi/docker@nested-build...named-contexts . I'll open PR as soon as this gets merged.

Member

tonistiigi commented Mar 21, 2017

Named contexts(#31067 (comment)) are implemented in tonistiigi/docker@nested-build...named-contexts . I'll open PR as soon as this gets merged.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 23, 2017

Collaborator

LGTM

Collaborator

tiborvass commented Mar 23, 2017

LGTM

@dmcgowan

LGTM

@icecrime

LGTM

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 23, 2017

Member

I'll add docs in follow-up as the context naming support will follow this PR.

Member

tonistiigi commented Mar 23, 2017

I'll add docs in follow-up as the context naming support will follow this PR.

@tonistiigi tonistiigi merged commit 2fa8fe4 into moby:master Mar 24, 2017

5 of 6 checks passed

powerpc Jenkins build Docker-PRs-powerpc 743 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31997 has succeeded
Details
janky Jenkins build Docker-PRs 40620 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11696 has succeeded
Details
z Jenkins build Docker-PRs-s390x 606 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Mar 24, 2017

@StefanScherer

This comment has been minimized.

Show comment
Hide comment
@StefanScherer

StefanScherer Mar 24, 2017

Contributor

Great. I've seen the tweets today, but first didn't understand why people are so happy about it. After reading #31067 I understood the benefit.
Some other captains discussed about the COPY --from=0 ... as it looks very low level.
Coincidentally I've seen some slides about Rocker (https://github.com/grammarly/rocker#exportimport) and their approach how to copy artifacts from a previous layer.

FROM google/golang:1.4
ADD . /src
WORKDIR /src
RUN CGO_ENABLED=0 go build -a -installsuffix cgo -v -o rocker.o rocker.go
EXPORT rocker.o

FROM busybox
IMPORT rocker.o /bin/rocker
CMD ["/bin/rocker"]

As we haven't seen options to Dockerfile instructions I just wonder if the final syntax could more look like this?

Contributor

StefanScherer commented Mar 24, 2017

Great. I've seen the tweets today, but first didn't understand why people are so happy about it. After reading #31067 I understood the benefit.
Some other captains discussed about the COPY --from=0 ... as it looks very low level.
Coincidentally I've seen some slides about Rocker (https://github.com/grammarly/rocker#exportimport) and their approach how to copy artifacts from a previous layer.

FROM google/golang:1.4
ADD . /src
WORKDIR /src
RUN CGO_ENABLED=0 go build -a -installsuffix cgo -v -o rocker.o rocker.go
EXPORT rocker.o

FROM busybox
IMPORT rocker.o /bin/rocker
CMD ["/bin/rocker"]

As we haven't seen options to Dockerfile instructions I just wonder if the final syntax could more look like this?

@StefanScherer

This comment has been minimized.

Show comment
Hide comment
@StefanScherer

StefanScherer Mar 24, 2017

Contributor

Just seen #32063, much better ❤️

Contributor

StefanScherer commented Mar 24, 2017

Just seen #32063, much better ❤️

@dserodio

This comment has been minimized.

Show comment
Hide comment
@dserodio

dserodio Mar 28, 2017

I'm confused, what is the syntax that was merged in this PR?

dserodio commented Mar 28, 2017

I'm confused, what is the syntax that was merged in this PR?

@alexisvincent

This comment has been minimized.

Show comment
Hide comment
@alexisvincent

alexisvincent Jun 10, 2017

Is it possible to run the image from a particular stage?

For instance if I have build/dev tools in a "builder" stage. That I want to use interactively.

e.g.

docker run my-image --stage=builder some-useful-binary

alexisvincent commented Jun 10, 2017

Is it possible to run the image from a particular stage?

For instance if I have build/dev tools in a "builder" stage. That I want to use interactively.

e.g.

docker run my-image --stage=builder some-useful-binary
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jun 10, 2017

Member

--target=

Member

dnephin commented Jun 10, 2017

--target=

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment