Fix very minor continuation bugs for better coverage

There were some very minor/subtle bugs in how I implemented continuation that wouldn't affect any real-world parsing we did, but still bothered me because I'm me.  This fixes them (and further increases test coverage as a result).
This commit is contained in:
Tianon Gravi 2025-01-09 15:40:00 -08:00
parent 0e00438cf2
commit 7ddf2bef73
2 changed files with 62 additions and 6 deletions

View File

@ -5,6 +5,7 @@ import (
"io" "io"
"strconv" "strconv"
"strings" "strings"
"unicode"
) )
type Metadata struct { type Metadata struct {
@ -31,10 +32,11 @@ func ParseReader(dockerfile io.Reader) (*Metadata, error) {
line := strings.TrimSpace(scanner.Text()) line := strings.TrimSpace(scanner.Text())
if line == "" { if line == "" {
// ignore blank lines // ignore straight up blank lines (no complexity)
continue continue
} }
// (we can't have a comment that ends in a continuation line - that's not continuation, that's part of the comment)
if line[0] == '#' { if line[0] == '#' {
// TODO handle "escape" parser directive // TODO handle "escape" parser directive
// TODO handle "syntax" parser directive -- explode appropriately (since custom syntax invalidates our Dockerfile parsing) // TODO handle "syntax" parser directive -- explode appropriately (since custom syntax invalidates our Dockerfile parsing)
@ -44,20 +46,36 @@ func ParseReader(dockerfile io.Reader) (*Metadata, error) {
// handle line continuations // handle line continuations
// (TODO see note above regarding "escape" parser directive) // (TODO see note above regarding "escape" parser directive)
for line[len(line)-1] == '\\' && scanner.Scan() { for line[len(line)-1] == '\\' {
nextLine := strings.TrimSpace(scanner.Text()) if !scanner.Scan() {
if nextLine == "" || nextLine[0] == '#' { line = line[0:len(line)-1]
// ignore blank lines and comments break
}
// "strings.TrimRightFunc(IsSpace)" because whitespace *after* the escape character is supported and ignored 🙈
nextLine := strings.TrimRightFunc(scanner.Text(), unicode.IsSpace)
if nextLine == "" { // if it's all space, TrimRight will be TrimSpace 😏
// ignore "empty continuation" lines (https://github.com/moby/moby/pull/33719)
continue
}
if strings.TrimLeftFunc(nextLine, unicode.IsSpace)[0] == '#' {
// ignore comments inside continuation (https://github.com/moby/moby/issues/29005)
continue continue
} }
line = line[0:len(line)-1] + nextLine line = line[0:len(line)-1] + nextLine
} }
// TODO *technically* a line like " RUN echo hi " should be parsed as "RUN" "echo hi" (cut off instruction, then the rest of the line with TrimSpace), but for our needs "strings.Fields" is good enough for now
// line = strings.TrimSpace(line) // (emulated below; "strings.Fields" does essentially the same exact thing so we don't need to do it explicitly here too)
fields := strings.Fields(line) fields := strings.Fields(line)
if len(fields) < 1 { if len(fields) < 1 {
// must be a much more complex empty line?? // ignore empty lines
continue continue
} }
instruction := strings.ToUpper(fields[0]) instruction := strings.ToUpper(fields[0])
// TODO balk at ARG / $ in from values // TODO balk at ARG / $ in from values

View File

@ -75,6 +75,44 @@ func TestParse(t *testing.T) {
Froms: []string{"bash:latest", "busybox:uclibc", "bash:5", "bash:latest", "scratch", "bash:latest", "bash:5", "bash:latest"}, Froms: []string{"bash:latest", "busybox:uclibc", "bash:5", "bash:latest", "scratch", "bash:latest", "bash:5", "bash:latest"},
}, },
}, },
{
name: "empty continuations",
dockerfile: `
\
\
\
\
\
\
`,
},
{
name: "continuation edge cases",
dockerfile: `
# continuation does not apply to this comment \
FROM scratch
# but everything below this is part of a single continuation
FROM\
\
\
\
\
# comments inside are fine
# and really yucky empty lines:
\
\
scratch\
`,
metadata: dockerfile.Metadata{
Froms: []string{"scratch", "scratch"},
},
},
{ {
// TODO is this even something that's supported by classic builder/buildkit? (Tianon *thinks* it was supported once, but maybe he's misremembering and it's never been a thing Dockerfiles, only docker build --target=N ?) // TODO is this even something that's supported by classic builder/buildkit? (Tianon *thinks* it was supported once, but maybe he's misremembering and it's never been a thing Dockerfiles, only docker build --target=N ?)
name: "numbered stages", name: "numbered stages",