Skip to content
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.

Sign up for GitHub

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

Jump to bottom

dockerfile: expose TARGETSTAGE as builtin argument #5431

Merged
merged 1 commit into from
Oct 28, 2024
Merged

dockerfile: expose TARGETSTAGE as builtin argument #5431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,10 +227,6 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
opt.LLBCaps = &caps
}

platformOpt := buildPlatformOpt(&opt)

globalArgs := platformArgs(platformOpt, opt.BuildArgs)

dockerfile, err := parser.Parse(bytes.NewReader(dt))
if err != nil {
return nil, err
Expand Down Expand Up @@ -261,6 +257,13 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
validateStageNames(stages, lint)
validateCommandCasing(stages, lint)

platformOpt := buildPlatformOpt(&opt)
targetName := opt.Target
if targetName == "" {
targetName = stages[len(stages)-1].Name
}
globalArgs := defaultArgs(platformOpt, opt.BuildArgs, targetName)

shlex := shell.NewLex(dockerfile.EscapeToken)
outline := newOutlineCapture()

Expand Down
6 changes: 5 additions & 1 deletion frontend/dockerfile/dockerfile2llb/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,12 @@ func buildPlatformOpt(opt *ConvertOpt) *platformOpt {
}
}

func platformArgs(po *platformOpt, overrides map[string]string) *llb.EnvList {
func defaultArgs(po *platformOpt, overrides map[string]string, target string) *llb.EnvList {
bp := po.buildPlatforms[0]
tp := po.targetPlatform
if target == "" {
target = "default"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should have default if target not set. I think best would be to leave it empty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty value could cause some breakage maybe. We do name stage default in the --call=targets output as well. Another option would be to use numbers as we still support them for access, but might be quite confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do name stage default in the --call=targets output as well.

Ah I forgot about this subrequest, so yeah default is good. I was just wondering if default could be used as TARGETSTAGE but that would not make sense.

Copy link

@ByteNybbler ByteNybbler Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see it going either way.

--call=targets annotates whatever the final build stage is with (default), regardless of whether the build stage's name is an empty string or not.

I think having TARGETSTAGE be default when the Dockerfile's targeted build stage doesn't have a name might help avoid some potentially surprising behaviors. For example, there could be a scenario where the value of TARGETSTAGE ends up getting used as a directory name.

On the other hand, a Dockerfile can have a build stage named default which isn't the final build stage, so applying a default value for TARGETSTAGE here might be surprising in its own way. But that sounds like a pretty unusual Dockerfile.

I think just going with default will be applicable to the vast majority of use cases.

}
s := [...][2]string{
{"BUILDPLATFORM", platforms.Format(bp)},
{"BUILDOS", bp.OS},
Expand All @@ -48,6 +51,7 @@ func platformArgs(po *platformOpt, overrides map[string]string) *llb.EnvList {
{"TARGETOS", tp.OS},
{"TARGETARCH", tp.Architecture},
{"TARGETVARIANT", tp.Variant},
{"TARGETSTAGE", target},
}
env := &llb.EnvList{}
for _, kv := range s {
Expand Down
116 changes: 116 additions & 0 deletions frontend/dockerfile/dockerfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ var allTests = integration.TestFuncs(
testHistoryFinalizeTrace,
testEmptyStages,
testLocalCustomSessionID,
testTargetStageNameArg,
)

// Tests that depend on the `security.*` entitlements
Expand Down Expand Up @@ -1791,6 +1792,121 @@ COPY Dockerfile .
}
}

func testTargetStageNameArg(t *testing.T, sb integration.Sandbox) {
integration.SkipOnPlatform(t, "windows")
f := getFrontend(t, sb)

dockerfile := []byte(`
FROM alpine AS base
WORKDIR /out
RUN echo -n "value:$TARGETSTAGE" > /out/first
ARG TARGETSTAGE
RUN echo -n "value:$TARGETSTAGE" > /out/second

FROM scratch AS foo
COPY --from=base /out/ /

FROM scratch
COPY --from=base /out/ /
tonistiigi marked this conversation as resolved.
Show resolved Hide resolved
`)

dir := integration.Tmpdir(
t,
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)

destDir := t.TempDir()

c, err := client.New(sb.Context(), sb.Address())
require.NoError(t, err)
defer c.Close()

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
LocalMounts: map[string]fsutil.FS{
dockerui.DefaultLocalNameDockerfile: dir,
dockerui.DefaultLocalNameContext: dir,
},
FrontendAttrs: map[string]string{
"target": "foo",
},
Exports: []client.ExportEntry{
{
Type: client.ExporterLocal,
OutputDir: destDir,
},
},
}, nil)
require.NoError(t, err)

dt, err := os.ReadFile(filepath.Join(destDir, "first"))
require.NoError(t, err)
require.Equal(t, "value:", string(dt))

dt, err = os.ReadFile(filepath.Join(destDir, "second"))
require.NoError(t, err)
require.Equal(t, "value:foo", string(dt))

destDir = t.TempDir()

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
LocalMounts: map[string]fsutil.FS{
dockerui.DefaultLocalNameDockerfile: dir,
dockerui.DefaultLocalNameContext: dir,
},
Exports: []client.ExportEntry{
{
Type: client.ExporterLocal,
OutputDir: destDir,
},
},
}, nil)
require.NoError(t, err)

dt, err = os.ReadFile(filepath.Join(destDir, "first"))
require.NoError(t, err)
require.Equal(t, "value:", string(dt))

dt, err = os.ReadFile(filepath.Join(destDir, "second"))
require.NoError(t, err)
require.Equal(t, "value:default", string(dt))

// stage name defined in Dockerfile but not passed in request
dockerfile = append(dockerfile, []byte(`

FROM scratch AS final
COPY --from=base /out/ /
`)...)

dir = integration.Tmpdir(
t,
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)

destDir = t.TempDir()

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
LocalMounts: map[string]fsutil.FS{
dockerui.DefaultLocalNameDockerfile: dir,
dockerui.DefaultLocalNameContext: dir,
},
Exports: []client.ExportEntry{
{
Type: client.ExporterLocal,
OutputDir: destDir,
},
},
}, nil)
require.NoError(t, err)

dt, err = os.ReadFile(filepath.Join(destDir, "first"))
require.NoError(t, err)
require.Equal(t, "value:", string(dt))

dt, err = os.ReadFile(filepath.Join(destDir, "second"))
require.NoError(t, err)
require.Equal(t, "value:final", string(dt))
}

func testExportMultiPlatform(t *testing.T, sb integration.Sandbox) {
integration.SkipOnPlatform(t, "windows")
workers.CheckFeatureCompat(t, sb, workers.FeatureOCIExporter, workers.FeatureMultiPlatform)
Expand Down
Loading