Warn/deprecate continuing on empty lines in `Dockerfile` #29161

yongtang · 2016-12-06T03:09:40Z

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.

Here are some of the examples:

No warning for comments continuation

ubuntu@ubuntu:~/tmp$ cat Dockerfile 
FROM busybox
RUN echo \
# comment
hi
ubuntu@ubuntu:~/tmp$ sudo docker build -t test .
Sending build context to Docker daemon  11.2 MB
Step 1/2 : FROM busybox
 ---> e02e811dd08f
Step 2/2 : RUN echo hi
 ---> Running in 982812e845b2
hi
 ---> 58c355115a35
Removing intermediate container 982812e845b2
Successfully built 58c355115a35
ubuntu@ubuntu:~/tmp$

Warning for empty line continuation:

ubuntu@ubuntu:~/tmp$ cat Dockerfile 
FROM busybox
RUN echo "foo" \
#&& echo "bar";



# Do something else
RUN echo "something else"

RUN echo "end"
ubuntu@ubuntu:~/tmp$ sudo docker build -t test .
Sending build context to Docker daemon  11.2 MB
[WARNING]: Empty lines detected in the following instruction:
[WARNING]:   RUN echo "foo" \
[WARNING]:   #&& echo "bar";
[WARNING]:   
[WARNING]:   
[WARNING]:   
[WARNING]:   # Do something else
[WARNING]:   RUN echo "something else"
[WARNING]: Empty lines inside a RUN instruction is deprecated, and will be removed in Docker 1.16.
Step 1/3 : FROM busybox
 ---> e02e811dd08f
Step 2/3 : RUN echo "foo" RUN echo "something else"
 ---> Running in 0c95c73c0e6b
foo RUN echo something else
 ---> e94a89843d4b
Removing intermediate container 0c95c73c0e6b
Step 3/3 : RUN echo "end"
 ---> Running in 22d749190d5d
end
 ---> 82e486951d62
Removing intermediate container 22d749190d5d
Successfully built 82e486951d62

No warning as long as there are escaping symbols for each line:

ubuntu@ubuntu:~/tmp$ cat Dockerfile 
FROM busybox
RUN echo "foo"; \
\
# This is a comment
\
    # So is this
    \
    echo "and this is part of the same RUN"


RUN echo "this is the next RUN"
ubuntu@ubuntu:~/tmp$ sudo docker build -t test .
[sudo] password for ubuntu: 
Sending build context to Docker daemon  11.2 MB
Step 1/3 : FROM busybox
 ---> e02e811dd08f
Step 2/3 : RUN echo "foo";         echo "and this is part of the same RUN"
 ---> Running in cf785c61ed05
foo
and this is part of the same RUN
 ---> e375992a8a57
Removing intermediate container cf785c61ed05
Step 3/3 : RUN echo "this is the next RUN"
 ---> Running in cba814794d62
this is the next RUN
 ---> 712c017c8ef1
Removing intermediate container cba814794d62
Successfully built 712c017c8ef1
ubuntu@ubuntu:~/tmp$

Signed-off-by: Yong Tang yong.tang.github@outlook.com

lowenna · 2016-12-06T04:35:06Z

How does this affect the #escape= directive? The recommendation was to ensure there WAS a blank line between the #escape= and the FROM statement? (Sorry, not looked in detail)

vdemeester · 2016-12-06T09:56:31Z

/cc @thaJeztah @duglin
I did put 1.14 milestone on it 👼

thaJeztah · 2016-12-06T10:13:24Z

How does this affect the #escape= directive? The recommendation was to ensure there WAS a blank line between the #escape= and the FROM statement? (Sorry, not looked in detail)

@jhowardmsft this is only for blank lines inside a RUN statement, I.e., of someone uses a blank line without using \ (or backtic in PowerShell) as a line continuation marker

duglin · 2016-12-06T13:12:03Z

SGTM

I can't seem to find the spot where we agreed on the plan... what was the decision for comments in continuation lines? Deprecate those too or just leave them?

thaJeztah · 2016-12-06T14:43:56Z

I'm not sure the current implementation is correct. Should have a look if we can do it otherwise, but just printing a list of "Skipping empty line ...." before the docker build may not provide enough information to the user to resolve the issue.

Haven't had time to look into it further, but thought to just leave a comment for now 😊

yongtang · 2016-12-07T01:10:51Z

I think one issue is that, currently the output of build always shows the instruction in one line (after processing). For example,

FROM busybox
RUN echo "foo" \
#&& echo "bar";


# Do something else
RUN echo "something else"
ubuntu@ubuntu:~/tmp$ sudo docker build -t test .
Sending build context to Docker daemon  11.2 MB
Step 1/2 : FROM busybox
 ---> e02e811dd08f
Step 2/2 : RUN echo "foo" RUN echo "something else"
 ---> Running in 93c62c013a56
foo RUN echo something else
 ---> 03b1c6850531
Removing intermediate container 93c62c013a56
Successfully built 03b1c6850531
ubuntu@ubuntu:~/tmp$

In the above example, we only see:

Step 2/2 : RUN echo "foo" RUN echo "something else"

which might be confusing initially.

I think for the generated warning, it might make sense to output the instruction in the original format, e.g.,

[WARNING]: Empty lines detected in:
[WARNING]: [INSTRUCTION] RUN echo "foo" \
[WARNING]: [INSTRUCTION] #&& echo "bar";
[WARNING]: [INSTRUCTION] 
[WARNING]: [INSTRUCTION] 
[WARNING]: [INSTRUCTION] # Do something else
[WARNING]: [INSTRUCTION] RUN echo "something else"
[WARNING]: Empty lines inside an instruction is deprecated, and will be removed in Docker 1.16.

yongtang · 2016-12-12T01:58:38Z

The PR has been updated and the warning message has been updated.

Below is the new output to give user a better idea of which instruction (multiple lines) could be ambiguous:

ubuntu@ubuntu:~/tmp$ cat Dockerfile 
FROM busybox
RUN echo "foo" \
#&& echo "bar";



# Do something else
RUN echo "something else"

RUN echo "end"
ubuntu@ubuntu:~/tmp$ sudo docker build -t test .
Sending build context to Docker daemon  11.2 MB
[WARNING]: Empty lines detected in the following instruction:
[WARNING]:   RUN echo "foo" \
[WARNING]:   #&& echo "bar";
[WARNING]:   
[WARNING]:   
[WARNING]:   
[WARNING]:   # Do something else
[WARNING]:   RUN echo "something else"
[WARNING]: Empty lines inside a RUN instruction is deprecated, and will be removed in Docker 1.16.
Step 1/3 : FROM busybox
 ---> e02e811dd08f
Step 2/3 : RUN echo "foo" RUN echo "something else"
 ---> Running in 0c95c73c0e6b
foo RUN echo something else
 ---> e94a89843d4b
Removing intermediate container 0c95c73c0e6b
Step 3/3 : RUN echo "end"
 ---> Running in 22d749190d5d
end
 ---> 82e486951d62
Removing intermediate container 22d749190d5d
Successfully built 82e486951d62

yajo · 2017-01-20T08:17:38Z

Ain't it enough for those corner cases to escape it like \#&& echo "bar" and let the current behavior as it is?

yongtang · 2017-01-20T19:44:38Z

@yajo The issue is fairly complicated. One issue is that, under current behavior, a combination of escape with comment will result in the continuation beyond the last comment.

Below is the example:

FROM busybox
RUN echo "foo" \
#&& echo "bar";



# Do something else
RUN echo "something else"

The above Dockerfile resulting in:

Step 1 : FROM busybox
 ---> 47bcc53f74dc
Step 2 : RUN echo "foo" RUN echo "something else"
 ---> Running in 991b76486899
foo RUN echo something else
 ---> d488a4c21b22

See issue #24693 for reference.

yajo · 2017-01-23T10:04:43Z

Yeah, but that's what I'd expect given that Dockerfile. I mean that you should fix it like this:

FROM busybox
RUN echo "foo" \
\#&& echo "bar";



# Do something else
RUN echo "something else"

... isn't it?

asottile · 2017-01-30T04:04:31Z

builder/dockerfile/parser/parser.go

+				for _, line := range strings.Split(scannedLine, "\n") {
+					fmt.Fprintf(stderr, "[WARNING]:   %s\n", line)
+				}
+				fmt.Fprintf(stderr, "[WARNING]: Empty lines inside a RUN instruction is deprecated, and will be removed in Docker 1.16.\n")


fwiw, this now mismatches with the version listed in deprecated.md

@yongtang per the discussion, perhaps we should remove this line (or at least remove the "version"). We discussed this in the maintainers meeting, and we may have to accept that this behaviour is "bad" (hence the warnings to motivate people to change it), but that removing would be too much of a backward breaking change, that we'd likely never be able to actually remove it.

LK4D4 · 2017-01-30T21:41:40Z

@duglin @thaJeztah did we decide on design here?

vdemeester

Design makes sense to me but I wonder if we are going to really remove this anytime soon (I mean, it's gonna break a lot of Dockerfile if they don't update).

/ping @tiborvass @vieux and @tonistiigi for inputs 👼

thaJeztah · 2017-03-16T19:26:11Z

We discussed this in the maintainers meeting and think the warning is a great thing to have, but we're still a bit worried about actually deprecating it. We should, but not sure how long to take before removing it.

Adding a version to the dockerfile format would help 😇 #4907

thaJeztah · 2017-03-16T19:26:54Z

We also should have some tests for these 😄

alexellis · 2017-03-16T19:27:52Z

Would be great to see a unit test for either scenario - a test with a Dockerfile that should and shouldn't error.

rcjsuen · 2017-03-22T23:31:04Z

Would love to see tests for both scenarios as well. They would be of great help to those of us writing Dockerfile editors and/or linters.

vdemeester · 2017-03-26T10:19:15Z

ping @yongtang 👼

tonistiigi · 2017-04-03T20:38:44Z

Design LGTM

vdemeester · 2017-04-03T20:41:56Z

@yongtang needs a rebase 👼

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 <yong.tang.github@outlook.com>

yongtang · 2017-04-04T01:11:57Z

Rebased to fix merge conflict.

dnephin

I wish we had a better way to pass in stderr, it doesn't really feel appropriate as an argument to Parser right now, but I'm not seeing an easy way.

ParserOptions{} might be good if we start adding more arguments.

dnephin · 2017-04-04T15:16:48Z

builder/dockerfile/parser/parser.go

@@ -171,11 +171,20 @@ func Parse(rwc io.Reader, d *Directive) (*Node, error) {
 		}
 		startLine := currentLine

+		warning := false


warning is a bit ambiguous, in this context it could mean many things.

How about blankLineWithinInstruction ?

dnephin · 2017-04-04T15:18:32Z

builder/dockerfile/parser/parser.go

 		if line != "" && child == nil {
 			for scanner.Scan() {
 				newline := scanner.Text()
+				// Keep the original content of the current instruction
+				scannedLine += "\n" + newline


Instead of re-using this variable that was intended to be just an intermediary in a calculation , which also requires both string concatenation and splitting later on, how about storing the lines in a slice and avoiding the concat+split?

dnephin · 2017-04-04T15:29:22Z

builder/dockerfile/parser/parser.go

+				for _, line := range strings.Split(scannedLine, "\n") {
+					fmt.Fprintf(stderr, "[WARNING]:   %s\n", line)
+				}
+				fmt.Fprintf(stderr, "[WARNING]: Empty lines inside a RUN instruction is deprecated, and will be removed in Docker 1.16.\n")


This would probably be better off in a new function, it's not directly related to parsing.

if blankLineWithinInstruction { warnOnBlankLinWithinInstruction(stderr, scannedLines) }

dnephin · 2017-04-12T20:17:00Z

I think we're going to be changing the result value of Parse() in another PR (#32580) to be a struct. That will allow us to return Warnings as part of the response, and avoid having to pass in stderr here.

I'll ping you when that PR opens

thaJeztah · 2017-04-13T16:49:07Z

I see #32580 was opened

cpuguy83 · 2017-05-19T15:03:29Z

This requires a pretty major rebase.

thaJeztah · 2017-06-15T18:04:17Z

@yongtang let me close this for now, but let me know if you want to work on this again

dnephin · 2017-06-15T18:07:44Z

I'll look at carrying this in a new PR.

dnephin · 2017-06-16T22:10:03Z

Opened #33719 to carry

GordonTheTurtle added the status/0-triage label Dec 6, 2016

yongtang mentioned this pull request Dec 6, 2016

Parser in docker 1.13 breaks backwards compatibility for comments in middle of RUN statement #29005

Closed

yongtang force-pushed the 29005-warning-empty-line-in-run branch 2 times, most recently from ef2aa7f to c24fc80 Compare December 6, 2016 04:32

vdemeester added this to the 1.14.0 milestone Dec 6, 2016

vdemeester added status/1-design-review and removed status/0-triage labels Dec 6, 2016

yongtang force-pushed the 29005-warning-empty-line-in-run branch from c24fc80 to 684c150 Compare December 12, 2016 01:55

yongtang force-pushed the 29005-warning-empty-line-in-run branch 3 times, most recently from 073ec20 to 7433634 Compare December 17, 2016 20:44

yosifkit mentioned this pull request Jan 19, 2017

Add lua support. docker-library/haproxy#38

Merged

asottile reviewed Jan 30, 2017

View reviewed changes

GordonTheTurtle assigned vdemeester Feb 10, 2017

vdemeester reviewed Feb 14, 2017

View reviewed changes

thaJeztah modified the milestones: 17.05.0, 17.04.0 Mar 15, 2017

vdemeester requested a review from tonistiigi April 3, 2017 20:34

vdemeester added the area/builder label Apr 3, 2017

vdemeester added status/2-code-review and removed status/1-design-review labels Apr 3, 2017

yongtang force-pushed the 29005-warning-empty-line-in-run branch from 7433634 to 5106e0a Compare April 4, 2017 01:11

dnephin reviewed Apr 4, 2017

View reviewed changes

dnephin mentioned this pull request Apr 12, 2017

Refactor Dockerfile.parser and directive #32580

Merged

thaJeztah removed this from the 17.05.0 milestone May 12, 2017

thaJeztah closed this Jun 15, 2017

dnephin mentioned this pull request Jun 16, 2017

[Builder] Warn on empty continuation lines #33719

Merged

yongtang deleted the 29005-warning-empty-line-in-run branch February 7, 2018 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn/deprecate continuing on empty lines in `Dockerfile` #29161

Warn/deprecate continuing on empty lines in `Dockerfile` #29161

yongtang commented Dec 6, 2016 •

edited

Loading

lowenna commented Dec 6, 2016

vdemeester commented Dec 6, 2016

thaJeztah commented Dec 6, 2016

duglin commented Dec 6, 2016

thaJeztah commented Dec 6, 2016

yongtang commented Dec 7, 2016

yongtang commented Dec 12, 2016

yajo commented Jan 20, 2017

yongtang commented Jan 20, 2017

yajo commented Jan 23, 2017

asottile Jan 30, 2017

thaJeztah Apr 4, 2017

LK4D4 commented Jan 30, 2017

vdemeester left a comment

thaJeztah commented Mar 16, 2017

thaJeztah commented Mar 16, 2017

alexellis commented Mar 16, 2017

rcjsuen commented Mar 22, 2017

vdemeester commented Mar 26, 2017

tonistiigi commented Apr 3, 2017

vdemeester commented Apr 3, 2017

yongtang commented Apr 4, 2017

dnephin left a comment

dnephin Apr 4, 2017

dnephin Apr 4, 2017

dnephin Apr 4, 2017

dnephin commented Apr 12, 2017 •

edited

Loading

thaJeztah commented Apr 13, 2017

cpuguy83 commented May 19, 2017

thaJeztah commented Jun 15, 2017

dnephin commented Jun 15, 2017

dnephin commented Jun 16, 2017

Warn/deprecate continuing on empty lines in Dockerfile #29161

Warn/deprecate continuing on empty lines in Dockerfile #29161

Conversation

yongtang commented Dec 6, 2016 • edited Loading

lowenna commented Dec 6, 2016

vdemeester commented Dec 6, 2016

thaJeztah commented Dec 6, 2016

duglin commented Dec 6, 2016

thaJeztah commented Dec 6, 2016

yongtang commented Dec 7, 2016

yongtang commented Dec 12, 2016

yajo commented Jan 20, 2017

yongtang commented Jan 20, 2017

yajo commented Jan 23, 2017

asottile Jan 30, 2017

Choose a reason for hiding this comment

thaJeztah Apr 4, 2017

Choose a reason for hiding this comment

LK4D4 commented Jan 30, 2017

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Mar 16, 2017

thaJeztah commented Mar 16, 2017

alexellis commented Mar 16, 2017

rcjsuen commented Mar 22, 2017

vdemeester commented Mar 26, 2017

tonistiigi commented Apr 3, 2017

vdemeester commented Apr 3, 2017

yongtang commented Apr 4, 2017

dnephin left a comment

Choose a reason for hiding this comment

dnephin Apr 4, 2017

Choose a reason for hiding this comment

dnephin Apr 4, 2017

Choose a reason for hiding this comment

dnephin Apr 4, 2017

Choose a reason for hiding this comment

dnephin commented Apr 12, 2017 • edited Loading

thaJeztah commented Apr 13, 2017

cpuguy83 commented May 19, 2017

thaJeztah commented Jun 15, 2017

dnephin commented Jun 15, 2017

dnephin commented Jun 16, 2017

Warn/deprecate continuing on empty lines in `Dockerfile` #29161

Warn/deprecate continuing on empty lines in `Dockerfile` #29161

yongtang commented Dec 6, 2016 •

edited

Loading

dnephin commented Apr 12, 2017 •

edited

Loading