From d37e8d0bfaaf3a40ef2432f6bed7bab2004e36eb Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 16 Jun 2025 21:10:59 -0700 Subject: [PATCH] .github/workflows: remove redundant work between staticcheck jobs Make the OS-specific staticcheck jobs only test stuff that's specialized for that OS. Do that using a new ./tool/listpkgs program that's a fancy 'go list' with more filtering flags. Updates tailscale/corp#28679 Change-Id: I790be2e3a0b42b105bd39f68c4b20e217a26de60 Signed-off-by: Brad Fitzpatrick --- .github/workflows/test.yml | 87 ++++++++++++++-- Makefile | 2 +- tool/listpkgs/listpkgs.go | 206 +++++++++++++++++++++++++++++++++++++ 3 files changed, 283 insertions(+), 12 deletions(-) create mode 100644 tool/listpkgs/listpkgs.go diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 313ce609f..6d8ab863c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -232,10 +232,6 @@ jobs: - name: Restore Cache uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 with: - # Note: unlike the other setups, this is only grabbing the mod download - # cache, rather than the whole mod directory, as the download cache - # contains zips that can be unpacked in parallel faster than they can be - # fetched and extracted by tar path: | ~/.cache/go-build ~\AppData\Local\go-build @@ -722,14 +718,40 @@ jobs: staticcheck: runs-on: ubuntu-24.04 needs: gomod-cache + name: staticcheck (${{ matrix.name }}) strategy: fail-fast: false # don't abort the entire matrix if one element fails matrix: - goos: ["linux", "windows", "darwin"] - goarch: ["amd64"] include: - - goos: "windows" - goarch: "386" + - name: "macOS" + goos: "darwin" + goarch: "arm64" + flags: "--with-tags-all=darwin" + - name: "Windows" + goos: "windows" + goarch: "amd64" + flags: "--with-tags-all=windows" + - name: "Linux" + goos: "linux" + goarch: "amd64" + flags: "--with-tags-all=linux" + - name: "Portable (1/4)" + goos: "linux" + goarch: "amd64" + flags: "--without-tags-any=windows,darwin,linux --shard=1/4" + - name: "Portable (2/4)" + goos: "linux" + goarch: "amd64" + flags: "--without-tags-any=windows,darwin,linux --shard=2/4" + - name: "Portable (3/4)" + goos: "linux" + goarch: "amd64" + flags: "--without-tags-any=windows,darwin,linux --shard=3/4" + - name: "Portable (4/4)" + goos: "linux" + goarch: "amd64" + flags: "--without-tags-any=windows,darwin,linux --shard=4/4" + steps: - name: checkout uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 @@ -741,14 +763,14 @@ jobs: path: gomodcache key: ${{ needs.gomod-cache.outputs.cache-key }} enableCrossOsArchive: true - - name: run staticcheck + - name: run staticcheck (${{ matrix.name }}) working-directory: src run: | export GOROOT=$(./tool/go env GOROOT) ./tool/go run -exec \ "env GOOS=${{ matrix.goos }} GOARCH=${{ matrix.goarch }}" \ honnef.co/go/tools/cmd/staticcheck -- \ - $(env GOOS=${{ matrix.goos }} GOARCH=${{ matrix.goarch }} ./tool/go list ./... | grep -v tempfork) + $(./tool/go run ./tool/listpkgs --ignore-3p --goos=${{ matrix.goos }} --goarch=${{ matrix.goarch }} ${{ matrix.flags }} ./...) notify_slack: if: always() @@ -795,7 +817,7 @@ jobs: }] } - check_mergeability: + merge_blocker: if: always() runs-on: ubuntu-24.04 needs: @@ -819,3 +841,46 @@ jobs: uses: re-actors/alls-green@05ac9388f0aebcb5727afa17fcccfecd6f8ec5fe # v1.2.2 with: jobs: ${{ toJSON(needs) }} + + # This waits on all the jobs which must never fail. Branch protection rules + # enforce these. No flaky tests are allowed in these jobs. (We don't want flaky + # tests anywhere, really, but a flaky test here prevents merging.) + check_mergeability_strict: + if: always() + runs-on: ubuntu-24.04 + needs: + - android + - cross + - crossmin + - ios + - tailscale_go + - depaware + - go_generate + - go_mod_tidy + - licenses + - staticcheck + steps: + - name: Decide if change is okay to merge + if: github.event_name != 'push' + uses: re-actors/alls-green@05ac9388f0aebcb5727afa17fcccfecd6f8ec5fe # v1.2.2 + with: + jobs: ${{ toJSON(needs) }} + + check_mergeability: + if: always() + runs-on: ubuntu-24.04 + needs: + - check_mergeability_strict + - test + - windows + - vm + - wasm + - fuzz + - race-root-integration + - privileged + steps: + - name: Decide if change is okay to merge + if: github.event_name != 'push' + uses: re-actors/alls-green@05ac9388f0aebcb5727afa17fcccfecd6f8ec5fe # v1.2.2 + with: + jobs: ${{ toJSON(needs) }} diff --git a/Makefile b/Makefile index 1978af90d..41c67c711 100644 --- a/Makefile +++ b/Makefile @@ -64,7 +64,7 @@ buildmultiarchimage: ## Build (and optionally push) multiarch docker image check: staticcheck vet depaware buildwindows build386 buildlinuxarm buildwasm ## Perform basic checks and compilation tests staticcheck: ## Run staticcheck.io checks - ./tool/go run honnef.co/go/tools/cmd/staticcheck -- $$(./tool/go list ./... | grep -v tempfork) + ./tool/go run honnef.co/go/tools/cmd/staticcheck -- $$(./tool/go run ./tool/listpkgs --ignore-3p ./...) kube-generate-all: kube-generate-deepcopy ## Refresh generated files for Tailscale Kubernetes Operator ./tool/go generate ./cmd/k8s-operator diff --git a/tool/listpkgs/listpkgs.go b/tool/listpkgs/listpkgs.go new file mode 100644 index 000000000..400bf90c1 --- /dev/null +++ b/tool/listpkgs/listpkgs.go @@ -0,0 +1,206 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +// listpkgs prints the import paths that match the Go package patterns +// given on the command line and conditionally filters them in various ways. +package main + +import ( + "bufio" + "flag" + "fmt" + "go/build/constraint" + "log" + "os" + "slices" + "strings" + "sync" + + "golang.org/x/tools/go/packages" +) + +var ( + ignore3p = flag.Bool("ignore-3p", false, "ignore third-party packages forked/vendored into Tailscale") + goos = flag.String("goos", "", "GOOS to use for loading packages (default: current OS)") + goarch = flag.String("goarch", "", "GOARCH to use for loading packages (default: current architecture)") + withTagsAllStr = flag.String("with-tags-all", "", "if non-empty, a comma-separated list of builds tags to require (a package will only be listed if it contains all of these build tags)") + withoutTagsAnyStr = flag.String("without-tags-any", "", "if non-empty, a comma-separated list of build constraints to exclude (a package will be omitted if it contains any of these build tags)") + shard = flag.String("shard", "", "if non-empty, a string of the form 'N/M' to only print packages in shard N of M (e.g. '1/3', '2/3', '3/3/' for different thirds of the list)") +) + +func main() { + flag.Parse() + + patterns := flag.Args() + if len(patterns) == 0 { + flag.Usage() + os.Exit(1) + } + + cfg := &packages.Config{ + Mode: packages.LoadFiles, + Env: os.Environ(), + } + if *goos != "" { + cfg.Env = append(cfg.Env, "GOOS="+*goos) + } + if *goarch != "" { + cfg.Env = append(cfg.Env, "GOARCH="+*goarch) + } + + pkgs, err := packages.Load(cfg, patterns...) + if err != nil { + log.Fatalf("loading packages: %v", err) + } + + var withoutAny []string + if *withoutTagsAnyStr != "" { + withoutAny = strings.Split(*withoutTagsAnyStr, ",") + } + var withAll []string + if *withTagsAllStr != "" { + withAll = strings.Split(*withTagsAllStr, ",") + } + + seen := map[string]bool{} + matches := 0 +Pkg: + for _, pkg := range pkgs { + if pkg.PkgPath == "" { // malformed (shouldn’t happen) + continue + } + if seen[pkg.PkgPath] { + continue // suppress duplicates when patterns overlap + } + seen[pkg.PkgPath] = true + + pkgPath := pkg.PkgPath + + if *ignore3p && isThirdParty(pkgPath) { + continue + } + if withAll != nil { + for _, t := range withAll { + if !hasBuildTag(pkg, t) { + continue Pkg + } + } + } + for _, t := range withoutAny { + if hasBuildTag(pkg, t) { + continue Pkg + } + } + matches++ + + if *shard != "" { + var n, m int + if _, err := fmt.Sscanf(*shard, "%d/%d", &n, &m); err != nil || n < 1 || m < 1 { + log.Fatalf("invalid shard format %q; expected 'N/M'", *shard) + } + if m > 0 && (matches-1)%m != n-1 { + continue // not in this shard + } + } + fmt.Println(pkgPath) + } + + // If any package had errors (e.g. missing deps) report them via packages.PrintErrors. + // This mirrors `go list` behaviour when -e is *not* supplied. + if packages.PrintErrors(pkgs) > 0 { + os.Exit(1) + } +} + +func isThirdParty(pkg string) bool { + return strings.HasPrefix(pkg, "tailscale.com/tempfork/") +} + +// hasBuildTag reports whether any source file in pkg mentions `tag` +// in a //go:build constraint. +func hasBuildTag(pkg *packages.Package, tag string) bool { + all := slices.Concat(pkg.CompiledGoFiles, pkg.OtherFiles, pkg.IgnoredFiles) + suffix := "_" + tag + ".go" + for _, name := range all { + if strings.HasSuffix(name, suffix) { + return true + } + ok, err := fileMentionsTag(name, tag) + if err != nil { + log.Printf("reading %s: %v", name, err) + continue + } + if ok { + return true + } + } + return false +} + +// tagSet is a set of build tags. +// The values are always true. We avoid non-std set types +// to make this faster to "go run" on empty caches. +type tagSet map[string]bool + +var ( + mu sync.Mutex + fileTags = map[string]tagSet{} // abs path -> set of build tags mentioned in file +) + +func getFileTags(filename string) (tagSet, error) { + mu.Lock() + tags, ok := fileTags[filename] + mu.Unlock() + if ok { + return tags, nil + } + + f, err := os.Open(filename) + if err != nil { + return nil, err + } + defer f.Close() + + ts := make(tagSet) + s := bufio.NewScanner(f) + for s.Scan() { + line := s.Text() + if strings.TrimSpace(line) == "" { + continue // still in leading blank lines + } + if !strings.HasPrefix(line, "//") { + // hit real code – done with header comments + // TODO(bradfitz): care about /* */ comments? + break + } + if !strings.HasPrefix(line, "//go:build") { + continue // some other comment + } + expr, err := constraint.Parse(line) + if err != nil { + return nil, fmt.Errorf("parsing %q: %w", line, err) + } + // Call Eval to populate ts with the tags mentioned in the expression. + // We don't care about the result, just the side effect of populating ts. + expr.Eval(func(tag string) bool { + ts[tag] = true + return true // arbitrary + }) + } + if err := s.Err(); err != nil { + return nil, fmt.Errorf("reading %s: %w", filename, err) + } + + mu.Lock() + defer mu.Unlock() + fileTags[filename] = ts + return tags, nil +} + +func fileMentionsTag(filename, tag string) (bool, error) { + tags, err := getFileTags(filename) + if err != nil { + return false, err + } + return tags[tag], nil +}