Merge pull request moby#24725 from yongtang/24693-dockerfile-empty-line-after-escape

thaJeztah · web-flow · commit c5adc271b299 · 2016-10-28T13:53:15.000-07:00
Fix dockerfile parser with empty line after escape
diff --git a/builder/dockerfile/parser/parser.go b/builder/dockerfile/parser/parser.go
@@ -176,10 +176,17 @@ func Parse(rwc io.Reader, d *Directive) (*Node, error) {
 				newline := scanner.Text()
 				currentLine++
 
-				if stripComments(strings.TrimSpace(newline)) == "" {
-					continue
+				// If escape followed by a comment line then stop
+				// Note here that comment line starts with `#` at
+				// the first pos of the line
+				if stripComments(newline) == "" {
+					break
 				}
 
+				// If escape followed by an empty line then stop
+				if strings.TrimSpace(newline) == "" {
+					break
+				}
 				line, child, err = ParseLine(line+newline, d)
 				if err != nil {
 					return nil, err
diff --git a/builder/dockerfile/parser/parser_test.go b/builder/dockerfile/parser/parser_test.go
@@ -150,8 +150,8 @@ func TestLineInformation(t *testing.T) {
 		t.Fatalf("Error parsing dockerfile %s: %v", testFileLineInfo, 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)
+	if ast.StartLine != 5 || ast.EndLine != 27 {
+		fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 27, ast.StartLine, ast.EndLine)
 		t.Fatalf("Root line information doesn't match result.")
 	}
 	if len(ast.Children) != 3 {
@@ -161,7 +161,7 @@ func TestLineInformation(t *testing.T) {
 	expected := [][]int{
 		{5, 5},
 		{11, 12},
-		{17, 31},
+		{17, 27},
 	}
 	for i, child := range ast.Children {
 		if child.StartLine != expected[i][0] || child.EndLine != expected[i][1] {
diff --git a/builder/dockerfile/parser/testfile-line/Dockerfile b/builder/dockerfile/parser/testfile-line/Dockerfile
@@ -16,15 +16,11 @@ ENV GOPATH \
 # Install the packages we need, clean up after them and us
 RUN apt-get update \
 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.clean \
-
-
     && apt-get install -y --no-install-recommends git golang ca-certificates \
     && apt-get clean \
     && rm -rf /var/lib/apt/lists \
-
 	&& go get -v github.com/brimstone/consuldock \
     && mv $GOPATH/bin/consuldock /usr/local/bin/consuldock \
-
 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty \
 	&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') \
 	&& rm /tmp/dpkg.* \
diff --git a/builder/dockerfile/parser/testfiles/brimstone-consuldock/Dockerfile b/builder/dockerfile/parser/testfiles/brimstone-consuldock/Dockerfile
@@ -16,10 +16,8 @@ RUN apt-get update \
     && apt-get install -y --no-install-recommends git golang ca-certificates \
     && apt-get clean \
     && rm -rf /var/lib/apt/lists \
-
 	&& go get -v github.com/brimstone/consuldock \
     && mv $GOPATH/bin/consuldock /usr/local/bin/consuldock \
-
 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty \
 	&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') \
 	&& rm /tmp/dpkg.* \
diff --git a/builder/dockerfile/parser/testfiles/brimstone-docker-consul/Dockerfile b/builder/dockerfile/parser/testfiles/brimstone-docker-consul/Dockerfile
@@ -23,14 +23,12 @@ RUN apt-get update \
     && apt-get install -y --no-install-recommends unzip wget \
     && apt-get clean \
     && rm -rf /var/lib/apt/lists \
-
     && cd /tmp \
     && wget https://dl.bintray.com/mitchellh/consul/0.3.1_web_ui.zip \
        -O web_ui.zip \
     && unzip web_ui.zip \
     && mv dist /webui \
     && rm web_ui.zip \
-
 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty \
 	&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') \
 	&& rm /tmp/dpkg.*
@@ -42,10 +40,8 @@ RUN apt-get update \
     && apt-get install -y --no-install-recommends git golang ca-certificates build-essential \
     && apt-get clean \
     && rm -rf /var/lib/apt/lists \
-
 	&& go get -v github.com/hashicorp/consul \
 	&& mv $GOPATH/bin/consul /usr/bin/consul \
-
 	&& dpkg -l | awk '/^ii/ {print $2}' > /tmp/dpkg.dirty \
 	&& apt-get remove --purge -y $(diff /tmp/dpkg.clean /tmp/dpkg.dirty | awk '/^>/ {print $2}') \
 	&& rm /tmp/dpkg.* \
diff --git a/builder/dockerfile/parser/testfiles/continueIndent/Dockerfile b/builder/dockerfile/parser/testfiles/continueIndent/Dockerfile
@@ -27,8 +27,5 @@ bye\
 frog
 
 RUN echo hello \
-# this is a comment
-
-# this is a comment with a blank line surrounding it
-
-this is some more useful stuff
+# this is a comment that breaks escape continuation
+RUN echo this is some more useful stuff
diff --git a/builder/dockerfile/parser/testfiles/continueIndent/result b/builder/dockerfile/parser/testfiles/continueIndent/result
@@ -6,4 +6,5 @@
 (run "echo hi   world  goodnight")
 (run "echo goodbyefrog")
 (run "echo goodbyefrog")
-(run "echo hello this is some more useful stuff")
+(run "echo hello")
+(run "echo this is some more useful stuff")
diff --git a/builder/dockerfile/parser/testfiles/empty-line-after-escape/Dockerfile b/builder/dockerfile/parser/testfiles/empty-line-after-escape/Dockerfile
@@ -0,0 +1,15 @@
+FROM busybox
+
+# The following will create two instructions
+# `Run foo`
+# `bar`
+# because empty line will break the escape.
+# The parser will generate the following:
+# (from "busybox")
+# (run "foo")
+# (bar "")
+# And `bar` will return an error instruction later
+# Note: Parse() will not immediately error out.
+RUN foo \
+
+bar
diff --git a/builder/dockerfile/parser/testfiles/empty-line-after-escape/result b/builder/dockerfile/parser/testfiles/empty-line-after-escape/result
@@ -0,0 +1,3 @@
+(from "busybox")
+(run "foo")
+(bar "")
diff --git a/builder/dockerfile/parser/testfiles/escapes/Dockerfile b/builder/dockerfile/parser/testfiles/escapes/Dockerfile
@@ -6,9 +6,7 @@ RUN apt-get \update && \
 ADD \conf\\" /.znc
 
 RUN foo \
-
 bar \
-
 baz
 
 CMD [ "\/usr\\\"/bin/znc", "-f", "-r" ]
diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go
@@ -3610,8 +3610,8 @@ RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/do
 
 # Switch back to root and double check that worked exactly as we might expect it to
 USER root
+# Add a "supplementary" group for our dockerio user
 RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '0:0/root:root/0 10:root wheel' ] && \
-	# Add a "supplementary" group for our dockerio user \
 	echo 'supplementary:x:1002:dockerio' >> /etc/group
 
 # ... and then go verify that we get it like we expect
@@ -7139,3 +7139,38 @@ func (s *DockerSuite) TestBuildNetContainer(c *check.C) {
 	host, _ := dockerCmd(c, "run", "testbuildnetcontainer", "cat", "/otherhost")
 	c.Assert(strings.TrimSpace(host), check.Equals, "foobar")
 }
+
+// Test case for #24693
+func (s *DockerSuite) TestBuildRunEmptyLineAfterEscape(c *check.C) {
+	name := "testbuildemptylineafterescape"
+	_, out, err := buildImageWithOut(name,
+		`
+FROM busybox
+RUN echo x \
+
+RUN echo y
+RUN echo z
+# Comment requires the '#' to start from position 1
+# RUN echo w
+`, true)
+	c.Assert(err, checker.IsNil)
+	c.Assert(out, checker.Contains, "Step 1/4 : FROM busybox")
+	c.Assert(out, checker.Contains, "Step 2/4 : RUN echo x")
+	c.Assert(out, checker.Contains, "Step 3/4 : RUN echo y")
+	c.Assert(out, checker.Contains, "Step 4/4 : RUN echo z")
+
+	// With comment, see #24693
+	name = "testbuildcommentandemptylineafterescape"
+	_, out, err = buildImageWithOut(name,
+		`
+FROM busybox
+RUN echo grafana && \
+    echo raintank \
+#echo env-load
+RUN echo vegeta
+`, true)
+	c.Assert(err, checker.IsNil)
+	c.Assert(out, checker.Contains, "Step 1/3 : FROM busybox")
+	c.Assert(out, checker.Contains, "Step 2/3 : RUN echo grafana &&     echo raintank")
+	c.Assert(out, checker.Contains, "Step 3/3 : RUN echo vegeta")
+}

Original file line number	Diff line number	Diff line change
`@@ -150,8 +150,8 @@ func TestLineInformation(t *testing.T) {`
`150`	`150`	`t.Fatalf("Error parsing dockerfile %s: %v", testFileLineInfo, err)`
`151`	`151`	`}`
`152`	`152`
`153`		`- if ast.StartLine != 5 \|\| ast.EndLine != 31 {`
`154`		`- fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 31, ast.StartLine, ast.EndLine)`
	`153`	`+ if ast.StartLine != 5 \|\| ast.EndLine != 27 {`
	`154`	`+ fmt.Fprintf(os.Stderr, "Wrong root line information: expected(%d-%d), actual(%d-%d)\n", 5, 27, ast.StartLine, ast.EndLine)`
`155`	`155`	`t.Fatalf("Root line information doesn't match result.")`
`156`	`156`	`}`
`157`	`157`	`if len(ast.Children) != 3 {`
`@@ -161,7 +161,7 @@ func TestLineInformation(t *testing.T) {`
`161`	`161`	`expected := [][]int{`
`162`	`162`	`{5, 5},`
`163`	`163`	`{11, 12},`
`164`		`- {17, 31},`
	`164`	`+ {17, 27},`
`165`	`165`	`}`
`166`	`166`	`for i, child := range ast.Children {`
`167`	`167`	`if child.StartLine != expected[i][0] \|\| child.EndLine != expected[i][1] {`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+(from "busybox")`
	`2`	`+(run "foo")`
	`3`	`+(bar "")`