From c7b5dfa5c55e82754196040bebefbbcd8d681c29 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Thu, 26 Oct 2017 14:39:40 -0400 Subject: [PATCH] Gazelle: packages.Walk now calls back in all subdirectories Walk now calls its callback in all directories it traverses, whether or not they have buildable code or are outside of the subtrees Gazelle was asked to update. This is needed for import path indexing, since we'll need to scan rules in all directories. Related #859 --- go/tools/gazelle/gazelle/main.go | 12 +- go/tools/gazelle/packages/fileinfo_go_test.go | 2 +- go/tools/gazelle/packages/walk.go | 121 +++++++++++------- go/tools/gazelle/packages/walk_test.go | 42 +++--- go/tools/gazelle/rules/generator_test.go | 5 +- 5 files changed, 114 insertions(+), 68 deletions(-) diff --git a/go/tools/gazelle/gazelle/main.go b/go/tools/gazelle/gazelle/main.go index 8a59d5c5aa..a1d206422a 100644 --- a/go/tools/gazelle/gazelle/main.go +++ b/go/tools/gazelle/gazelle/main.go @@ -71,7 +71,7 @@ type visitor interface { // Gazelle processes. "pkg" describes the buildable Go code. It will not // be nil. "oldFile" is the existing build file in the visited directory. // It may be nil if no file is present. - visit(c *config.Config, pkg *packages.Package, oldFile *bf.File) + visit(rel string, c *config.Config, pkg *packages.Package, oldFile *bf.File, isUpdateDir bool) // finish is called once after all directories have been visited. finish() @@ -116,7 +116,10 @@ type hierarchicalVisitor struct { shouldProcessRoot, didProcessRoot bool } -func (v *hierarchicalVisitor) visit(c *config.Config, pkg *packages.Package, oldFile *bf.File) { +func (v *hierarchicalVisitor) visit(_ string, c *config.Config, pkg *packages.Package, oldFile *bf.File, _ bool) { + if pkg == nil { + return + } g := rules.NewGenerator(c, v.r, v.l, pkg.Rel, oldFile) rules, empty := g.GenerateRules(pkg) genFile := &bf.File{ @@ -156,7 +159,10 @@ type flatVisitor struct { oldRootFile *bf.File } -func (v *flatVisitor) visit(c *config.Config, pkg *packages.Package, oldFile *bf.File) { +func (v *flatVisitor) visit(_ string, c *config.Config, pkg *packages.Package, oldFile *bf.File, _ bool) { + if pkg == nil { + return + } if pkg.Rel == "" { v.oldRootFile = oldFile } diff --git a/go/tools/gazelle/packages/fileinfo_go_test.go b/go/tools/gazelle/packages/fileinfo_go_test.go index 0a72e95f99..e3c1fa306f 100644 --- a/go/tools/gazelle/packages/fileinfo_go_test.go +++ b/go/tools/gazelle/packages/fileinfo_go_test.go @@ -357,7 +357,7 @@ import "C" t.Fatal(err) } c := &config.Config{RepoRoot: repo} - got := buildPackage(c, sub, []string{"sub.go"}, nil, nil, false) + got := buildPackage(c, sub, "sub", []string{"sub.go"}, nil, nil, false) want := &Package{ Name: "sub", Dir: sub, diff --git a/go/tools/gazelle/packages/walk.go b/go/tools/gazelle/packages/walk.go index 22a5fb5493..b9b7caa971 100644 --- a/go/tools/gazelle/packages/walk.go +++ b/go/tools/gazelle/packages/walk.go @@ -28,36 +28,78 @@ import ( "github.com/bazelbuild/rules_go/go/tools/gazelle/config" ) -// A WalkFunc is a callback called by Walk for each package. -type WalkFunc func(c *config.Config, pkg *Package, oldFile *bf.File) +// A WalkFunc is a callback called by Walk in each visited directory. +// +// rel is the relative slash-separated path to the directory from the +// repository root. Will be "" for the repository root directory itself. +// +// c is the configuration for the current directory. This may have been +// modified by directives in the directory's build file. +// +// pkg contains information about how to build source code in the directory. +// Will be nil for directories that don't contain buildable code, directories +// that Gazelle was not asked update, and directories where Walk +// encountered errors. +// +// oldFile is the existing build file in the directory. Will be nil if there +// was no file. +// +// isUpdateDir is true for directories that Gazelle was asked to update. +type WalkFunc func(rel string, c *config.Config, pkg *Package, oldFile *bf.File, isUpdateDir bool) -// Walk walks through directories under "root". -// It calls back "f" for each package. If an existing BUILD file is present -// in the directory, it will be parsed and passed to "f" as well. +// Walk traverses a directory tree. In each directory, Walk parses existing +// build files. In directories that Gazelle was asked to update (c.Dirs), Walk +// also parses source files and infers build information. +// +// c is the base configuration for the repository. c may be copied and modified +// by directives found in build files. // -// Walk is similar to "golang.org/x/tools/go/buildutil".ForEachPackage, but -// it does not assume the standard Go tree because Bazel rules_go uses -// go_prefix instead of the standard tree. +// root is an absolute file path to the directory to traverse. // -// If a directory contains no buildable Go code, "f" is not called. If a -// directory contains one package with any name, "f" will be called with that -// package. If a directory contains multiple packages and one of the package -// names matches the directory name, "f" will be called on that package and the -// other packages will be silently ignored. If none of the package names match -// the directory name, or if some other error occurs, an error will be logged, -// and "f" will not be called. -func Walk(c *config.Config, dir string, f WalkFunc) { +// f is a function that will be called for each visited directory. +func Walk(c *config.Config, root string, f WalkFunc) { + // Determine relative paths for the directories to be updated. + var updateRels []string + for _, dir := range c.Dirs { + rel, err := filepath.Rel(c.RepoRoot, dir) + if err != nil { + // This should have been verified when c was built. + log.Panicf("%s: not a subdirectory of repository root %q", dir, c.RepoRoot) + } + rel = filepath.ToSlash(rel) + if rel == "." || rel == "/" { + rel = "" + } + updateRels = append(updateRels, rel) + } + rootRel, err := filepath.Rel(c.RepoRoot, root) + if err != nil { + log.Panicf("%s: not a subdirectory of repository root %q", root, c.RepoRoot) + } + if rootRel == "." || rootRel == "/" { + rootRel = "" + } + // visit walks the directory tree in post-order. It returns whether the - // the directory it was called on or any subdirectory contains a Bazel - // package. This affects whether "testdata" directories are considered + // given directory or any subdirectory contained a build file or buildable + // source code. This affects whether "testdata" directories are considered // data dependencies. - var visit func(string) bool - visit = func(path string) bool { + var visit func(string, string, bool) bool + visit = func(dir, rel string, isUpdateDir bool) bool { + // Check if this directory should be updated. + if !isUpdateDir { + for _, updateRel := range updateRels { + if updateRel == "" || rel == updateRel || strings.HasPrefix(rel, updateRel+"/") { + isUpdateDir = true + } + } + } + // Look for an existing BUILD file. var oldFile *bf.File haveError := false for _, base := range c.ValidBuildFileNames { - oldPath := filepath.Join(path, base) + oldPath := filepath.Join(dir, base) st, err := os.Stat(oldPath) if os.IsNotExist(err) || err == nil && st.IsDir() { continue @@ -70,7 +112,7 @@ func Walk(c *config.Config, dir string, f WalkFunc) { } if oldFile != nil { log.Printf("in directory %s, multiple Bazel files are present: %s, %s", - path, filepath.Base(oldFile.Path), base) + dir, filepath.Base(oldFile.Path), base) haveError = true continue } @@ -98,7 +140,7 @@ func Walk(c *config.Config, dir string, f WalkFunc) { } // List files and subdirectories. - files, err := ioutil.ReadDir(path) + files, err := ioutil.ReadDir(dir) if err != nil { log.Print(err) return false @@ -113,7 +155,7 @@ func Walk(c *config.Config, dir string, f WalkFunc) { switch { case base == "" || base[0] == '.' || base[0] == '_' || excluded[base] || - base == "vendor" && f.IsDir() && c.DepMode != config.VendorMode: + base == "vendor" && f.IsDir() && c.DepMode == config.ExternalMode: continue case f.IsDir(): @@ -132,7 +174,7 @@ func Walk(c *config.Config, dir string, f WalkFunc) { hasTestdata := false subdirHasPackage := false for _, sub := range subdirs { - hasPackage := visit(filepath.Join(path, sub)) + hasPackage := visit(filepath.Join(dir, sub), path.Join(rel, sub), isUpdateDir) if sub == "testdata" && !hasPackage { hasTestdata = true } @@ -140,7 +182,8 @@ func Walk(c *config.Config, dir string, f WalkFunc) { } hasPackage := subdirHasPackage || oldFile != nil - if haveError { + if haveError || !isUpdateDir { + f(rel, c, nil, oldFile, isUpdateDir) return hasPackage } @@ -149,15 +192,12 @@ func Walk(c *config.Config, dir string, f WalkFunc) { if oldFile != nil { genFiles = findGenFiles(oldFile, excluded) } - pkg := buildPackage(c, path, pkgFiles, otherFiles, genFiles, hasTestdata) - if pkg != nil { - f(c, pkg, oldFile) - hasPackage = true - } - return hasPackage + pkg := buildPackage(c, dir, rel, pkgFiles, otherFiles, genFiles, hasTestdata) + f(rel, c, pkg, oldFile, isUpdateDir) + return hasPackage || pkg != nil } - visit(dir) + visit(root, rootRel, false) } // buildPackage reads source files in a given directory and returns a Package @@ -168,17 +208,7 @@ func Walk(c *config.Config, dir string, f WalkFunc) { // name matches the directory base name will be returned. If there is no such // package or if an error occurs, an error will be logged, and nil will be // returned. -func buildPackage(c *config.Config, dir string, pkgFiles, otherFiles, genFiles []string, hasTestdata bool) *Package { - rel, err := filepath.Rel(c.RepoRoot, dir) - if err != nil { - log.Print(err) - return nil - } - rel = filepath.ToSlash(rel) - if rel == "." { - rel = "" - } - +func buildPackage(c *config.Config, dir, rel string, pkgFiles, otherFiles, genFiles []string, hasTestdata bool) *Package { // Process .go and .proto files first, since these determine the package name. packageMap := make(map[string]*Package) cgo := false @@ -212,8 +242,7 @@ func buildPackage(c *config.Config, dir string, pkgFiles, otherFiles, genFiles [ HasTestdata: hasTestdata, } } - err = packageMap[info.packageName].addFile(c, info, false) - if err != nil { + if err := packageMap[info.packageName].addFile(c, info, false); err != nil { log.Print(err) } } diff --git a/go/tools/gazelle/packages/walk_test.go b/go/tools/gazelle/packages/walk_test.go index 2866ca9416..c02581aa6b 100644 --- a/go/tools/gazelle/packages/walk_test.go +++ b/go/tools/gazelle/packages/walk_test.go @@ -47,7 +47,13 @@ func checkFiles(t *testing.T, files []fileSpec, goPrefix string, want []*package p.Dir = filepath.Join(dir, filepath.FromSlash(p.Rel)) } - got := walkPackages(dir, goPrefix, dir) + c := &config.Config{ + RepoRoot: dir, + GoPrefix: goPrefix, + Dirs: []string{dir}, + ValidBuildFileNames: config.DefaultValidBuildFileNames, + } + got := walkPackages(c) checkPackages(t, got, want) } @@ -74,15 +80,12 @@ func createFiles(files []fileSpec) (string, error) { return dir, nil } -func walkPackages(repoRoot, goPrefix, dir string) []*packages.Package { - c := &config.Config{ - RepoRoot: repoRoot, - GoPrefix: goPrefix, - ValidBuildFileNames: config.DefaultValidBuildFileNames, - } +func walkPackages(c *config.Config) []*packages.Package { var pkgs []*packages.Package - packages.Walk(c, dir, func(_ *config.Config, pkg *packages.Package, _ *bf.File) { - pkgs = append(pkgs, pkg) + packages.Walk(c, c.RepoRoot, func(_ string, _ *config.Config, pkg *packages.Package, _ *bf.File, _ bool) { + if pkg != nil { + pkgs = append(pkgs, pkg) + } }) return pkgs } @@ -214,7 +217,12 @@ func TestMultiplePackagesWithoutDefault(t *testing.T) { } defer os.RemoveAll(dir) - got := walkPackages(dir, "", dir) + c := &config.Config{ + RepoRoot: dir, + Dirs: []string{dir}, + ValidBuildFileNames: config.DefaultValidBuildFileNames, + } + got := walkPackages(c) if len(got) > 0 { t.Errorf("got %v; want empty slice", got) } @@ -270,7 +278,12 @@ func TestRootWithoutPrefix(t *testing.T) { } defer os.RemoveAll(dir) - got := walkPackages(dir, "", dir) + c := &config.Config{ + RepoRoot: dir, + Dirs: []string{dir}, + ValidBuildFileNames: config.DefaultValidBuildFileNames, + } + got := walkPackages(c) if len(got) > 0 { t.Errorf("got %v; want empty slice", got) } @@ -645,15 +658,12 @@ func TestVendor(t *testing.T) { c := &config.Config{ RepoRoot: dir, + Dirs: []string{dir}, GoPrefix: "", ValidBuildFileNames: config.DefaultValidBuildFileNames, DepMode: tc.mode, } - var got []*packages.Package - packages.Walk(c, dir, func(_ *config.Config, pkg *packages.Package, _ *bf.File) { - got = append(got, pkg) - }) - + got := walkPackages(c) checkPackages(t, got, tc.want) }) } diff --git a/go/tools/gazelle/rules/generator_test.go b/go/tools/gazelle/rules/generator_test.go index 1d1b53a450..83e7cbfbfc 100644 --- a/go/tools/gazelle/rules/generator_test.go +++ b/go/tools/gazelle/rules/generator_test.go @@ -32,6 +32,7 @@ import ( func testConfig(repoRoot, goPrefix string) *config.Config { c := &config.Config{ RepoRoot: repoRoot, + Dirs: []string{repoRoot}, GoPrefix: goPrefix, GenericTags: config.BuildTags{}, ValidBuildFileNames: []string{"BUILD.old"}, @@ -44,8 +45,8 @@ func testConfig(repoRoot, goPrefix string) *config.Config { func packageFromDir(c *config.Config, dir string) (*packages.Package, *bf.File) { var pkg *packages.Package var oldFile *bf.File - packages.Walk(c, dir, func(_ *config.Config, p *packages.Package, f *bf.File) { - if p.Dir == dir { + packages.Walk(c, dir, func(rel string, _ *config.Config, p *packages.Package, f *bf.File, _ bool) { + if p != nil && p.Dir == dir { pkg = p oldFile = f }