diff --git a/frontend/dockerfile/builder/build.go b/frontend/dockerfile/builder/build.go index 6e32c65feb09..a49b97ae4ca6 100644 --- a/frontend/dockerfile/builder/build.go +++ b/frontend/dockerfile/builder/build.go @@ -87,8 +87,7 @@ func Build(ctx context.Context, c client.Client) (_ *client.Result, err error) { return dockerfile2llb.ListTargets(ctx, src.Data) }, Lint: func(ctx context.Context) (*lint.LintResults, error) { - warnings, err := dockerfile2llb.DockerfileLint(ctx, src.Data, convertOpt) - return &lint.LintResults{Warnings: warnings}, err + return dockerfile2llb.DockerfileLint(ctx, src.Data, convertOpt) }, }); err != nil { return nil, err diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 08e6b2009d35..63c4158931a8 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -111,32 +111,39 @@ func Dockefile2Outline(ctx context.Context, dt []byte, opt ConvertOpt) (*outline return &o, nil } -func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) ([]lint.Warning, error) { - warnings := []lint.Warning{} - sourceData := strings.Split(string(opt.SourceMap.Data), "\n") +func DockerfileLint(ctx context.Context, dt []byte, opt ConvertOpt) (*lint.LintResults, error) { + results := &lint.LintResults{} opt.Warn = func(short, url string, detail [][]byte, location []parser.Range) { - var rulename, ruledetail string + var ruleName, ruleDetail string if strings.HasPrefix(short, "Lint Rule ") { - rulename = strings.TrimPrefix(short, "Lint Rule ") - ruleParts := strings.Split(rulename, ":") - rulename = strings.Trim(ruleParts[0], "'") - ruledetail = strings.TrimSpace(ruleParts[1]) + ruleName = strings.TrimPrefix(short, "Lint Rule ") + ruleParts := strings.Split(ruleName, ":") + ruleName = strings.Trim(ruleParts[0], "'") + ruleDetail = strings.TrimSpace(ruleParts[1]) } else { return } - warnings = append(warnings, lint.Warning{ - RuleName: rulename, - Detail: ruledetail, - Filename: opt.SourceMap.Filename, - Source: sourceData[location[0].Start.Line-1 : location[0].End.Line], - StartLine: location[0].Start.Line, - }) + sourceIndex := results.AddSource(opt.SourceMap.Filename, "Dockerfile", opt.SourceMap.Data) + sourceLocation := []lint.Range{} + for _, loc := range location { + sourceLocation = append(sourceLocation, lint.Range{ + Start: lint.Position{ + Line: loc.Start.Line, + Column: loc.Start.Character, + }, + End: lint.Position{ + Line: loc.End.Line, + Column: loc.End.Character, + }, + }) + } + results.AddWarning(ruleName, ruleDetail, sourceIndex, sourceLocation) } _, err := toDispatchState(ctx, dt, opt) if err != nil { return nil, err } - return warnings, nil + return results, nil } func ListTargets(ctx context.Context, dt []byte) (*targets.List, error) { diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 6d8ac6ee8a77..6c89b7785561 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -3,6 +3,7 @@ package dockerfile import ( "context" "encoding/json" + "fmt" "os" "sort" "testing" @@ -15,109 +16,16 @@ import ( "github.com/moby/buildkit/frontend/subrequests/lint" "github.com/moby/buildkit/util/testutil/integration" - "github.com/moby/buildkit/util/testutil/workers" "github.com/pkg/errors" "github.com/stretchr/testify/require" "github.com/tonistiigi/fsutil" ) var lintTests = integration.TestFuncs( - testLintRequest, testLintStageName, testLintNoEmptyContinuations, ) -func testLintRequest(t *testing.T, sb integration.Sandbox) { - integration.SkipOnPlatform(t, "windows") - workers.CheckFeatureCompat(t, sb, workers.FeatureFrontendOutline) - f := getFrontend(t, sb) - if _, ok := f.(*clientFrontend); !ok { - t.Skip("only test with client frontend") - } - - dockerfile := []byte(` - FROM scratch as target - COPY Dockerfile \ - - . - ARG inherited=box - copy Dockerfile /foo - - FROM scratch AS target2 - COPY Dockerfile /Dockerfile - `) - - dir := integration.Tmpdir( - t, - fstest.CreateFile("Dockerfile", []byte(dockerfile), 0600), - ) - - c, err := client.New(sb.Context(), sb.Address()) - require.NoError(t, err) - defer c.Close() - - destDir, err := os.MkdirTemp("", "buildkit") - require.NoError(t, err) - defer os.RemoveAll(destDir) - - called := false - frontend := func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { - res, err := c.Solve(ctx, gateway.SolveRequest{ - FrontendOpt: map[string]string{ - "frontend.caps": "moby.buildkit.frontend.subrequests", - "requestid": "frontend.lint", - "build-arg:BAR": "678", - "target": "target", - }, - Frontend: "dockerfile.v0", - }) - require.NoError(t, err) - - lintResults, err := unmarshalLintResults(res) - require.NoError(t, err) - - require.Equal(t, 3, len(lintResults.Warnings)) - sort.Slice(lintResults.Warnings, func(i, j int) bool { - // sort by line number in ascending order - return lintResults.Warnings[i].StartLine < lintResults.Warnings[j].StartLine - }) - checkLintRequestWarnings(t, lintResults.Warnings, []lint.Warning{ - { - RuleName: "FromAsCasing", - Detail: "'as' and 'FROM' keywords' casing do not match (line 2)", - Filename: "Dockerfile", - StartLine: 2, - Source: []string{"\tFROM scratch as target"}, - }, - { - RuleName: "NoEmptyContinuations", - Detail: "Empty continuation line (line 5)", - Filename: "Dockerfile", - StartLine: 5, - Source: []string{"\t."}, - }, - { - RuleName: "FileConsistentCommandCasing", - Detail: "Command 'copy' should match the case of the command majority (uppercase) (line 7)", - Filename: "Dockerfile", - StartLine: 7, - Source: []string{"\tcopy Dockerfile /foo"}, - }, - }) - called = true - return nil, nil - } - - _, err = c.Build(sb.Context(), client.SolveOpt{ - LocalMounts: map[string]fsutil.FS{ - dockerui.DefaultLocalNameDockerfile: dir, - }, - }, "", frontend, nil) - require.NoError(t, err) - - require.True(t, called) -} - func testLintStageName(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(` # warning: stage name should be lowercase @@ -130,14 +38,18 @@ FROM scratch AS base3 `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'StageNameCasing': Stage name 'BadStageName' should be lowercase (line 3)", - Detail: "Stage names should be lowercase", - Level: 1, + RuleName: "StageNameCasing", + Description: "Stage name 'BadStageName' should be lowercase", + Line: 3, + Detail: "Stage names should be lowercase", + Level: 1, }, { - Short: "Lint Rule 'FromAsCasing': 'as' and 'FROM' keywords' casing do not match (line 6)", - Detail: "The 'as' keyword should match the case of the 'from' keyword", - Level: 1, + RuleName: "FromAsCasing", + Description: "'as' and 'FROM' keywords' casing do not match", + Line: 6, + Detail: "The 'as' keyword should match the case of the 'from' keyword", + Level: 1, }, }) @@ -149,9 +61,11 @@ from scratch as base2 `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FromAsCasing': 'AS' and 'from' keywords' casing do not match (line 3)", - Detail: "The 'as' keyword should match the case of the 'from' keyword", - Level: 1, + RuleName: "FromAsCasing", + Description: "'AS' and 'from' keywords' casing do not match", + Line: 3, + Detail: "The 'as' keyword should match the case of the 'from' keyword", + Level: 1, }, }) } @@ -169,10 +83,12 @@ COPY Dockerfile \ checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'NoEmptyContinuations': Empty continuation line (line 6)", - Detail: "Empty continuation lines will become errors in a future release", - URL: "https://github.com/moby/moby/pull/33719", - Level: 1, + RuleName: "NoEmptyContinuations", + Description: "Empty continuation line", + Line: 6, + Detail: "Empty continuation lines will become errors in a future release", + URL: "https://github.com/moby/moby/pull/33719", + Level: 1, }, }) } @@ -185,9 +101,11 @@ FROM scratch AS base2 `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'SelfConsistentCommandCasing': Command 'From' should be consistently cased (line 3)", - Detail: "Commands should be in consistent casing (all lower or all upper)", - Level: 1, + RuleName: "SelfConsistentCommandCasing", + Description: "Command 'From' should be consistently cased", + Line: 3, + Detail: "Commands should be in consistent casing (all lower or all upper)", + Level: 1, }, }) dockerfile = []byte(` @@ -197,9 +115,11 @@ from scratch as base2 `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'SelfConsistentCommandCasing': Command 'frOM' should be consistently cased (line 3)", - Detail: "Commands should be in consistent casing (all lower or all upper)", - Level: 1, + RuleName: "SelfConsistentCommandCasing", + Description: "Command 'frOM' should be consistently cased", + Line: 3, + Detail: "Commands should be in consistent casing (all lower or all upper)", + Level: 1, }, }) } @@ -213,9 +133,11 @@ COPY Dockerfile /bar `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'copy' should match the case of the command majority (uppercase) (line 4)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, + RuleName: "FileConsistentCommandCasing", + Description: "Command 'copy' should match the case of the command majority (uppercase)", + Line: 4, + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, }, }) @@ -227,9 +149,11 @@ copy Dockerfile /bar `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'COPY' should match the case of the command majority (lowercase) (line 4)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, + RuleName: "FileConsistentCommandCasing", + Description: "Command 'COPY' should match the case of the command majority (lowercase)", + Line: 4, + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, }, }) @@ -242,9 +166,11 @@ COPY Dockerfile /baz `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'from' should match the case of the command majority (uppercase) (line 3)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, + RuleName: "FileConsistentCommandCasing", + Description: "Command 'from' should match the case of the command majority (uppercase)", + Line: 3, + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, }, }) @@ -257,9 +183,11 @@ copy Dockerfile /baz `) checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{ { - Short: "Lint Rule 'FileConsistentCommandCasing': Command 'FROM' should match the case of the command majority (lowercase) (line 3)", - Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", - Level: 1, + RuleName: "FileConsistentCommandCasing", + Description: "Command 'FROM' should match the case of the command majority (lowercase)", + Line: 3, + Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)", + Level: 1, }, }) @@ -278,22 +206,52 @@ COPY Dockerfile /bar checkLinterWarnings(t, sb, dockerfile, []expectedLintWarning{}) } -func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) { - // As a note, this test depends on the `expected` lint - // warnings to be in order of appearance in the Dockerfile. +func checkUnmarshal(t *testing.T, sb integration.Sandbox, c *client.Client, f frontend, dir *integration.TmpDirWithName, dockerfile []byte, expected []expectedLintWarning) { + destDir, err := os.MkdirTemp("", "buildkit") + require.NoError(t, err) + defer os.RemoveAll(destDir) - integration.SkipOnPlatform(t, "windows") - f := getFrontend(t, sb) + called := false + frontend := func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { + res, err := c.Solve(ctx, gateway.SolveRequest{ + FrontendOpt: map[string]string{ + "frontend.caps": "moby.buildkit.frontend.subrequests", + "requestid": "frontend.lint", + "build-arg:BAR": "678", + }, + Frontend: "dockerfile.v0", + }) + require.NoError(t, err) - dir := integration.Tmpdir( - t, - fstest.CreateFile("Dockerfile", dockerfile, 0600), - ) + lintResults, err := unmarshalLintResults(res) + require.NoError(t, err) - c, err := client.New(sb.Context(), sb.Address()) + require.Equal(t, len(expected), len(lintResults.Warnings)) + sort.Slice(lintResults.Warnings, func(i, j int) bool { + // sort by line number in ascending order + firstRange := lintResults.Warnings[i].Location[0] + secondRange := lintResults.Warnings[j].Location[0] + return firstRange.Start.Line < secondRange.Start.Line + }) + // Compare expectedLintWarning with actual lint results + for i, w := range lintResults.Warnings { + checkLintWarning(t, w, expected[i]) + } + called = true + return nil, nil + } + + _, err = c.Build(sb.Context(), client.SolveOpt{ + LocalMounts: map[string]fsutil.FS{ + dockerui.DefaultLocalNameDockerfile: dir, + }, + }, "", frontend, nil) require.NoError(t, err) - defer c.Close() + require.True(t, called) +} + +func checkProgressStream(t *testing.T, sb integration.Sandbox, c *client.Client, f frontend, dir *integration.TmpDirWithName, dockerfile []byte, expected []expectedLintWarning) { status := make(chan *client.SolveStatus) statusDone := make(chan struct{}) done := make(chan struct{}) @@ -315,7 +273,7 @@ func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte } }() - _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + _, err := f.Solve(sb.Context(), c, client.SolveOpt{ FrontendAttrs: map[string]string{ "platform": "linux/amd64,linux/arm64", }, @@ -345,27 +303,42 @@ func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte return w1.Range[0].Start.Line < w2.Range[0].Start.Line }) for i, w := range warnings { - require.Equal(t, expected[i].Short, string(w.Short)) - require.Equal(t, expected[i].Detail, string(w.Detail[0])) - require.Equal(t, expected[i].URL, w.URL) - require.Equal(t, expected[i].Level, w.Level) + checkVertexWarning(t, w, expected[i]) } } -func checkLintRequestWarnings(t *testing.T, actual, expected []lint.Warning) { - require.Equal(t, len(expected), len(actual)) - - for i, expected := range expected { - actual := actual[i] - require.Equal(t, expected.RuleName, actual.RuleName) - require.Equal(t, expected.Detail, actual.Detail) - require.Equal(t, expected.Filename, actual.Filename) - require.Equal(t, expected.StartLine, actual.StartLine) - require.Equal(t, len(expected.Source), len(actual.Source)) - for j, expectedSource := range expected.Source { - require.Equal(t, expectedSource, actual.Source[j]) - } - } +func checkLinterWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) { + // As a note, this test depends on the `expected` lint + // warnings to be in order of appearance in the Dockerfile. + + integration.SkipOnPlatform(t, "windows") + f := getFrontend(t, sb) + + dir := integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + checkProgressStream(t, sb, c, f, dir, dockerfile, expected) + checkUnmarshal(t, sb, c, f, dir, dockerfile, expected) +} + +func checkVertexWarning(t *testing.T, warning *client.VertexWarning, expected expectedLintWarning) { + short := fmt.Sprintf("Lint Rule '%s': %s (line %d)", expected.RuleName, expected.Description, expected.Line) + require.Equal(t, short, string(warning.Short)) + require.Equal(t, expected.Detail, string(warning.Detail[0])) + require.Equal(t, expected.URL, warning.URL) + require.Equal(t, expected.Level, warning.Level) +} + +func checkLintWarning(t *testing.T, warning lint.Warning, expected expectedLintWarning) { + require.Equal(t, expected.RuleName, warning.RuleName) + detail := fmt.Sprintf("%s (line %d)", expected.Description, expected.Line) + require.Equal(t, detail, warning.Detail) } func unmarshalLintResults(res *gateway.Result) (*lint.LintResults, error) { @@ -379,3 +352,12 @@ func unmarshalLintResults(res *gateway.Result) (*lint.LintResults, error) { } return &l, nil } + +type expectedLintWarning struct { + RuleName string + Description string + Line int + Detail string + URL string + Level int +} diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 53d3eae46cd0..98485715f95d 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -7300,13 +7300,6 @@ type nopWriteCloser struct { func (nopWriteCloser) Close() error { return nil } -type expectedLintWarning struct { - Short string - Detail string - URL string - Level int -} - type secModeSandbox struct{} func (*secModeSandbox) UpdateConfigFile(in string) string { diff --git a/frontend/subrequests/lint/lint.go b/frontend/subrequests/lint/lint.go index 8ef6892740b6..d03e40656c1f 100644 --- a/frontend/subrequests/lint/lint.go +++ b/frontend/subrequests/lint/lint.go @@ -26,16 +26,67 @@ var SubrequestLintDefinition = subrequests.Request{ }, } +type Source struct { + Filename string `json:"filename"` + Language string `json:"language"` + Data []byte `json:"data"` +} + +type Range struct { + Start Position `json:"start"` + End Position `json:"end"` +} + +type Position struct { + Line int `json:"line"` + Column int `json:"column"` +} + type Warning struct { - RuleName string `json:"rule_name"` - Detail string `json:"detail"` - Filename string `json:"filename"` - Source []string `json:"source"` - StartLine int `json:"start_line"` + RuleName string `json:"rule_name"` + Detail string `json:"detail"` + SourceIndex int `json:"source_index"` + Location []Range `json:"location"` } type LintResults struct { Warnings []Warning `json:"warnings"` + Sources []Source `json:"sources"` +} + +func (results *LintResults) AddSource(filename string, language string, data []byte) int { + sourceE := Source{ + Filename: filename, + Language: language, + Data: data, + } + for i, source := range results.Sources { + if sourceEqual(source, sourceE) { + return i + } + } + results.Sources = append(results.Sources, Source{ + Filename: filename, + Language: language, + Data: data, + }) + return len(results.Sources) - 1 +} + +func (results *LintResults) AddWarning(ruleName, detail string, sourceIndex int, location []Range) { + results.Warnings = append(results.Warnings, Warning{ + RuleName: ruleName, + Detail: detail, + SourceIndex: sourceIndex, + Location: location, + }) +} + +func sourceEqual(a, b Source) bool { + if a.Filename != b.Filename || a.Language != b.Language { + return false + } + return bytes.Equal(a.Data, b.Data) } func (warns LintResults) ToResult() (*client.Result, error) { @@ -79,10 +130,19 @@ func PrintLintViolations(dt []byte, w io.Writer) error { for _, rule := range lintWarningRules { fmt.Fprintf(tw, "Lint Rule %s\n", rule) for _, warning := range lintWarnings[rule] { - fmt.Fprintf(tw, "\t%s:%d\n", warning.Filename, warning.StartLine) + source := warnings.Sources[warning.SourceIndex] + sourceData := bytes.Split(source.Data, []byte("\n")) + firstRange := warning.Location[0] + if firstRange.Start.Line != firstRange.End.Line { + fmt.Fprintf(tw, "\t%s:%d-%d\n", source.Filename, firstRange.Start.Line, firstRange.End.Line) + } else { + fmt.Fprintf(tw, "\t%s:%d\n", source.Filename, firstRange.Start.Line) + } fmt.Fprintf(tw, "\t%s\n", warning.Detail) - for offset, source := range warning.Source { - fmt.Fprintf(tw, "\t\t%d\t|\t%s\n", warning.StartLine+offset, source) + for _, r := range warning.Location { + for i := r.Start.Line; i <= r.End.Line; i++ { + fmt.Fprintf(tw, "\t%d\t|\t%s\n", i, sourceData[i-1]) + } } fmt.Fprintln(tw) }