Merge pull request #33719 from dnephin/warn-on-empty-continuation-carry

[Builder] Warn on empty continuation lines

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 == "" {

builder/dockerfile/parser/parser.go

@@ -243,6 +243,15 @@ type Result struct {
 	AST         *Node
 	EscapeToken rune
 	Platform    string
+	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
@@ -252,6 +261,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 +282,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 +290,8 @@ func Parse(rwc io.Reader) (*Result, error) {
 			}
 			currentLine++
 
-			// TODO: warn this is being deprecated/removed
 			if isEmptyContinuationLine(bytesRead) {
+				hasEmptyContinuationLine = true
 				continue
 			}
 
@@ -289,13 +300,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 +351,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))
 }

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")
+}

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)
 }