diff --git a/modules/mesh-build-go/cmd/mesh-build-go/lint.go b/modules/mesh-build-go/cmd/mesh-build-go/lint.go index 7fb41f5f..6accd280 100644 --- a/modules/mesh-build-go/cmd/mesh-build-go/lint.go +++ b/modules/mesh-build-go/cmd/mesh-build-go/lint.go @@ -60,17 +60,23 @@ func lintable(dir string, each func(path string) error) error { }) } -// unformatted are the Go files of a directory gofmt would change, relative to the tree, and the build tags they name. -func unformatted(tree, d string) (files, tags []string, err error) { +// unformatted are the Go files of a directory gofmt would change, relative to the tree, the build tags the directory's +// files name, and whether a file is left out by one of them (a negated tag, //go:build !integration). Only files whose +// directory (relative to the go.mod directory, "." for its own) is in only are checked for gofmt; nil checks every file. +func unformatted(tree, d string, only map[string]bool) (files, tags []string, negated bool, err error) { seen := map[string]bool{} - err = lintable(filepath.Join(tree, d), func(path string) error { + base := filepath.Join(tree, d) + err = lintable(base, func(path string) error { src, err := os.ReadFile(path) if err != nil { return err } - if out, ferr := format.Source(src); ferr != nil || !bytes.Equal(out, src) { - rel, _ := filepath.Rel(tree, path) - files = append(files, filepath.ToSlash(rel)) + pkgDir, _ := filepath.Rel(base, filepath.Dir(path)) + if only == nil || only[filepath.ToSlash(pkgDir)] { + if out, ferr := format.Source(src); ferr != nil || !bytes.Equal(out, src) { + rel, _ := filepath.Rel(tree, path) + files = append(files, filepath.ToSlash(rel)) + } } scan := bufio.NewScanner(bytes.NewReader(src)) for scan.Scan() { @@ -79,8 +85,15 @@ func unformatted(tree, d string) (files, tags []string, err error) { break } if expr, ok := strings.CutPrefix(line, "//go:build "); ok { - for _, tag := range buildTag.FindAllString(expr, -1) { - if !notATag[tag] && !strings.HasPrefix(tag, "go1") && !seen[tag] { + for _, at := range buildTag.FindAllStringIndex(expr, -1) { + tag := expr[at[0]:at[1]] + if notATag[tag] || strings.HasPrefix(tag, "go1") { + continue + } + if before := strings.TrimRight(expr[:at[0]], " ("); strings.HasSuffix(before, "!") { + negated = true + } + if !seen[tag] { seen[tag] = true tags = append(tags, tag) } @@ -91,13 +104,43 @@ func unformatted(tree, d string) (files, tags []string, err error) { }) sort.Strings(files) sort.Strings(tags) - return files, tags, err + return files, tags, negated, err } -// lint is gofmt and go vet of one go.mod directory d of the tree, run with run in the directory's go environment; a -// failure of either is a Failure of the level naming what failed. -func (m *Module) lint(ctx context.Context, run forgekey.Runner, tree, d string, env []string) ([]Failure, error) { - files, tags, err := unformatted(tree, d) +// lintScope is what of a go.mod directory d the lint covers, from the patterns the level runs there: every package +// (./..., and the directories nil) when a pattern is relative or none is given, else the import paths given and the +// directories of their files, relative to d. +func lintScope(tree, d string, patterns []string) (targets []string, dirs map[string]bool) { + if len(patterns) == 0 { + return []string{"./..."}, nil + } + raw, _ := os.ReadFile(filepath.Join(tree, d, "go.mod")) + mp := modulePath(string(raw)) + dirs = map[string]bool{} + for _, p := range patterns { + if strings.HasPrefix(p, "./") || p == "." { + return []string{"./..."}, nil + } + rel, ok := strings.CutPrefix(p, mp) + if mp == "" || !ok || (rel != "" && !strings.HasPrefix(rel, "/")) { + return []string{"./..."}, nil // a package of no module here: the whole directory, as before + } + if rel = strings.TrimPrefix(rel, "/"); rel == "" { + rel = "." + } + dirs[rel] = true + targets = append(targets, p) + } + return targets, dirs +} + +// lint is gofmt and go vet of one go.mod directory d of the tree, run with run in the directory's go environment, over +// the packages the level runs there (patterns, as packagesByDir gives them; issue 1921, try speed S3); a failure of either +// is a Failure of the level naming what failed. go vet runs once with every tag the directory names, which vets the +// untagged files too; only a directory where a tag leaves a file out vets also without tags, both passes at once. +func (m *Module) lint(ctx context.Context, run forgekey.Runner, tree, d string, env, patterns []string) ([]Failure, error) { + targets, only := lintScope(tree, d, patterns) + files, tags, negated, err := unformatted(tree, d, only) if err != nil { return nil, fmt.Errorf("reading the Go files of %s: %w", d, err) } @@ -106,32 +149,47 @@ func (m *Module) lint(ctx context.Context, run forgekey.Runner, tree, d string, failed = append(failed, Failure{Package: d, Test: "gofmt", Output: "not as gofmt writes them (run gofmt -w): " + strings.Join(files, ", ")}) } - sets := [][]string{nil} - if len(tags) > 0 { - sets = append(sets, tags) + sets := [][]string{tags} + if len(tags) > 0 && negated { + sets = [][]string{nil, tags} } - for _, set := range sets { + type vetRun struct { + name string + r forgekey.Ran + } + runs := make([]vetRun, len(sets)) + var wg sync.WaitGroup + for i, set := range sets { args := []string{"-C", filepath.Join(tree, d), "vet"} name := "go vet" if len(set) > 0 { args = append(args, "-tags", strings.Join(set, ",")) name += " -tags " + strings.Join(set, ",") } - args = append(args, "./...") - r := run(ctx, env, "go", args...) - if r.Err != "" { - return nil, fmt.Errorf("%s in %s could not be run: %s", name, d, firstLine(r.Err+"\n"+r.Stderr)) + args = append(args, targets...) + runs[i].name = name + wg.Add(1) + go func() { + defer wg.Done() + runs[i].r = run(ctx, env, "go", args...) + }() + } + wg.Wait() + for _, v := range runs { + if v.r.Err != "" { + return nil, fmt.Errorf("%s in %s could not be run: %s", v.name, d, firstLine(v.r.Err+"\n"+v.r.Stderr)) } - if r.Status != 0 { - failed = append(failed, Failure{Package: d, Test: name, Output: strings.TrimSpace(r.Stderr + "\n" + r.Stdout)}) + if v.r.Status != 0 { + failed = append(failed, Failure{Package: d, Test: v.name, Output: strings.TrimSpace(v.r.Stderr + "\n" + v.r.Stdout)}) } } return failed, nil } // lintAll lints each directory, up to m.parallel() at once like the level's tests; the failures join in directory order -// and the error is the first directory's, so the answer does not depend on which ended first (issue 1930). -func (m *Module) lintAll(ctx context.Context, run forgekey.Runner, tree string, dirs []string, env func(d string) []string) ([]Failure, error) { +// and the error is the first directory's, so the answer does not depend on which ended first (issue 1930). byDir names +// the packages the level runs in each directory (packagesByDir); nil lints each directory whole. +func (m *Module) lintAll(ctx context.Context, run forgekey.Runner, tree string, dirs []string, env func(d string) []string, byDir map[string][]string) ([]Failure, error) { type answer struct { failed []Failure err error @@ -154,7 +212,7 @@ func (m *Module) lintAll(ctx context.Context, run forgekey.Runner, tree string, crashMu.Unlock() } }() - answers[i].failed, answers[i].err = m.lint(ctx, run, tree, d, env(d)) + answers[i].failed, answers[i].err = m.lint(ctx, run, tree, d, env(d), byDir[d]) }() } wg.Wait() diff --git a/modules/mesh-build-go/cmd/mesh-build-go/lint_test.go b/modules/mesh-build-go/cmd/mesh-build-go/lint_test.go index fef621b0..ec4ac01a 100644 --- a/modules/mesh-build-go/cmd/mesh-build-go/lint_test.go +++ b/modules/mesh-build-go/cmd/mesh-build-go/lint_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "slices" "strings" "sync" "testing" @@ -28,8 +29,9 @@ func writeTree(t *testing.T, files map[string]string) string { } // 1887/D7-go, ADR 0529 ยง1: an unformatted file fails the unit level naming the file (testdata and vendor are left -// alone), and go vet runs with no tag and with every tag the files name together, integration and isolation included, so -// a vet error behind isolation fails the level naming the run. +// alone), and go vet runs with every tag the files name together, integration and isolation included, so a vet error +// behind isolation fails the level naming the run. One pass with every tag vets the untagged files too (issue 1921, try +// speed S3), as no file here is left out by a tag. func TestTheUnitLevelRunsGofmtAndVetWithEveryTag(t *testing.T) { tree := writeTree(t, map[string]string{ "go.mod": "module x\n\ngo 1.26\n", @@ -51,11 +53,11 @@ func TestTheUnitLevelRunsGofmtAndVetWithEveryTag(t *testing.T) { } return forgekey.Ran{} } - failed, err := (&Module{}).lint(context.Background(), run, tree, ".", nil) + failed, err := (&Module{}).lint(context.Background(), run, tree, ".", nil, nil) if err != nil { t.Fatal(err) } - if strings.Join(vetted, " | ") != "vet ./... | vet -tags integration,isolation ./..." { + if strings.Join(vetted, " | ") != "vet -tags integration,isolation ./..." { t.Errorf("vet ran %v", vetted) } if len(failed) != 2 || failed[0].Test != "gofmt" || !strings.Contains(failed[0].Output, "b.go") || @@ -67,7 +69,7 @@ func TestTheUnitLevelRunsGofmtAndVetWithEveryTag(t *testing.T) { } // A formatted directory that vets passes. clean := writeTree(t, map[string]string{"go.mod": "module y\n", "a.go": "package y\n"}) - if failed, err := (&Module{}).lint(context.Background(), func(context.Context, []string, string, ...string) forgekey.Ran { return forgekey.Ran{} }, clean, ".", nil); err != nil || len(failed) != 0 { + if failed, err := (&Module{}).lint(context.Background(), func(context.Context, []string, string, ...string) forgekey.Ran { return forgekey.Ran{} }, clean, ".", nil, nil); err != nil || len(failed) != 0 { t.Errorf("a clean directory: %+v, %v", failed, err) } } @@ -121,7 +123,7 @@ func TestTheLintOfManyDirectoriesRunsAtOnceUpToTheBound(t *testing.T) { return forgekey.Ran{} } m := &Module{Parallel: 3} - failed, err := m.lintAll(context.Background(), run, tree, dirs, func(string) []string { return nil }) + failed, err := m.lintAll(context.Background(), run, tree, dirs, func(string) []string { return nil }, nil) if err != nil { t.Fatal(err) } @@ -132,3 +134,68 @@ func TestTheLintOfManyDirectoriesRunsAtOnceUpToTheBound(t *testing.T) { t.Errorf("failed = %+v, want b's then e's", failed) } } + +// 1921/S3: the lint of a level that runs some packages vets those packages alone, in one pass with every tag their +// directory names, and reports a gofmt fault only in their files: a fault in a package the level keeps was reported +// when that package changed. +func TestTheLintOfSomePackagesVetsThemInOnePassAndFormatsOnlyTheirFiles(t *testing.T) { + tree := writeTree(t, map[string]string{ + "go.mod": "module x\n\ngo 1.26\n", + "a/a.go": "package a\n\nfunc A() {}\n", + "a/a_integration_test.go": "//go:build integration\n\npackage a\n", + "b/b.go": "package b\nfunc B() {}\n", + "a/deeper/d.go": "package deeper\nfunc D() {}\n", + }) + var vetted []string + run := func(ctx context.Context, env []string, name string, args ...string) forgekey.Ran { + vetted = append(vetted, strings.Join(args[2:], " ")) + return forgekey.Ran{} + } + failed, err := (&Module{}).lint(context.Background(), run, tree, ".", nil, []string{"x/a"}) + if err != nil { + t.Fatal(err) + } + if strings.Join(vetted, " | ") != "vet -tags integration x/a" { + t.Errorf("vet ran %v, want one pass over x/a with its tag", vetted) + } + if len(failed) != 0 { + t.Errorf("the faults of packages the level keeps were reported: %+v", failed) + } + vetted = nil + failed, err = (&Module{}).lint(context.Background(), run, tree, ".", nil, []string{"x/b"}) + if err != nil { + t.Fatal(err) + } + if len(failed) != 1 || failed[0].Test != "gofmt" || !strings.Contains(failed[0].Output, "b/b.go") || strings.Contains(failed[0].Output, "d.go") { + t.Errorf("failed = %+v, want gofmt naming b/b.go alone", failed) + } + if strings.Join(vetted, " | ") != "vet -tags integration x/b" { + t.Errorf("vet ran %v", vetted) + } +} + +// 1921/S3: a file a tag leaves out (//go:build !integration) is vetted by no pass with the tag, so a directory holding +// one keeps the pass without tags beside the pass with them. +func TestADirectoryWithAFileATagLeavesOutKeepsThePassWithoutTags(t *testing.T) { + tree := writeTree(t, map[string]string{ + "go.mod": "module x\n", + "a.go": "package x\n", + "fake.go": "//go:build !integration\n\npackage x\n", + "real_test.go": "//go:build integration\n\npackage x\n", + }) + var mu sync.Mutex + var vetted []string + run := func(ctx context.Context, env []string, name string, args ...string) forgekey.Ran { + mu.Lock() + vetted = append(vetted, strings.Join(args[2:], " ")) + mu.Unlock() + return forgekey.Ran{} + } + if _, err := (&Module{}).lint(context.Background(), run, tree, ".", nil, nil); err != nil { + t.Fatal(err) + } + slices.Sort(vetted) + if strings.Join(vetted, " | ") != "vet -tags integration ./... | vet ./..." { + t.Errorf("vet ran %v, want the pass without tags and the pass with them", vetted) + } +} diff --git a/modules/mesh-build-go/cmd/mesh-build-go/test.go b/modules/mesh-build-go/cmd/mesh-build-go/test.go index e7d9624f..97a86bbc 100644 --- a/modules/mesh-build-go/cmd/mesh-build-go/test.go +++ b/modules/mesh-build-go/cmd/mesh-build-go/test.go @@ -211,7 +211,7 @@ func (m *Module) runLevel(ctx context.Context, tree string, o levelOpts) (*TestR linted := m.clock() failed, err := m.lintAll(ctx, run, tree, todo, func(d string) []string { return append(m.goEnvFor(filepath.Join(tree, d)), o.env...) - }) + }, byDir) if err != nil { return nil, err }