diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index d02b3cd7a4..744f503dd3 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -62,7 +62,7 @@ type Builder struct { disableCommit bool cacheBusted bool buildArgs *buildArgs - directive *parser.Directive + escapeToken rune imageCache builder.ImageCache from builder.Image @@ -122,7 +122,7 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back runConfig: new(container.Config), tmpContainers: map[string]struct{}{}, buildArgs: newBuildArgs(config.BuildArgs), - directive: parser.NewDefaultDirective(), + escapeToken: parser.DefaultEscapeToken, } b.imageContexts = &imageContexts{b: b} return b, nil @@ -190,9 +190,9 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri return "", err } - addNodesForLabelOption(dockerfile, b.options.Labels) + addNodesForLabelOption(dockerfile.AST, b.options.Labels) - if err := checkDispatchDockerfile(dockerfile); err != nil { + if err := checkDispatchDockerfile(dockerfile.AST); err != nil { return "", err } @@ -220,10 +220,13 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri return b.image, nil } -func (b *Builder) dispatchDockerfileWithCancellation(dockerfile *parser.Node) (string, error) { - total := len(dockerfile.Children) +func (b *Builder) dispatchDockerfileWithCancellation(dockerfile *parser.Result) (string, error) { + // TODO: pass this to dispatchRequest instead + b.escapeToken = dockerfile.EscapeToken + + total := len(dockerfile.AST.Children) var shortImgID string - for i, n := range dockerfile.Children { + for i, n := range dockerfile.AST.Children { select { case <-b.clientCtx.Done(): logrus.Debug("Builder: build cancelled!") @@ -320,13 +323,13 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con return nil, err } - ast, err := parser.Parse(bytes.NewBufferString(strings.Join(changes, "\n")), b.directive) + result, err := parser.Parse(bytes.NewBufferString(strings.Join(changes, "\n"))) if err != nil { return nil, err } // ensure that the commands are valid - for _, n := range ast.Children { + for _, n := range result.AST.Children { if !validCommitCommands[n.Value] { return nil, fmt.Errorf("%s is not a valid change command", n.Value) } @@ -337,11 +340,11 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con b.Stderr = ioutil.Discard b.disableCommit = true - if err := checkDispatchDockerfile(ast); err != nil { + if err := checkDispatchDockerfile(result.AST); err != nil { return nil, err } - if err := dispatchFromDockerfile(b, ast); err != nil { + if err := dispatchFromDockerfile(b, result); err != nil { return nil, err } return b.runConfig, nil @@ -356,8 +359,12 @@ func checkDispatchDockerfile(dockerfile *parser.Node) error { return nil } -func dispatchFromDockerfile(b *Builder, ast *parser.Node) error { +func dispatchFromDockerfile(b *Builder, result *parser.Result) error { + // TODO: pass this to dispatchRequest instead + b.escapeToken = result.EscapeToken + ast := result.AST total := len(ast.Children) + for i, n := range ast.Children { if err := b.dispatch(i, total, n); err != nil { return err diff --git a/builder/dockerfile/builder_test.go b/builder/dockerfile/builder_test.go index cd79af1dfc..5c2fc326fe 100644 --- a/builder/dockerfile/builder_test.go +++ b/builder/dockerfile/builder_test.go @@ -10,8 +10,7 @@ import ( func TestAddNodesForLabelOption(t *testing.T) { dockerfile := "FROM scratch" - d := parser.NewDefaultDirective() - nodes, err := parser.Parse(strings.NewReader(dockerfile), d) + result, err := parser.Parse(strings.NewReader(dockerfile)) assert.NilError(t, err) labels := map[string]string{ @@ -21,6 +20,7 @@ func TestAddNodesForLabelOption(t *testing.T) { "org.b": "cli-b", "org.a": "cli-a", } + nodes := result.AST addNodesForLabelOption(nodes, labels) expected := []string{ diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 5a467e6285..3df8070b93 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -200,7 +200,7 @@ func from(b *Builder, args []string, attributes map[string]bool, original string substituionArgs = append(substituionArgs, key+"="+value) } - name, err := ProcessWord(args[0], substituionArgs, b.directive.EscapeToken()) + name, err := ProcessWord(args[0], substituionArgs, b.escapeToken) if err != nil { return err } diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index d721637d13..644b610825 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -10,7 +10,6 @@ import ( "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/strslice" "github.com/docker/docker/builder" - "github.com/docker/docker/builder/dockerfile/parser" "github.com/docker/docker/pkg/testutil/assert" "github.com/docker/go-connections/nat" ) @@ -205,7 +204,6 @@ func newBuilderWithMockBackend() *Builder { options: &types.ImageBuildOptions{}, docker: &MockBackend{}, buildArgs: newBuildArgs(make(map[string]*string)), - directive: parser.NewDefaultDirective(), } b.imageContexts = &imageContexts{b: b} return b diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index df50ee561d..472e9ae09a 100644 --- a/builder/dockerfile/evaluator.go +++ b/builder/dockerfile/evaluator.go @@ -180,7 +180,7 @@ func (b *Builder) evaluateEnv(cmd string, str string, envs []string) ([]string, return []string{word}, err } } - return processFunc(str, envs, b.directive.EscapeToken()) + return processFunc(str, envs, b.escapeToken) } // buildArgsWithoutConfigEnv returns a list of key=value pairs for all the build diff --git a/builder/dockerfile/evaluator_test.go b/builder/dockerfile/evaluator_test.go index adc72d3334..419fa0f9ff 100644 --- a/builder/dockerfile/evaluator_test.go +++ b/builder/dockerfile/evaluator_test.go @@ -171,7 +171,7 @@ func executeTestCase(t *testing.T, testCase dispatchTestCase) { }() r := strings.NewReader(testCase.dockerfile) - n, err := parser.Parse(r, parser.NewDefaultDirective()) + result, err := parser.Parse(r) if err != nil { t.Fatalf("Error when parsing Dockerfile: %s", err) @@ -188,9 +188,9 @@ func executeTestCase(t *testing.T, testCase dispatchTestCase) { Stdout: ioutil.Discard, context: context, buildArgs: newBuildArgs(options.BuildArgs), - directive: parser.NewDefaultDirective(), } + n := result.AST err = b.dispatch(0, len(n.Children), n.Children[0]) if err == nil { diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 5f9b9b0648..e4e59e87f8 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -462,12 +462,12 @@ func (b *Builder) processImageFrom(img builder.Image) error { // parse the ONBUILD triggers by invoking the parser for _, step := range onBuildTriggers { - ast, err := parser.Parse(strings.NewReader(step), b.directive) + result, err := parser.Parse(strings.NewReader(step)) if err != nil { return err } - for _, n := range ast.Children { + for _, n := range result.AST.Children { if err := checkDispatch(n); err != nil { return err } @@ -481,7 +481,7 @@ func (b *Builder) processImageFrom(img builder.Image) error { } } - if err := dispatchFromDockerfile(b, ast); err != nil { + if err := dispatchFromDockerfile(b, result); err != nil { return err } } @@ -650,7 +650,7 @@ func (b *Builder) clearTmp() { } // readAndParseDockerfile reads a Dockerfile from the current context. -func (b *Builder) readAndParseDockerfile() (*parser.Node, error) { +func (b *Builder) readAndParseDockerfile() (*parser.Result, error) { // If no -f was specified then look for 'Dockerfile'. If we can't find // that then look for 'dockerfile'. If neither are found then default // back to 'Dockerfile' and use that in the error message. @@ -664,9 +664,9 @@ func (b *Builder) readAndParseDockerfile() (*parser.Node, error) { } } - nodes, err := b.parseDockerfile() + result, err := b.parseDockerfile() if err != nil { - return nodes, err + return nil, err } // After the Dockerfile has been parsed, we need to check the .dockerignore @@ -680,10 +680,10 @@ func (b *Builder) readAndParseDockerfile() (*parser.Node, error) { if dockerIgnore, ok := b.context.(builder.DockerIgnoreContext); ok { dockerIgnore.Process([]string{b.options.Dockerfile}) } - return nodes, nil + return result, nil } -func (b *Builder) parseDockerfile() (*parser.Node, error) { +func (b *Builder) parseDockerfile() (*parser.Result, error) { f, err := b.context.Open(b.options.Dockerfile) if err != nil { if os.IsNotExist(err) { @@ -702,5 +702,5 @@ func (b *Builder) parseDockerfile() (*parser.Node, error) { return nil, fmt.Errorf("The Dockerfile (%s) cannot be empty", b.options.Dockerfile) } } - return parser.Parse(f, b.directive) + return parser.Parse(f) } diff --git a/builder/dockerfile/parser/dumper/main.go b/builder/dockerfile/parser/dumper/main.go index 5e9af94121..0d98b49270 100644 --- a/builder/dockerfile/parser/dumper/main.go +++ b/builder/dockerfile/parser/dumper/main.go @@ -5,6 +5,7 @@ import ( "os" "github.com/docker/docker/builder/dockerfile/parser" + "go/ast" ) func main() { @@ -23,12 +24,10 @@ func main() { } defer f.Close() - d := parser.NewDefaultDirective() - ast, err := parser.Parse(f, d) + result, err := parser.Parse(f) if err != nil { panic(err) - } else { - fmt.Println(ast.Dump()) } + fmt.Println(result.AST.Dump()) } } diff --git a/builder/dockerfile/parser/json_test.go b/builder/dockerfile/parser/json_test.go index 26eea97064..d4489191da 100644 --- a/builder/dockerfile/parser/json_test.go +++ b/builder/dockerfile/parser/json_test.go @@ -28,10 +28,9 @@ var validJSONArraysOfStrings = map[string][]string{ func TestJSONArraysOfStrings(t *testing.T) { for json, expected := range validJSONArraysOfStrings { - d := Directive{} - d.SetEscapeToken(DefaultEscapeToken) + d := NewDefaultDirective() - if node, _, err := parseJSON(json, &d); err != nil { + if node, _, err := parseJSON(json, d); err != nil { t.Fatalf("%q should be a valid JSON array of strings, but wasn't! (err: %q)", json, err) } else { i := 0 @@ -51,10 +50,9 @@ func TestJSONArraysOfStrings(t *testing.T) { } } for _, json := range invalidJSONArraysOfStrings { - d := Directive{} - d.SetEscapeToken(DefaultEscapeToken) + d := NewDefaultDirective() - if _, _, err := parseJSON(json, &d); err != errDockerfileNotStringArray { + if _, _, err := parseJSON(json, d); err != errDockerfileNotStringArray { t.Fatalf("%q should be an invalid JSON array of strings, but wasn't!", json) } } diff --git a/builder/dockerfile/parser/parser.go b/builder/dockerfile/parser/parser.go index e4b3aa61e6..5864eb9c6c 100644 --- a/builder/dockerfile/parser/parser.go +++ b/builder/dockerfile/parser/parser.go @@ -34,7 +34,7 @@ type Node struct { Original string // original line used before parsing Flags []string // only top Node should have this set StartLine int // the line in the original dockerfile where the node begins - EndLine int // the line in the original dockerfile where the node ends + endLine int // the line in the original dockerfile where the node ends } // Dump dumps the AST defined by `node` as a list of sexps. @@ -70,7 +70,7 @@ var ( ) // DefaultEscapeToken is the default escape token -const DefaultEscapeToken = "\\" +const DefaultEscapeToken = '\\' // Directive is the structure used during a build run to hold the state of // parsing directives. @@ -81,8 +81,8 @@ type Directive struct { escapeSeen bool // Whether the escape directive has been seen } -// SetEscapeToken sets the default token for escaping characters in a Dockerfile. -func (d *Directive) SetEscapeToken(s string) error { +// setEscapeToken sets the default token for escaping characters in a Dockerfile. +func (d *Directive) setEscapeToken(s string) error { if s != "`" && s != "\\" { return fmt.Errorf("invalid ESCAPE '%s'. Must be ` or \\", s) } @@ -91,18 +91,13 @@ func (d *Directive) SetEscapeToken(s string) error { return nil } -// EscapeToken returns the escape token -func (d *Directive) EscapeToken() rune { - return d.escapeToken -} - // NewDefaultDirective returns a new Directive with the default escapeToken token func NewDefaultDirective() *Directive { directive := Directive{ escapeSeen: false, lookingForDirectives: true, } - directive.SetEscapeToken(DefaultEscapeToken) + directive.setEscapeToken(string(DefaultEscapeToken)) return &directive } @@ -200,7 +195,7 @@ func handleParserDirective(line string, d *Directive) (bool, error) { } for i, n := range tokenEscapeCommand.SubexpNames() { if n == "escapechar" { - if err := d.SetEscapeToken(tecMatch[i]); err != nil { + if err := d.setEscapeToken(tecMatch[i]); err != nil { return false, err } return true, nil @@ -209,9 +204,16 @@ func handleParserDirective(line string, d *Directive) (bool, error) { return false, nil } -// Parse is the main parse routine. -// It handles an io.ReadWriteCloser and returns the root of the AST. -func Parse(rwc io.Reader, d *Directive) (*Node, error) { +// Result is the result of parsing a Dockerfile +type Result struct { + AST *Node + EscapeToken rune +} + +// Parse reads lines from a Reader, parses the lines into an AST and returns +// the AST and escape token +func Parse(rwc io.Reader) (*Result, error) { + d := NewDefaultDirective() currentLine := 0 root := &Node{} root.StartLine = -1 @@ -267,18 +269,18 @@ func Parse(rwc io.Reader, d *Directive) (*Node, error) { if child != nil { // Update the line information for the current child. child.StartLine = startLine - child.EndLine = currentLine + child.endLine = currentLine // Update the line information for the root. The starting line of the root is always the // starting line of the first child and the ending line is the ending line of the last child. if root.StartLine < 0 { root.StartLine = currentLine } - root.EndLine = currentLine + root.endLine = currentLine root.Children = append(root.Children, child) } } - return root, nil + return &Result{AST: root, EscapeToken: d.escapeToken}, nil } // covers comments and empty lines. Lines should be trimmed before passing to diff --git a/builder/dockerfile/parser/parser_test.go b/builder/dockerfile/parser/parser_test.go index 82e78e91a9..af54e097bf 100644 --- a/builder/dockerfile/parser/parser_test.go +++ b/builder/dockerfile/parser/parser_test.go @@ -8,6 +8,8 @@ import ( "path/filepath" "runtime" "testing" + + "github.com/docker/docker/pkg/testutil/assert" ) const testDir = "testfiles" @@ -16,17 +18,11 @@ const testFileLineInfo = "testfile-line/Dockerfile" func getDirs(t *testing.T, dir string) []string { f, err := os.Open(dir) - if err != nil { - t.Fatal(err) - } - + assert.NilError(t, err) defer f.Close() dirs, err := f.Readdirnames(0) - if err != nil { - t.Fatal(err) - } - + assert.NilError(t, err) return dirs } @@ -35,15 +31,11 @@ func TestTestNegative(t *testing.T) { dockerfile := filepath.Join(negativeTestDir, dir, "Dockerfile") df, err := os.Open(dockerfile) - if err != nil { - t.Fatalf("Dockerfile missing for %s: %v", dir, err) - } + assert.NilError(t, err) defer df.Close() - _, err = Parse(df, NewDefaultDirective()) - if err == nil { - t.Fatalf("No error parsing broken dockerfile for %s", dir) - } + _, err = Parse(df) + assert.Error(t, err, "") } } @@ -53,31 +45,21 @@ func TestTestData(t *testing.T) { resultfile := filepath.Join(testDir, dir, "result") df, err := os.Open(dockerfile) - if err != nil { - t.Fatalf("Dockerfile missing for %s: %v", dir, err) - } + assert.NilError(t, err) defer df.Close() - ast, err := Parse(df, NewDefaultDirective()) - if err != nil { - t.Fatalf("Error parsing %s's dockerfile: %v", dir, err) - } + result, err := Parse(df) + assert.NilError(t, err) content, err := ioutil.ReadFile(resultfile) - if err != nil { - t.Fatalf("Error reading %s's result file: %v", dir, err) - } + assert.NilError(t, err) if runtime.GOOS == "windows" { // CRLF --> CR to match Unix behavior content = bytes.Replace(content, []byte{'\x0d', '\x0a'}, []byte{'\x0a'}, -1) } - if ast.Dump()+"\n" != string(content) { - fmt.Fprintln(os.Stderr, "Result:\n"+ast.Dump()) - fmt.Fprintln(os.Stderr, "Expected:\n"+string(content)) - t.Fatalf("%s: AST dump of dockerfile does not match result", dir) - } + assert.Equal(t, result.AST.Dump()+"\n", string(content)) } } @@ -119,46 +101,33 @@ func TestParseWords(t *testing.T) { for _, test := range tests { words := parseWords(test["input"][0], NewDefaultDirective()) - if len(words) != len(test["expect"]) { - t.Fatalf("length check failed. input: %v, expect: %q, output: %q", test["input"][0], test["expect"], words) - } - for i, word := range words { - if word != test["expect"][i] { - t.Fatalf("word check failed for word: %q. input: %q, expect: %q, output: %q", word, test["input"][0], test["expect"], words) - } - } + assert.DeepEqual(t, words, test["expect"]) } } func TestLineInformation(t *testing.T) { df, err := os.Open(testFileLineInfo) - if err != nil { - t.Fatalf("Dockerfile missing for %s: %v", testFileLineInfo, err) - } + assert.NilError(t, err) defer df.Close() - ast, err := Parse(df, NewDefaultDirective()) - if err != nil { - t.Fatalf("Error parsing dockerfile %s: %v", testFileLineInfo, err) - } + result, err := Parse(df) + assert.NilError(t, err) - if ast.StartLine != 5 || ast.EndLine != 31 { - fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 31, ast.StartLine, ast.EndLine) + ast := result.AST + if ast.StartLine != 5 || ast.endLine != 31 { + fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 31, ast.StartLine, ast.endLine) t.Fatal("Root line information doesn't match result.") } - if len(ast.Children) != 3 { - fmt.Fprintf(os.Stderr, "Wrong number of child: expected(%d), actual(%d)\n", 3, len(ast.Children)) - t.Fatalf("Root line information doesn't match result for %s", testFileLineInfo) - } + assert.Equal(t, len(ast.Children), 3) expected := [][]int{ {5, 5}, {11, 12}, {17, 31}, } for i, child := range ast.Children { - if child.StartLine != expected[i][0] || child.EndLine != expected[i][1] { + if child.StartLine != expected[i][0] || child.endLine != expected[i][1] { t.Logf("Wrong line information for child %d: expected(%d-%d), actual(%d-%d)\n", - i, expected[i][0], expected[i][1], child.StartLine, child.EndLine) + i, expected[i][0], expected[i][1], child.StartLine, child.endLine) t.Fatal("Root line information doesn't match result.") } }