From fa833fdef560f0fe9b40dbb37fd03030ac3d514b Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 18 Dec 2015 14:11:09 -0500 Subject: [PATCH] refactor/importgraph: changes for vendor support Each string in build.Package.{,Test,XTest}Imports must now be interpreted relative to the Package.Dir. This adds a ton of redundant I/O. Also: - use a counting semaphore to limit concurrent I/O calls (Fixes golang/go#10306) - remove obsolete call to runtime.GOMAXPROCS from the test. Change-Id: Ic556c4cf41cce7a88c0158800c992e66f354c484 Reviewed-on: https://go-review.googlesource.com/18050 Reviewed-by: Russ Cox --- refactor/importgraph/graph.go | 43 ++++++++++++++++++++++++++---- refactor/importgraph/graph_test.go | 19 ++++++++----- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/refactor/importgraph/graph.go b/refactor/importgraph/graph.go index 8ad8014c..fc8691d0 100644 --- a/refactor/importgraph/graph.go +++ b/refactor/importgraph/graph.go @@ -53,9 +53,10 @@ func (g Graph) Search(roots ...string) map[string]bool { // Build scans the specified Go workspace and builds the forward and // reverse import dependency graphs for all its packages. -// It also returns a mapping from import paths to errors for packages +// It also returns a mapping from canonical import paths to errors for packages // whose loading was not entirely successful. // A package may appear in the graph and in the errors mapping. +// All package paths are canonical and may contain "/vendor/". func Build(ctxt *build.Context) (forward, reverse Graph, errors map[string]error) { type importEdge struct { from, to string @@ -67,6 +68,7 @@ func Build(ctxt *build.Context) (forward, reverse Graph, errors map[string]error ch := make(chan interface{}) + sema := make(chan int, 20) // I/O concurrency limiting semaphore var wg sync.WaitGroup buildutil.ForEachPackage(ctxt, func(path string, err error) { wg.Add(1) @@ -77,7 +79,10 @@ func Build(ctxt *build.Context) (forward, reverse Graph, errors map[string]error return } - bp, err := ctxt.Import(path, "", 0) + sema <- 1 + bp, err := ctxt.Import(path, "", buildutil.AllowVendor) + <-sema + if err != nil { if _, ok := err.(*build.NoGoError); ok { // empty directory is not an error @@ -86,15 +91,43 @@ func Build(ctxt *build.Context) (forward, reverse Graph, errors map[string]error } // Even in error cases, Import usually returns a package. } + + // absolutize resolves an import path relative + // to the current package bp. + // The absolute form may contain "vendor". + // TODO(adonovan): opt: experiment with + // overriding the IsDir method with a caching version + // to avoid a huge number of redundant I/O calls. + absolutize := func(path string) string { return path } + if buildutil.AllowVendor != 0 { + memo := make(map[string]string) + absolutize = func(path string) string { + canon, ok := memo[path] + if !ok { + sema <- 1 + bp2, _ := ctxt.Import(path, bp.Dir, build.FindOnly|buildutil.AllowVendor) + <-sema + + if bp2 != nil { + canon = bp2.ImportPath + } else { + canon = path + } + memo[path] = canon + } + return canon + } + } + if bp != nil { for _, imp := range bp.Imports { - ch <- importEdge{path, imp} + ch <- importEdge{path, absolutize(imp)} } for _, imp := range bp.TestImports { - ch <- importEdge{path, imp} + ch <- importEdge{path, absolutize(imp)} } for _, imp := range bp.XTestImports { - ch <- importEdge{path, imp} + ch <- importEdge{path, absolutize(imp)} } } }() diff --git a/refactor/importgraph/graph_test.go b/refactor/importgraph/graph_test.go index a486c26b..595ecdb6 100644 --- a/refactor/importgraph/graph_test.go +++ b/refactor/importgraph/graph_test.go @@ -9,11 +9,13 @@ package importgraph_test import ( + "fmt" "go/build" - "runtime" "sort" + "strings" "testing" + "golang.org/x/tools/go/buildutil" "golang.org/x/tools/refactor/importgraph" _ "crypto/hmac" // just for test, below @@ -22,15 +24,12 @@ import ( const this = "golang.org/x/tools/refactor/importgraph" func TestBuild(t *testing.T) { - saved := runtime.GOMAXPROCS(8) // Build is highly parallel - defer runtime.GOMAXPROCS(saved) - forward, reverse, errors := importgraph.Build(&build.Default) // Test direct edges. // We throw in crypto/hmac to prove that external test files // (such as this one) are inspected. - for _, p := range []string{"go/build", "runtime", "testing", "crypto/hmac"} { + for _, p := range []string{"go/build", "testing", "crypto/hmac"} { if !forward[this][p] { t.Errorf("forward[importgraph][%s] not found", p) } @@ -40,7 +39,7 @@ func TestBuild(t *testing.T) { } // Test non-existent direct edges - for _, p := range []string{"fmt", "errors", "reflect"} { + for _, p := range []string{"errors", "reflect"} { if forward[this][p] { t.Errorf("unexpected: forward[importgraph][%s] found", p) } @@ -49,6 +48,14 @@ func TestBuild(t *testing.T) { } } + // Test vendor packages appear under their absolute names. + if buildutil.AllowVendor != 0 { // hack: Go 1.6+ only + if !forward["net/http"]["vendor/golang.org/x/net/http2/hpack"] { + t.Errorf("forward[net/http] does not include vendor/golang.org/x/net/http2/hpack: %v", + strings.Replace(fmt.Sprint(forward["net/http"]), ":true", "", -1)) + } + } + // Test Search is reflexive. if !forward.Search(this)[this] { t.Errorf("irreflexive: forward.Search(importgraph)[importgraph] not found")