From 78a82a7623d891f96077b60be7b9dd238a104ec5 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Mon, 5 Dec 2016 18:55:07 -0800 Subject: [PATCH 1/2] Warn/deprecate continuing on empty lines in `Dockerfile` This fix is related to 29005 and 24693. Currently in `Dockerfile` empty lines will continue as long as there is line escape before. This may cause some issues. The issue in 24693 is an example. A non-empty line after an empty line might be considered to be a separate instruction by many users. However, it is actually part of the last instruction under the current `Dockerfile` parsing rule. This fix is an effort to reduce the confusion around the parsing of `Dockerfile`. Even though this fix does not change the behavior of the `Dockerfile` parsing, it tries to deprecate the empty line continuation and present a warning for the user. In this case, at least it prompt users to check for the Dockerfile and avoid the confusion if possible. Signed-off-by: Yong Tang Upstream-commit: 7815c8f8754d5473eda7cd80277a4ea3c59e3c29 Component: engine --- .../builder/dockerfile/parser/parser.go | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/components/engine/builder/dockerfile/parser/parser.go b/components/engine/builder/dockerfile/parser/parser.go index a1f7a74570..6f6df9893d 100644 --- a/components/engine/builder/dockerfile/parser/parser.go +++ b/components/engine/builder/dockerfile/parser/parser.go @@ -243,6 +243,7 @@ type Result struct { AST *Node EscapeToken rune Platform string + Warnings []string } // Parse reads lines from a Reader, parses the lines into an AST and returns @@ -252,6 +253,7 @@ func Parse(rwc io.Reader) (*Result, error) { currentLine := 0 root := &Node{StartLine: -1} scanner := bufio.NewScanner(rwc) + warnings := []string{} var err error for scanner.Scan() { @@ -272,6 +274,7 @@ func Parse(rwc io.Reader) (*Result, error) { continue } + var hasEmptyContinuationLine bool for !isEndOfLine && scanner.Scan() { bytesRead, err := processLine(d, scanner.Bytes(), false) if err != nil { @@ -279,8 +282,8 @@ func Parse(rwc io.Reader) (*Result, error) { } currentLine++ - // TODO: warn this is being deprecated/removed if isEmptyContinuationLine(bytesRead) { + hasEmptyContinuationLine = true continue } @@ -289,13 +292,27 @@ func Parse(rwc io.Reader) (*Result, error) { line += continuationLine } + if hasEmptyContinuationLine { + warning := "[WARNING]: Empty continuation line found in:\n " + line + warnings = append(warnings, warning) + } + child, err := newNodeFromLine(line, d) if err != nil { return nil, err } root.AddChild(child, startLine, currentLine) } - return &Result{AST: root, EscapeToken: d.escapeToken, Platform: d.platformToken}, nil + + if len(warnings) > 0 { + warnings = append(warnings, "[WARNING]: Empty continuation lines will become errors in a future release.") + } + return &Result{ + AST: root, + Warnings: warnings, + EscapeToken: d.escapeToken, + Platform: d.platformToken, + }, nil } func trimComments(src []byte) []byte { @@ -326,6 +343,5 @@ func processLine(d *Directive, token []byte, stripLeftWhitespace bool) ([]byte, if stripLeftWhitespace { token = trimWhitespace(token) } - err := d.possibleParserDirective(string(token)) - return trimComments(token), err + return trimComments(token), d.possibleParserDirective(string(token)) } From 0a2d82735e511cf3a647e3bc106c8fd2dbf1e13d Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 16 Jun 2017 15:05:30 -0700 Subject: [PATCH 2/2] Add a test for warning on empty continuation lines. Signed-off-by: Daniel Nephin Upstream-commit: b47b375cb8bb0dca7ee0ebfa093bfc163ad867fb Component: engine --- .../engine/builder/dockerfile/builder.go | 1 + .../builder/dockerfile/parser/parser.go | 10 +++- .../builder/dockerfile/parser/parser_test.go | 57 ++++++++++++------- .../engine/builder/remotecontext/detect.go | 14 ++--- 4 files changed, 53 insertions(+), 29 deletions(-) diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go index 80cfaf0bc2..cee1436f92 100644 --- a/components/engine/builder/dockerfile/builder.go +++ b/components/engine/builder/dockerfile/builder.go @@ -255,6 +255,7 @@ func (b *Builder) build(source builder.Source, dockerfile *parser.Result) (*buil return nil, errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target) } + dockerfile.PrintWarnings(b.Stderr) b.buildArgs.WarnOnUnusedBuildArgs(b.Stderr) if dispatchState.imageID == "" { diff --git a/components/engine/builder/dockerfile/parser/parser.go b/components/engine/builder/dockerfile/parser/parser.go index 6f6df9893d..9766fbfdfe 100644 --- a/components/engine/builder/dockerfile/parser/parser.go +++ b/components/engine/builder/dockerfile/parser/parser.go @@ -246,6 +246,14 @@ type Result struct { Warnings []string } +// PrintWarnings to the writer +func (r *Result) PrintWarnings(out io.Writer) { + if len(r.Warnings) == 0 { + return + } + fmt.Fprintf(out, strings.Join(r.Warnings, "\n")+"\n") +} + // 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) { @@ -311,7 +319,7 @@ func Parse(rwc io.Reader) (*Result, error) { AST: root, Warnings: warnings, EscapeToken: d.escapeToken, - Platform: d.platformToken, + Platform: d.platformToken, }, nil } diff --git a/components/engine/builder/dockerfile/parser/parser_test.go b/components/engine/builder/dockerfile/parser/parser_test.go index 99b614ba9f..bb057ecab3 100644 --- a/components/engine/builder/dockerfile/parser/parser_test.go +++ b/components/engine/builder/dockerfile/parser/parser_test.go @@ -27,40 +27,39 @@ func getDirs(t *testing.T, dir string) []string { return dirs } -func TestTestNegative(t *testing.T) { +func TestParseErrorCases(t *testing.T) { for _, dir := range getDirs(t, negativeTestDir) { dockerfile := filepath.Join(negativeTestDir, dir, "Dockerfile") df, err := os.Open(dockerfile) - require.NoError(t, err) + require.NoError(t, err, dockerfile) defer df.Close() _, err = Parse(df) - assert.Error(t, err) + assert.Error(t, err, dockerfile) } } -func TestTestData(t *testing.T) { +func TestParseCases(t *testing.T) { for _, dir := range getDirs(t, testDir) { dockerfile := filepath.Join(testDir, dir, "Dockerfile") resultfile := filepath.Join(testDir, dir, "result") df, err := os.Open(dockerfile) - require.NoError(t, err) + require.NoError(t, err, dockerfile) defer df.Close() result, err := Parse(df) - require.NoError(t, err) + require.NoError(t, err, dockerfile) content, err := ioutil.ReadFile(resultfile) - require.NoError(t, err) + require.NoError(t, err, resultfile) if runtime.GOOS == "windows" { // CRLF --> CR to match Unix behavior content = bytes.Replace(content, []byte{'\x0d', '\x0a'}, []byte{'\x0a'}, -1) } - - assert.Contains(t, result.AST.Dump()+"\n", string(content), "In "+dockerfile) + assert.Equal(t, result.AST.Dump()+"\n", string(content), "In "+dockerfile) } } @@ -106,7 +105,7 @@ func TestParseWords(t *testing.T) { } } -func TestLineInformation(t *testing.T) { +func TestParseIncludesLineNumbers(t *testing.T) { df, err := os.Open(testFileLineInfo) require.NoError(t, err) defer df.Close() @@ -115,10 +114,8 @@ func TestLineInformation(t *testing.T) { require.NoError(t, err) 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.") - } + assert.Equal(t, 5, ast.StartLine) + assert.Equal(t, 31, ast.endLine) assert.Len(t, ast.Children, 3) expected := [][]int{ {5, 5}, @@ -126,10 +123,32 @@ func TestLineInformation(t *testing.T) { {17, 31}, } for i, child := range ast.Children { - 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) - t.Fatal("Root line information doesn't match result.") - } + msg := fmt.Sprintf("Child %d", i) + assert.Equal(t, expected[i], []int{child.StartLine, child.endLine}, msg) } } + +func TestParseWarnsOnEmptyContinutationLine(t *testing.T) { + dockerfile := bytes.NewBufferString(` +FROM alpine:3.6 + +RUN something \ + + following \ + + more + +RUN another \ + + thing + `) + + result, err := Parse(dockerfile) + require.NoError(t, err) + warnings := result.Warnings + assert.Len(t, warnings, 3) + assert.Contains(t, warnings[0], "Empty continuation line found in") + assert.Contains(t, warnings[0], "RUN something following more") + assert.Contains(t, warnings[1], "RUN another thing") + assert.Contains(t, warnings[2], "will become errors in a future release") +} diff --git a/components/engine/builder/remotecontext/detect.go b/components/engine/builder/remotecontext/detect.go index 889f0fce34..22ad5b7c9f 100644 --- a/components/engine/builder/remotecontext/detect.go +++ b/components/engine/builder/remotecontext/detect.go @@ -110,17 +110,13 @@ func newURLRemote(url string, dockerfilePath string, progressReader func(in io.R return progressReader(rc), nil }, }) - if err != nil { - if err == dockerfileFoundErr { - res, err := parser.Parse(dockerfile) - if err != nil { - return nil, nil, err - } - return nil, res, nil - } + switch { + case err == dockerfileFoundErr: + res, err := parser.Parse(dockerfile) + return nil, res, err + case err != nil: return nil, nil, err } - return withDockerfileFromContext(c.(modifiableContext), dockerfilePath) }