From c810b8ed25c302cf1c286db9b1992253ade94f99 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Mon, 21 Sep 2020 18:32:43 -0700 Subject: [PATCH] dockerfile: allow multiple values for ARG Signed-off-by: Tonis Tiigi --- frontend/dockerfile/dockerfile2llb/convert.go | 44 +++++++++-------- frontend/dockerfile/dockerfile_test.go | 47 +++++++++++++++++++ frontend/dockerfile/instructions/commands.go | 21 +++++---- frontend/dockerfile/instructions/parse.go | 44 +++++++++-------- .../dockerfile/instructions/parse_test.go | 5 -- .../parser/testfiles/args/Dockerfile | 3 ++ .../dockerfile/parser/testfiles/args/result | 3 ++ 7 files changed, 114 insertions(+), 53 deletions(-) create mode 100644 frontend/dockerfile/parser/testfiles/args/Dockerfile create mode 100644 frontend/dockerfile/parser/testfiles/args/result diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 76a7f90c..689d2d1e 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -94,11 +94,13 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State, shlex := shell.NewLex(dockerfile.EscapeToken) - for _, metaArg := range metaArgs { - if metaArg.Value != nil { - *metaArg.Value, _ = shlex.ProcessWordWithMap(*metaArg.Value, metaArgsToMap(optMetaArgs)) + for _, cmd := range metaArgs { + for _, metaArg := range cmd.Args { + if metaArg.Value != nil { + *metaArg.Value, _ = shlex.ProcessWordWithMap(*metaArg.Value, metaArgsToMap(optMetaArgs)) + } + optMetaArgs = append(optMetaArgs, setKVValue(metaArg, opt.BuildArgs)) } - optMetaArgs = append(optMetaArgs, setKVValue(metaArg.KeyValuePairOptional, opt.BuildArgs)) } metaResolver := opt.MetaResolver @@ -1072,26 +1074,30 @@ func dispatchShell(d *dispatchState, c *instructions.ShellCommand) error { } func dispatchArg(d *dispatchState, c *instructions.ArgCommand, metaArgs []instructions.KeyValuePairOptional, buildArgValues map[string]string) error { - commitStr := "ARG " + c.Key - buildArg := setKVValue(c.KeyValuePairOptional, buildArgValues) + commitStrs := make([]string, 0, len(c.Args)) + for _, arg := range c.Args { + buildArg := setKVValue(arg, buildArgValues) - if c.Value != nil { - commitStr += "=" + *c.Value - } - if buildArg.Value == nil { - for _, ma := range metaArgs { - if ma.Key == buildArg.Key { - buildArg.Value = ma.Value + commitStr := arg.Key + if arg.Value != nil { + commitStr += "=" + *arg.Value + } + commitStrs = append(commitStrs, commitStr) + if buildArg.Value == nil { + for _, ma := range metaArgs { + if ma.Key == buildArg.Key { + buildArg.Value = ma.Value + } } } - } - if buildArg.Value != nil { - d.state = d.state.AddEnv(buildArg.Key, *buildArg.Value) - } + if buildArg.Value != nil { + d.state = d.state.AddEnv(buildArg.Key, *buildArg.Value) + } - d.buildArgs = append(d.buildArgs, buildArg) - return commitToHistory(&d.image, commitStr, false, nil) + d.buildArgs = append(d.buildArgs, buildArg) + } + return commitToHistory(&d.image, "ARG "+strings.Join(commitStrs, " "), false, nil) } func pathRelativeToWorkingDir(s llb.State, p string) (string, error) { diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 9390dcbf..88481bbe 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -101,6 +101,7 @@ var allTests = []integration.Test{ testFrontendUseForwardedSolveResults, testFrontendInputs, testErrorsSourceMap, + testMultiArgs, } var fileOpTests = []integration.Test{ @@ -1158,6 +1159,52 @@ COPY --from=build /out . require.Equal(t, "bar-box-foo", string(dt)) } +func testMultiArgs(t *testing.T, sb integration.Sandbox) { + f := getFrontend(t, sb) + + dockerfile := []byte(` +ARG a1="foo bar" a2=box +ARG a3="$a2-foo" +FROM busy$a2 AS build +ARG a3 a4="123 456" a1 +RUN echo -n "$a1:$a3:$a4" > /out +FROM scratch +COPY --from=build /out . +`) + + dir, err := tmpdir( + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + require.NoError(t, err) + defer os.RemoveAll(dir) + + c, err := client.New(context.TODO(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + destDir, err := ioutil.TempDir("", "buildkit") + require.NoError(t, err) + defer os.RemoveAll(destDir) + + _, err = f.Solve(context.TODO(), c, client.SolveOpt{ + LocalDirs: map[string]string{ + builder.DefaultLocalNameDockerfile: dir, + builder.DefaultLocalNameContext: dir, + }, + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: destDir, + }, + }, + }, nil) + require.NoError(t, err) + + dt, err := ioutil.ReadFile(filepath.Join(destDir, "out")) + require.NoError(t, err) + require.Equal(t, "foo bar:box-foo:123 456", string(dt)) +} + func testExportMultiPlatform(t *testing.T, sb integration.Sandbox) { skipDockerd(t, sb) f := getFrontend(t, sb) diff --git a/frontend/dockerfile/instructions/commands.go b/frontend/dockerfile/instructions/commands.go index 4a4146a4..1190a4c3 100644 --- a/frontend/dockerfile/instructions/commands.go +++ b/frontend/dockerfile/instructions/commands.go @@ -380,22 +380,25 @@ func (c *StopSignalCommand) CheckPlatform(platform string) error { // Dockerfile author may optionally set a default value of this variable. type ArgCommand struct { withNameAndCode - KeyValuePairOptional + Args []KeyValuePairOptional } // Expand variables func (c *ArgCommand) Expand(expander SingleWordExpander) error { - p, err := expander(c.Key) - if err != nil { - return err - } - c.Key = p - if c.Value != nil { - p, err = expander(*c.Value) + for i, v := range c.Args { + p, err := expander(v.Key) if err != nil { return err } - c.Value = &p + v.Key = p + if v.Value != nil { + p, err = expander(*v.Value) + if err != nil { + return err + } + v.Value = &p + } + c.Args[i] = v } return nil } diff --git a/frontend/dockerfile/instructions/parse.go b/frontend/dockerfile/instructions/parse.go index 1be9d7b2..9bdd6af3 100644 --- a/frontend/dockerfile/instructions/parse.go +++ b/frontend/dockerfile/instructions/parse.go @@ -579,33 +579,37 @@ func parseStopSignal(req parseRequest) (*StopSignalCommand, error) { } func parseArg(req parseRequest) (*ArgCommand, error) { - if len(req.args) != 1 { - return nil, errExactlyOneArgument("ARG") + if len(req.args) < 1 { + return nil, errAtLeastOneArgument("ARG") } - kvpo := KeyValuePairOptional{} + pairs := make([]KeyValuePairOptional, len(req.args)) - arg := req.args[0] - // 'arg' can just be a name or name-value pair. Note that this is different - // from 'env' that handles the split of name and value at the parser level. - // The reason for doing it differently for 'arg' is that we support just - // defining an arg and not assign it a value (while 'env' always expects a - // name-value pair). If possible, it will be good to harmonize the two. - if strings.Contains(arg, "=") { - parts := strings.SplitN(arg, "=", 2) - if len(parts[0]) == 0 { - return nil, errBlankCommandNames("ARG") + for i, arg := range req.args { + kvpo := KeyValuePairOptional{} + + // 'arg' can just be a name or name-value pair. Note that this is different + // from 'env' that handles the split of name and value at the parser level. + // The reason for doing it differently for 'arg' is that we support just + // defining an arg and not assign it a value (while 'env' always expects a + // name-value pair). If possible, it will be good to harmonize the two. + if strings.Contains(arg, "=") { + parts := strings.SplitN(arg, "=", 2) + if len(parts[0]) == 0 { + return nil, errBlankCommandNames("ARG") + } + + kvpo.Key = parts[0] + kvpo.Value = &parts[1] + } else { + kvpo.Key = arg } - - kvpo.Key = parts[0] - kvpo.Value = &parts[1] - } else { - kvpo.Key = arg + pairs[i] = kvpo } return &ArgCommand{ - KeyValuePairOptional: kvpo, - withNameAndCode: newWithNameAndCode(req), + Args: pairs, + withNameAndCode: newWithNameAndCode(req), }, nil } diff --git a/frontend/dockerfile/instructions/parse_test.go b/frontend/dockerfile/instructions/parse_test.go index fe8ac50c..394caad9 100644 --- a/frontend/dockerfile/instructions/parse_test.go +++ b/frontend/dockerfile/instructions/parse_test.go @@ -163,11 +163,6 @@ func TestErrorCases(t *testing.T) { dockerfile: "ONBUILD MAINTAINER docker.io", expectedError: "MAINTAINER isn't allowed as an ONBUILD trigger", }, - { - name: "ARG two arguments", - dockerfile: "ARG foo bar", - expectedError: "ARG requires exactly one argument", - }, { name: "MAINTAINER unknown flag", dockerfile: "MAINTAINER --boo joe@example.com", diff --git a/frontend/dockerfile/parser/testfiles/args/Dockerfile b/frontend/dockerfile/parser/testfiles/args/Dockerfile new file mode 100644 index 00000000..6dcbdb9c --- /dev/null +++ b/frontend/dockerfile/parser/testfiles/args/Dockerfile @@ -0,0 +1,3 @@ +ARG foo bar=baz +FROM ubuntu +ARG abc="123 456" def diff --git a/frontend/dockerfile/parser/testfiles/args/result b/frontend/dockerfile/parser/testfiles/args/result new file mode 100644 index 00000000..90db1d5b --- /dev/null +++ b/frontend/dockerfile/parser/testfiles/args/result @@ -0,0 +1,3 @@ +(arg "foo" "bar=baz") +(from "ubuntu") +(arg "abc=\"123 456\"" "def")