Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frontend/dockerfile: BFlags.Parse: include flag with "--" prefix in errors #5369

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

thaJeztah
Copy link
Member

frontend/dockerfile: BFlags.Parse: return earlier on "--" terminator

Minor optimization; put the check for a literal "--" first, instead of
matching for "--" as prefix. Also add some documentation to the code,
to outline why we return early.

from the POSIX Utility Syntax Guidelines;
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02

The first -- argument that is not an option-argument should be accepted
as a delimiter indicating the end of options. Any following arguments
should be treated as operands, even if they begin with the '-' character.

frontend/dockerfile: BFlags.Parse: include flag with "--" prefix in errors

Before this change, only the name of the flag would be printed as part
of errors. Change the errors to include the flag-name including the
"--" prefix to make it more clear the error is about a flag that was
provided but invalid.

Before this change:

Dockerfile:2
--------------------
1 |     FROM alpine
2 | >>> COPY --nosuchflag . .
3 |
--------------------
ERROR: failed to solve: dockerfile parse error on line 2: unknown flag: nosuchflag

Dockerfile:4
--------------------
2 |     FROM alpine
3 |     WORKDIR /test
4 | >>> COPY --exclude /.git . .
5 |
--------------------
ERROR: failed to solve: dockerfile parse error on line 4: missing a value on flag: exclude

With this change;

Dockerfile:2
--------------------
1 |     FROM alpine
2 | >>> COPY --nosuchflag . .
3 |
--------------------
ERROR: failed to solve: dockerfile parse error on line 2: unknown flag: --nosuchflag

Dockerfile:4
--------------------
2 |     FROM alpine
3 |     WORKDIR /test
4 | >>> COPY --exclude /.git . .
5 |
--------------------
ERROR: failed to solve: dockerfile parse error on line 4: missing a value on flag: --exclude

@@ -115,7 +115,7 @@ RUN --mont=target=/mytmp,type=tmpfs /bin/true
},
}, nil)
require.Error(t, err)
require.Contains(t, err.Error(), "unknown flag: mont")
require.Contains(t, err.Error(), "unknown flag: --mont")
require.Contains(t, err.Error(), "did you mean mount?")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably also look at this one to say did you mean --mount?, but I think the same error-type is used in multiple situations, so it probably needs some changes to make it preserve --, but that not to affect the "matching";

type suggestError struct {
err error
match string
}
func (e *suggestError) Error() string {
return e.err.Error() + " (did you mean " + e.match + "?)"
}
// Unwrap returns the underlying error.
func (e *suggestError) Unwrap() error {
return e.err
}

Maybe the -- could be trimmed as part of the Search (not sure), but then it would have to be added again when printing, so this needs a slightly more deeper dive into how this all works;

match, ok := Search(val, options, caseSensitive)

@thaJeztah
Copy link
Member Author

Failure is unrelated; opened a PR to fix it;

@thaJeztah
Copy link
Member Author

Hm... looks like I broke something here;

   sandbox.go:138: time="2024-09-30T12:49:03Z" level=error msg="/moby.buildkit.v1.Control/Solve returned error: rpc error: code = Unknown desc = dockerfile parse error on line 2: unknown flag: --platform" spanID=3b196da4ac85c2c4 traceID=7e2abcd2600868353a504108e50183ae
    sandbox.go:138: dockerfile parse error on line 2: unknown flag: --platform
    sandbox.go:138: 63105 8fad595 buildkitd --containerd-worker-gc=false --containerd-worker=true --containerd-worker-addr /tmp/bktest_containerd2334226698/containerd.sock --containerd-worker-labels=org.mobyproject.buildkit.worker.sandbox=true --oci-worker=false --

Minor optimization; put the check for a literal "--" first, instead of
matching for "--" as prefix. Also add some documentation to the code,
to outline why we return early.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

arg, value, hasValue := strings.Cut(arg[2:], "=")
flagName, value, hasValue := strings.Cut(a, "=")
arg := a[2:]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ this line seems to be the bug

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOH!!! I see what I did there; was originally taking a slightly different approach 🙈 . Let me fix that

…rrors

Before this change, only the name of the flag would be printed as part
of errors. Change the errors to include the flag-name _including_ the
"--" prefix to make it more clear the error is about a flag that was
provided but invalid.

Before this change:

    Dockerfile:2
    --------------------
    1 |     FROM alpine
    2 | >>> COPY --nosuchflag . .
    3 |
    --------------------
    ERROR: failed to solve: dockerfile parse error on line 2: unknown flag: nosuchflag

    Dockerfile:4
    --------------------
    2 |     FROM alpine
    3 |     WORKDIR /test
    4 | >>> COPY --exclude /.git . .
    5 |
    --------------------
    ERROR: failed to solve: dockerfile parse error on line 4: missing a value on flag: exclude

With this change;

    Dockerfile:2
    --------------------
    1 |     FROM alpine
    2 | >>> COPY --nosuchflag . .
    3 |
    --------------------
    ERROR: failed to solve: dockerfile parse error on line 2: unknown flag: --nosuchflag

    Dockerfile:4
    --------------------
    2 |     FROM alpine
    3 |     WORKDIR /test
    4 | >>> COPY --exclude /.git . .
    5 |
    --------------------
    ERROR: failed to solve: dockerfile parse error on line 4: missing a value on flag: --exclude

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

OK; mistake was fixed, and CI is happy now, so this should be ready for review 🤗

@tonistiigi tonistiigi merged commit a52c750 into moby:master Oct 1, 2024
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants