Dockerfile: fix parsing of trailing backslash #1559
Conversation
|
Isn't this why was added? |
|
While Would help not having to escape backslashes, and help with PowerShell scripts (which, I think was the main purpose), I think it would still need to handle escaped line-continuation markers at the end (e.g. an env-var with a trailing ``` should also be possible, correct?) |
| ENV A path | ||
| ENV B another\\path | ||
| ENV C trailing\\backslash\\ | ||
| ENV D This gets appended to C |
There was a problem hiding this comment.
@thaJeztah this is a bit confusing as the text says it gets appended to C, but it doesn't and it shouldn't, right?
There was a problem hiding this comment.
Good one, yes, I copied the "reproducer" example, but I guess it would be better to change to "This should not be appended to C" in the test
| return errors.Errorf("invalid escape token '%s' does not match ` or \\", s) | ||
| } | ||
| d.escapeToken = rune(s[0]) | ||
| d.lineContinuationRegex = regexp.MustCompile(`\` + s + `[ \t]*$`) | ||
| d.lineContinuationRegex = regexp.MustCompile(`([^\` + s + `])\` + s + `[ \t]*$|^\` + s + `[ \t]*$`) |
There was a problem hiding this comment.
Shouldn't using optional block suffice?
regexp.MustCompile(`([^\` + s + `])?\` + s + `[ \t]*$`)
There was a problem hiding this comment.
Will have to give that a test 🤔 it's been a while since I wrote this 😂
There was a problem hiding this comment.
Ok, I recall why it was needed; the second branch is needed for line-continuation symbols on their own line, which would otherwise be handled incorrectly.
Let me add some additional comments to this block, as there's also one corner-case we can't match easily (line-continuation preceded by an escaped escape-token, e.g.;
RUN echo hello\\\
worldWhich should result in echo hello\\world, but we currently don't match
|
ping @thaJeztah if you want to get this in v0.8 |
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
fix_trailing_escape
branch
from
d50f08b
to
de0e30c
Compare
If the line-continuation marker (`\`) is escaped, it should not be treated as such, but as a literal backslash. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
fix_trailing_escape
branch
from
de0e30c
to
cc7a3de
Compare
|
@tiborvass @tonistiigi updated; PTAL |
This old test is failing after an edge-case change in dockerfile parsing considered a bugfix: moby/buildkit#1559 Instead of fixing the test, I suggest removing it as there are already tests for it in BuildKit. Signed-off-by: Tibor Vass <tibor@docker.com>
This old test is failing after an edge-case change in dockerfile parsing considered a bugfix: moby/buildkit#1559 Instead of fixing the test, I suggest removing it as there are already tests for it in BuildKit. Signed-off-by: Tibor Vass <tibor@docker.com> Upstream-commit: beff0a5f2c62e655260c09697680c25a3e9a7ce1 Component: engine
fixes docker/for-win#5254
If the line-continuation marker (
\) is escaped, it should not be treated as such, but as a literal backslash, for example, the following should produce 5ENVinstructions: