RUN directive parsing buggy

[geekflyer]

I'm noticing a bunch of issues with makisu not being able to parse certain RUN directives that just work fine with docker build (18.06.1-ce).

If that happens I get an error message similar to this:

2018/11/19 00:25:14 Starting makisu (version=v0.1.0-13-gac3b5bb)
{"level":"info","ts":1542587114.7364843,"msg":"Using build context: /makisu-context"}
2018/11/19 00:25:14 Command failure: failed to get dockerfile: failed to parse dockerfile: failed to create new directive (line 16): failed to parse ECHO directive with args 'foo': Unsupported directive type

Here are examples which work with docker build, but not with makisu:

1. example:

RUN \ 
  echo foo

NOTICE: There is an empty space after the \ which makes makiso choke.

2. example:

RUN \
  # some comment that makisu doesn't like
  echo foo

This example doesn't contain an empty space after \ but it contains a # comment that makes makiso choke as well.

[yiranwang52]

bash doesn't allow either case, so Makisu doesn't allow them either.
Docker's parser probably didn't allow these intentionally, but simply overlooked them.
Cases like this are why we decided to reimplement our own dockerfile parser.

[geekflyer]

After testing this myself in bash, I believe your assessment is only partially correct:

1. spaces after \ are indeed not allowed.
2. comments in between are allowed in bash:

e.g. run the following script:

#!/usr/bin/env bash

echo foo && \
  # a comment
  echo bar

Works fine for me and prints:

foo
bar

bash version: 3.2.57(1)-release
OS-X

[yiranwang52]

I think it depends.
This is allowed in bash:

echo foo && \
  # a comment
  echo bar

But the following isn't:

echo \
  # a comment
  bar

I think that's because first example is equivalent of:

echo foo && # a comment
echo bar

which are 2 separate lines and is fine for bash.
However in a dockerfile, a new line without preceding \ needs to start with RUN or other keywords, so this has to be rejected.

[geekflyer]

Honestly I don't get it.
Shouldn't you thrive to make your RUN directive either docker or bash compatible instead of creating your own makisu Dockerfile flavor which is not compatible with either one?

However in a dockerfile, a new line needs to start with RUN or other keywords...

How so? Plenty of newlines in dockerfiles in the web: https://github.com/nodejs/docker-node/blob/master/10/stretch/Dockerfile#L27 .

Not allowing comments in multi-line RUN directives is a pretty big departure from Dockerfile syntax where having large multi-line RUN commands is common in order to reduce the layer count.

I'm aware that multi-line docker RUN directives may become somewhat obsolete when replaced with smart use of #!COMMIT but not allowing comments in RUN makes adoption / migration to makisu more painful than it could be.

If you are absolutely sure that you intentionally want to disallow or discourage multi-line RUN statements with comments in between I think it would be helpful to at least document this caveat / difference to regular Dockerfile's in the makisu README or throw a very explicit error message about it in the parser.

[yiranwang52]

Sorry if i wasn't clear. I updated my comment.
I meant these 2 cases are equivalent in bash:

echo foo && \
  # a comment
  echo bar

echo foo && # a comment
echo bar

So the \ essentially doesn't do anything. It's still fine for bash, because it's interpreted as 2 separate commands.
But if you put that in dockerfile, it's as if you have the following dockerfile, which certainly shouldn't be allowed:

RUN echo foo && 
echo bar

Makisu supports normal usage of \ (without comments in between):

RUN echo foo && \
  echo bar

It's just our support of \ is more similar to bash, which only means "escape next character".
I think this is more like a bug in docker's parser - and I think it's better to explicitly break such scenarios than re-implementing a bug.

[yiranwang52]

There are more users asking for better compatibility with Docker, so we decided to fix it. See #108.

[h4wkmoon]

Hi, sorry to unearth an old issue.

This directive is not parsed correctly.
RUN sed -i 's/#PermitRootLogin yes/PermitRootLogin yes/' /etc/ssh/sshd_config
(I know I shouldn't really not do ssh to containers. Not much choice here, and it is not the topic)

Result is :
RUN sed -i 's/

or did I missed something ?

[h4wkmoon]

oups.
#287
Sorry.