LGTM

actually, looks like tests are failing, need to look more into this

ping @duglin

Looks like tests are failing, think there may be more to look into

Don't have time right now to full review but I do recall that we had some really funky rules w.r.t. auto-continuing lines when they're blank. Wouldn't surprise me if the failures are related to that. And the last time we thought about "fixing" this we didn't because we didn't want to break existing Dockerfiles that might count on this odd behavior.

Yes, I was wondering, perhaps we strip the continuation character before we arrive here, in case this may see \ as an empty line (but I should look as well)

Thanks @duglin @thaJeztah. The failed test has the following file:

RUN apt-get \update && \
  apt-get \"install znc -y
ADD \conf\\" /.znc

RUN foo \

bar \

baz

CMD [ "\/usr\\\"/bin/znc", "-f", "-r" ]

and expect the result to be:

RUN foo bar baz

In other words, the previous behavior of docker considers empty lines (and comments) as continuation. And you could place many empty lines (or comment) in between:

FROM busybox
RUN echo foo \




RUN bar


CMD [ "\/usr\\\"/bin/znc", "-f", "-r" ]

The result of the above would be one command RUN:

RUN echo foo RUN bar
CMD [ "\/usr\\\"/bin/znc", "-f", "-r" ]

I think it makes sense to at least not allow empty lines to continue as it is quite misleading. People may place an escape by mistake, then it will continue forever, no matter how many empty lines have passed.

We could keep the comment for continuation to not break many existing behaviors, i.e.,

FROM busybox
RUN  \
# my comment
# some other comment 
# and some script, etc
echo bar

will be

RUN echo bar

and the old behavior is maintained in this case.

I updated pull request for the test cases to allow the test pass. But let me know the best way to handle them in these situations, or we could leave the old behavior alone if needed.

Sounds good, but we may want to look at the history of that test; if it was testing a specific case, or just confirming "broken" behavior

I think that the bug is not with our treatment of multi-line commands, but with comments:

RUN cd /go/src/github.com/grafana && \
    git clone https://github.com/raintank/grafana-api-golang-client.git && \
    cd grafana-api-golang-client && \
    git checkout raintank #&& \
    #go get github.com/raintank/env-load

RUN go get github.com/tsenart/vegeta
RUN go get github.com/raintank/inspect/inspect-es

Currently the #go get github.com/raintank/env-load line seems to be stripped but the trailing #&& \ is not on the previous line. We should fix how we strip comments, not remove a (questionable) feature of the parser as a hotfix. For example, will the changed code fix this case (from reading it, I don't think it will):

RUN cd /go/src/github.com/grafana && \
    git clone https://github.com/raintank/grafana-api-golang-client.git && \
    cd grafana-api-golang-client && \
    git checkout raintank #&& \
    #go get github.com/raintank/env-load
RUN go get github.com/tsenart/vegeta

@cyphar yes, it surprised be that the #go get line is stripped, because the Dockerfile syntax says that comments must begin at position 1, and cannot have spaces before them.

If we stick to that then, in this case, the #go get line would be treated as part of the RUN, and just concatenation with the previous line.

I'm not sure why we ever wanted to support the blank lines though, or if that was a bug in the past, that we decided to keep supporting

TBH, I do actually prefer more lax commenting. It's weird to me (coming from shell scripts) that #&& \ isn't stripped. But definitely there's an inconsistency here that we should deal with.

Looks like the blank line behavior was introduced here; #8580

@cyphar it's because everything inside the RUN should be passed as-is to the shell inside the comtainer, basically no processing should take place, especially since we now have a SHELL directive, and # may not even be a comment in the shell that is selected

Thanks @cyphar @thaJeztah. Not knowing enough history of docker, I can only guess the reason comment line is continued might be related to the ambiguity when # and \ exist at the same line.

For example, in case of

\ # Should the comment be removed first so that `\` is line escape,
\ # or should we consider this is the escape of space ` ` followed by `#` and comment?

and

# Should the end of line `\` be considered as part of the comment , \
# or the end of line `\` be considered as an escape and not part of the comment ? \

The outputs of the above likely depends on if we parse from the left, or parse from the right.

So continuation with comment lines avoids the issue mentioned above.

Again, I don't know enough history of docker so I could be totally wrong here.

On the other hand, I feel like the continuation on empty line is quite misleading. The issue is that, if the user adds an escape \ at one line, then the user will be forced to adds something other than comment lines or empty lines to stop the escape. And this could easily getting into the next RUN XXX if not careful.

I personally don't like the fact that we are acting differently to shells with \ continuation. But to be clear, my point was that I don't think this patch fixes the underlying problem (if you remove the newline between the two RUNs I think it will still act incorrectly). We need to fix how comments are dealt with -- and fixing up line continuation is a separate thing that we should probably also do.

Also, please note that we don't actually support comments at the end of lines. We only support comments if the # is the first character in the line. Is it a bit of a weird restriction? Sure, but then again we aren't sure what the shell is during build time.

What are we doing with this?

my vote is to "fix" this but since it has been this way for ages (intentionally) we'd need agreement to break backwards compatibility.

to be clear, I'd like to see two fixes:

1. make blank lines end a continuation sequence
2. allow for blanks in front of a comment (#) - remove requirement that "#" be only in the first column

While I think supporting comments at the end of lines would be good, that's a bigger change that would be trickier since we don't have a formal "end of statement" in Dockerfiles so knowing when "#" isn't part of the RUN command itself could be tricky. So for now I think doing the two things above would be a good first step.

@thaJeztah that's fine - its just that this PR migrated a bit into fixing some of the oddities in our parser so I tossed in one that was related. Either way is fine with me.

@duglin thanks - I wasn't sure if everyone would be on board with changing that, and potentially block this PR (which is already quite old) from being merged.

@yongtang could you keep the change for comments out of this PR, that way I hope we'd be able to get this moving forward quicker. I'm open to doing it in a separate PR so that it can be discussed separately

Thanks @duglin @thaJeztah. The PR has been updated so that now only ^# is considered as the comment. Please take a look and let me know if there are any issues.

The current version seems to correctly break the continuation sequence.

Provided that we're still ok to have a subsequent PR to fix the rest, LGTM

ping @duglin @thaJeztah

The PR has been rebased to fix the merge conflict. All tests passed. Ping @duglin @thaJeztah

LGTM

This is a pretty huge breaking change for Dockerfile parsing -- are we sure we're committed to making this change?

The failure mode is pretty awful too; here's an example error from trying to build one of the php official image variants:

$ docker version
Client:
 Version:      1.13.0-rc2
 API version:  1.25
 Go version:   go1.7.3
 Git commit:   1f9b3ef
 Built:        Wed Nov 23 17:40:58 2016
 OS/Arch:      linux/amd64

Server:
 Version:             1.13.0-rc2
 API version:         1.25
 Minimum API version: 1.12
 Go version:          go1.7.3
 Git commit:          1f9b3ef
 Built:               Wed Nov 23 17:40:58 2016
 OS/Arch:             linux/amd64
 Experimental:        false
$ docker build 7.0/alpine
Sending build context to Docker daemon 13.82 kB
Error response from daemon: Unknown instruction: --ENABLE-FTP

For reference, here's the exact line that's failing on: https://github.com/docker-library/php/blob/7d058f79e6fdf7b8e97d5004d1098c436d348a47/7.0/alpine/Dockerfile#L97

@tianon @duglin
Before this PR, the behavior of parser was (See #24725 (comment)):

• Space is continued as long as it begins with \:

FROM busybox
RUN echo foo \




RUN bar

CMD [ "\/usr\\\"/bin/znc", "-f", "-r" ]

will result in (many empty lines will continue):

RUN echo foo RUN bar
CMD [ "\/usr\\\"/bin/znc", "-f", "-r" ]

• Comment is continued as long as it begins with \:

FROM busybox
RUN  \
# my comment
# some other comment 
# and some script, etc
echo bar

will result in (may comment lines will continue):

RUN echo bar

This PR changed both based on discussions.

If there are concerns with "no-continuation-for-comments", maybe we could revert back?

We probably could keep the other changes "no-continuation-for-empty-lines" alone as it is confusing. The reason is that, in case an user have placed a \ at the end of one line, then this \ will continue even if many empty lines has been added.

Alternatively, I am wondering if it makes sense to add a VERSION for Dockerfile? This should allows for backward-compatibility in the future.

@yongtang was discussing with Tianon; problem is here; https://github.com/docker/docker/pull/24725/files#diff-a70e690b6dc1bcdbae36a5f54c868bbeR183

It breaks on a comment line, but should probably detect if it's a comment-only line, and if so, continue

Due to the previous behavior, inside RUN, that means also if there's whitespace before the #

Thanks @thaJeztah. Let me take a look and see if I could provide a quick fix for it.

The behaviour in 1.13-RC2 has broken all of my Dockerfiles as I depend on both spaces and comments being allowed after continuations in the RUN and ENV instructions. As a user, this is a huge and unwelcome breaking change for 1.13 if it stays.

@kinghuang we are aware of this change, and will address before 1.13 GA. The discussion is taking place here; #29005, and is currently focussed on the "blank lines", perhaps you can add a minimal example of this from your Dockerfile.

I'll lock the discussion here, so that the discussion does not take place on multiple places, but you're welcome to join in the discussion on #29005