From 6dbc754ed6802a4ab81d5e6cd8697cf5e2c261bc Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 29 Dec 2023 13:22:52 -0800 Subject: [PATCH] Retool typecheck to be simpler Instead of walking paths ourselves, just let Go's packages library do it. This is a slight CLI change - it wants "./foo" rather than "foo". This also flagged a few things which seem to be legit failures. --- hack/verify-typecheck.sh | 2 +- pkg/controller/job/indexed_job_utils_test.go | 4 +- pkg/kubelet/util/util_windows_test.go | 6 - pkg/util/filesystem/util_windows_test.go | 2 + test/typecheck/main.go | 171 +++++++------------ test/typecheck/main_test.go | 22 +-- 6 files changed, 63 insertions(+), 144 deletions(-) diff --git a/hack/verify-typecheck.sh b/hack/verify-typecheck.sh index 685e50b24ae..af9585a1b2a 100755 --- a/hack/verify-typecheck.sh +++ b/hack/verify-typecheck.sh @@ -32,7 +32,7 @@ TYPECHECK_SERIAL="${TYPECHECK_SERIAL:-false}" go run ./test/typecheck "$@" "--serial=$TYPECHECK_SERIAL" || ret=$? if [[ $ret -ne 0 ]]; then - echo "!!! Type Check has failed. This may cause cross platform build failures." >&2 + echo "!!! Typecheck has failed. This may cause cross platform build failures." >&2 echo "!!! Please see https://git.k8s.io/kubernetes/test/typecheck for more information." >&2 exit 1 fi diff --git a/pkg/controller/job/indexed_job_utils_test.go b/pkg/controller/job/indexed_job_utils_test.go index 8800fe87563..71ab7178477 100644 --- a/pkg/controller/job/indexed_job_utils_test.go +++ b/pkg/controller/job/indexed_job_utils_test.go @@ -904,11 +904,11 @@ func TestGetIndexFailureCount(t *testing.T) { wantResult: 2, }, "valid maxint32 value": { - pod: buildPod().indexFailureCount(strconv.Itoa(math.MaxInt32)).Pod, + pod: buildPod().indexFailureCount(strconv.FormatInt(math.MaxInt32, 10)).Pod, wantResult: math.MaxInt32, }, "too large value": { - pod: buildPod().indexFailureCount(strconv.Itoa(math.MaxInt32 + 1)).Pod, + pod: buildPod().indexFailureCount(strconv.FormatInt(math.MaxInt32+1, 10)).Pod, wantResult: 0, }, "negative value": { diff --git a/pkg/kubelet/util/util_windows_test.go b/pkg/kubelet/util/util_windows_test.go index fa5ab43a5e5..9f59849f419 100644 --- a/pkg/kubelet/util/util_windows_test.go +++ b/pkg/kubelet/util/util_windows_test.go @@ -21,16 +21,10 @@ package util import ( "fmt" - "math/rand" - "net" - "os" "reflect" "runtime" - "sync" "testing" - "time" - winio "github.com/Microsoft/go-winio" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) diff --git a/pkg/util/filesystem/util_windows_test.go b/pkg/util/filesystem/util_windows_test.go index 7a4afefce42..f27880b28c9 100644 --- a/pkg/util/filesystem/util_windows_test.go +++ b/pkg/util/filesystem/util_windows_test.go @@ -20,12 +20,14 @@ limitations under the License. package filesystem import ( + "math/rand" "net" "os" "sync" "testing" "time" + winio "github.com/Microsoft/go-winio" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) diff --git a/test/typecheck/main.go b/test/typecheck/main.go index 9cb1c17a9e1..ad2416ae112 100644 --- a/test/typecheck/main.go +++ b/test/typecheck/main.go @@ -23,7 +23,6 @@ import ( "io" "log" "os" - "path/filepath" "sort" "strings" "sync" @@ -33,16 +32,16 @@ import ( ) var ( - verbose = flag.Bool("verbose", false, "print more information") - cross = flag.Bool("cross", true, "build for all platforms") - platforms = flag.String("platform", "", "comma-separated list of platforms to typecheck") - timings = flag.Bool("time", false, "output times taken for each phase") - defuses = flag.Bool("defuse", false, "output defs/uses") - serial = flag.Bool("serial", false, "don't type check platforms in parallel (equivalent to --parallel=1)") - parallel = flag.Int("parallel", 2, "limits how many platforms can be checked in parallel. 0 means no limit.") - skipTest = flag.Bool("skip-test", false, "don't type check test code") - tags = flag.String("tags", "", "comma-separated list of build tags to apply in addition to go's defaults") - ignoreDirs = flag.String("ignore-dirs", "", "comma-separated list of directories to ignore in addition to the default hardcoded list including staging, vendor, and hidden dirs") + verbose = flag.Bool("verbose", false, "print more information") + cross = flag.Bool("cross", true, "build for all platforms") + platforms = flag.String("platform", "", "comma-separated list of platforms to typecheck") + timings = flag.Bool("time", false, "output times taken for each phase") + defuses = flag.Bool("defuse", false, "output defs/uses") + serial = flag.Bool("serial", false, "don't type check platforms in parallel (equivalent to --parallel=1)") + parallel = flag.Int("parallel", 2, "limits how many platforms can be checked in parallel. 0 means no limit.") + skipTest = flag.Bool("skip-test", false, "don't type check test code") + tags = flag.String("tags", "", "comma-separated list of build tags to apply in addition to go's defaults") + ignorePatterns = flag.String("ignore", "", "comma-separated list of Go patterns to ignore") // When processed in order, windows and darwin are early to make // interesting OS-based errors happen earlier. @@ -54,27 +53,6 @@ var ( "linux/ppc64le", "linux/s390x", "windows/arm64", } - - // directories we always ignore - standardIgnoreDirs = []string{ - // Staging code is symlinked from vendor/k8s.io, and uses import - // paths as if it were inside of vendor/. It fails typechecking - // inside of staging/, but works when typechecked as part of vendor/. - "staging", - // OS-specific vendor code tends to be imported by OS-specific - // packages. We recursively typecheck imported vendored packages for - // each OS, but don't typecheck everything for every OS. - "vendor", - "_output", - // This is a weird one. /testdata/ is *mostly* ignored by Go, - // and this translates to kubernetes/vendor not working. - // edit/record.go doesn't compile without gopkg.in/yaml.v2 - // in $GOSRC/$GOROOT (both typecheck and the shell script). - "pkg/kubectl/cmd/testdata/edit", - // Tools we use for maintaining the code base but not necessarily - // ship as part of the release - "hack/tools", - } ) func newConfig(platform string) *packages.Config { @@ -102,95 +80,26 @@ func newConfig(platform string) *packages.Config { } } -type collector struct { - dirs []string - ignoreDirs []string -} - -func newCollector(ignoreDirs string) collector { - c := collector{ - ignoreDirs: append([]string(nil), standardIgnoreDirs...), - } - if ignoreDirs != "" { - c.ignoreDirs = append(c.ignoreDirs, strings.Split(ignoreDirs, ",")...) - } - return c -} - -func (c *collector) walk(roots []string) error { - for _, root := range roots { - err := filepath.Walk(root, c.handlePath) - if err != nil { - return err - } - } - sort.Strings(c.dirs) - return nil -} - -// handlePath walks the filesystem recursively, collecting directories, -// ignoring some unneeded directories (hidden/vendored) that are handled -// specially later. -func (c *collector) handlePath(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if info.IsDir() { - name := info.Name() - // Ignore hidden directories (.git, .cache, etc) - if (len(name) > 1 && (name[0] == '.' || name[0] == '_')) || name == "testdata" { - if *verbose { - fmt.Printf("DBG: skipping dir %s\n", path) - } - return filepath.SkipDir - } - for _, dir := range c.ignoreDirs { - if path == dir { - if *verbose { - fmt.Printf("DBG: ignoring dir %s\n", path) - } - return filepath.SkipDir - } - } - // Make dirs into relative pkg names. - // NOTE: can't use filepath.Join because it elides the leading "./" - pkg := path - if !strings.HasPrefix(pkg, "./") { - pkg = "./" + pkg - } - c.dirs = append(c.dirs, pkg) - if *verbose { - fmt.Printf("DBG: added dir %s\n", path) - } - } - return nil -} - -func (c *collector) verify(plat string) ([]string, error) { +func verify(plat string, patterns []string, ignore map[string]bool) ([]string, error) { errors := []packages.Error{} start := time.Now() config := newConfig(plat) - rootPkgs, err := packages.Load(config, c.dirs...) + pkgs, err := packages.Load(config, patterns...) if err != nil { return nil, err } // Recursively import all deps and flatten to one list. allMap := map[string]*packages.Package{} - for _, pkg := range rootPkgs { + for _, pkg := range pkgs { + if ignore[pkg.PkgPath] { + continue + } if *verbose { serialFprintf(os.Stdout, "pkg %q has %d GoFiles\n", pkg.PkgPath, len(pkg.GoFiles)) } - allMap[pkg.PkgPath] = pkg - if len(pkg.Imports) > 0 { - for _, imp := range pkg.Imports { - if *verbose { - serialFprintf(os.Stdout, "pkg %q imports %q\n", pkg.PkgPath, imp.PkgPath) - } - allMap[imp.PkgPath] = imp - } - } + accumulate(pkg, allMap) } keys := make([]string, 0, len(allMap)) for k := range allMap { @@ -225,6 +134,19 @@ func (c *collector) verify(plat string) ([]string, error) { return dedup(errors), nil } +func accumulate(pkg *packages.Package, allMap map[string]*packages.Package) { + allMap[pkg.PkgPath] = pkg + for _, imp := range pkg.Imports { + if allMap[imp.PkgPath] != nil { + continue + } + if *verbose { + serialFprintf(os.Stdout, "pkg %q imports %q\n", pkg.PkgPath, imp.PkgPath) + } + accumulate(imp, allMap) + } +} + func dedup(errors []packages.Error) []string { ret := []string{} @@ -247,6 +169,24 @@ func serialFprintf(w io.Writer, format string, a ...interface{}) { _, _ = fmt.Fprintf(w, format, a...) } +func resolvePkgs(patterns ...string) (map[string]bool, error) { + config := &packages.Config{ + Mode: packages.NeedName, + } + pkgs, err := packages.Load(config, patterns...) + if err != nil { + return nil, err + } + paths := map[string]bool{} + for _, p := range pkgs { + // ignore list errors (e.g. doesn't exist) + if len(p.Errors) == 0 { + paths[p.PkgPath] = true + } + } + return paths, nil +} + func main() { flag.Parse() args := flag.Args() @@ -256,13 +196,16 @@ func main() { } if len(args) == 0 { - args = append(args, ".") + args = append(args, "./...") } - c := newCollector(*ignoreDirs) - - if err := c.walk(args); err != nil { - log.Fatalf("Error walking: %v", err) + ignore := []string{} + if *ignorePatterns != "" { + ignore = append(ignore, strings.Split(*ignorePatterns, ",")...) + } + ignorePkgs, err := resolvePkgs(ignore...) + if err != nil { + log.Fatalf("failed to resolve ignored packages: %v", err) } plats := crossPlatforms[:] @@ -295,7 +238,7 @@ func main() { f := false serialFprintf(os.Stdout, "type-checking %s\n", plat) - errors, err := c.verify(plat) + errors, err := verify(plat, args, ignorePkgs) if err != nil { serialFprintf(os.Stderr, "ERROR(%s): failed to verify: %v\n", plat, err) f = true diff --git a/test/typecheck/main_test.go b/test/typecheck/main_test.go index 817f43f9061..029688857d3 100644 --- a/test/typecheck/main_test.go +++ b/test/typecheck/main_test.go @@ -17,7 +17,6 @@ limitations under the License. package main import ( - "errors" "flag" "os" "path/filepath" @@ -43,12 +42,7 @@ func TestVerify(t *testing.T) { } for _, tc := range tcs { - c := newCollector("") - if err := c.walk([]string{tc.path}); err != nil { - t.Fatalf("error walking %s: %v", tc.path, err) - } - - errs, err := c.verify("linux/amd64") + errs, err := verify("linux/amd64", []string{tc.path}, nil) if err != nil { t.Errorf("unexpected error: %v", err) } else if len(errs) != tc.expect { @@ -72,20 +66,6 @@ func setEnvVars(t testing.TB) { } } -func TestHandlePath(t *testing.T) { - c := collector{ - ignoreDirs: standardIgnoreDirs, - } - e := errors.New("ex") - i, _ := os.Stat(".") // i.IsDir() == true - if c.handlePath("foo", nil, e) != e { - t.Error("handlePath not returning errors") - } - if c.handlePath("vendor", i, nil) != filepath.SkipDir { - t.Error("should skip vendor") - } -} - func TestDedup(t *testing.T) { testcases := []struct { input []packages.Error