go/packages: handle multiple modules in gopackagestest
Adding a second module to the gopackagestest configuration exposed a
flaky failure. go/packages was attempting to construct an ID relative to
a module. Depending on which module it saw first, it could construct a
relative path based on the incorrect module. Add an additional module to
the overlay tests to make sure we don't regress here.
Also, fix up a few staticcheck errors that were spotted along the way.
Change-Id: I53c2c0eee62ff88eadcb96a3770452dd7799312e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/199645
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
diff --git a/go/packages/golist.go b/go/packages/golist.go
index 6b341b7..ff0f55e 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -294,15 +294,16 @@
// Special case to handle issue #33482:
// If this is a file= query for ad-hoc packages where the file only exists on an overlay,
// and exists outside of a module, add the file in for the package.
- if len(dirResponse.Packages) == 1 && len(dirResponse.Packages) == 1 &&
- dirResponse.Packages[0].ID == "command-line-arguments" && len(dirResponse.Packages[0].GoFiles) == 0 {
- filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
- // TODO(matloob): check if the file is outside of a root dir?
- for path := range cfg.Overlay {
- if path == filename {
- dirResponse.Packages[0].Errors = nil
- dirResponse.Packages[0].GoFiles = []string{path}
- dirResponse.Packages[0].CompiledGoFiles = []string{path}
+ if len(dirResponse.Packages) == 1 && (dirResponse.Packages[0].ID == "command-line-arguments" || dirResponse.Packages[0].PkgPath == filepath.ToSlash(query)) {
+ if len(dirResponse.Packages[0].GoFiles) == 0 {
+ filename := filepath.Join(pattern, filepath.Base(query)) // avoid recomputing abspath
+ // TODO(matloob): check if the file is outside of a root dir?
+ for path := range cfg.Overlay {
+ if path == filename {
+ dirResponse.Packages[0].Errors = nil
+ dirResponse.Packages[0].GoFiles = []string{path}
+ dirResponse.Packages[0].CompiledGoFiles = []string{path}
+ }
}
}
}
@@ -682,7 +683,7 @@
// contained in a known module or GOPATH entry. This will allow the package to be
// properly "reclaimed" when overlays are processed.
if filepath.IsAbs(p.ImportPath) && p.Error != nil {
- pkgPath, ok := getPkgPath(p.ImportPath, rootsDirs)
+ pkgPath, ok := getPkgPath(cfg, p.ImportPath, rootsDirs)
if ok {
p.ImportPath = pkgPath
}
@@ -792,15 +793,31 @@
}
// getPkgPath finds the package path of a directory if it's relative to a root directory.
-func getPkgPath(dir string, goInfo func() *goInfo) (string, bool) {
+func getPkgPath(cfg *Config, dir string, goInfo func() *goInfo) (string, bool) {
+ absDir, err := filepath.Abs(dir)
+ if err != nil {
+ cfg.Logf("error getting absolute path of %s: %v", dir, err)
+ return "", false
+ }
for rdir, rpath := range goInfo().rootDirs {
+ absRdir, err := filepath.Abs(rdir)
+ if err != nil {
+ cfg.Logf("error getting absolute path of %s: %v", rdir, err)
+ continue
+ }
+ // Make sure that the directory is in the module,
+ // to avoid creating a path relative to another module.
+ if !strings.HasPrefix(absDir, absRdir) {
+ cfg.Logf("%s does not have prefix %s", absDir, absRdir)
+ continue
+ }
// TODO(matloob): This doesn't properly handle symlinks.
r, err := filepath.Rel(rdir, dir)
if err != nil {
continue
}
if rpath != "" {
- // We choose only ore root even though the directory even it can belong in multiple modules
+ // We choose only one root even though the directory even it can belong in multiple modules
// or GOPATH entries. This is okay because we only need to work with absolute dirs when a
// file is missing from disk, for instance when gopls calls go/packages in an overlay.
// Once the file is saved, gopls, or the next invocation of the tool will get the correct
@@ -808,6 +825,7 @@
// TODO(matloob): Implement module tiebreaking?
return path.Join(rpath, filepath.ToSlash(r)), true
}
+ return filepath.ToSlash(r), true
}
return "", false
}
@@ -859,7 +877,7 @@
cmd.Stdout = stdout
cmd.Stderr = stderr
defer func(start time.Time) {
- cfg.Logf("%s for %v, stderr: <<%s>>\n", time.Since(start), cmdDebugStr(cmd, args...), stderr)
+ cfg.Logf("%s for %v, stderr: <<%s>> stdout: <<%s>>\n", time.Since(start), cmdDebugStr(cmd, args...), stderr, stdout)
}(time.Now())
if err := cmd.Run(); err != nil {
@@ -987,12 +1005,6 @@
if len(stderr.Bytes()) != 0 && os.Getenv("GOPACKAGESPRINTGOLISTERRORS") != "" {
fmt.Fprintf(os.Stderr, "%s stderr: <<%s>>\n", cmdDebugStr(cmd, args...), stderr)
}
-
- // debugging
- if false {
- fmt.Fprintf(os.Stderr, "%s stdout: <<%s>>\n", cmdDebugStr(cmd, args...), stdout)
- }
-
return stdout, nil
}
diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go
index 308e79c..a7de622 100644
--- a/go/packages/golist_overlay.go
+++ b/go/packages/golist_overlay.go
@@ -6,7 +6,6 @@
"fmt"
"go/parser"
"go/token"
- "path"
"path/filepath"
"strconv"
"strings"
@@ -87,26 +86,10 @@
if pkg == nil {
// Try to find the module or gopath dir the file is contained in.
// Then for modules, add the module opath to the beginning.
- var pkgPath string
- for rdir, rpath := range rootDirs().rootDirs {
- // TODO(matloob): This doesn't properly handle symlinks.
- r, err := filepath.Rel(rdir, dir)
- if err != nil {
- continue
- }
- pkgPath = filepath.ToSlash(r)
- if rpath != "" {
- pkgPath = path.Join(rpath, pkgPath)
- }
- // We only create one new package even it can belong in multiple modules or GOPATH entries.
- // This is okay because tools (such as the LSP) that use overlays will recompute the overlay
- // once the file is saved, and golist will do the right thing.
- // TODO(matloob): Implement module tiebreaking?
+ pkgPath, ok := getPkgPath(cfg, dir, rootDirs)
+ if !ok {
break
}
- if pkgPath == "" {
- continue
- }
isXTest := strings.HasSuffix(pkgName, "_test")
if isXTest {
pkgPath += "_test"
diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go
index 4b9720a..2a73156 100644
--- a/go/packages/packages_test.go
+++ b/go/packages/packages_test.go
@@ -983,14 +983,23 @@
func TestNewPackagesInOverlay(t *testing.T) { packagestest.TestAll(t, testNewPackagesInOverlay) }
func testNewPackagesInOverlay(t *testing.T, exporter packagestest.Exporter) {
- exported := packagestest.Export(t, exporter, []packagestest.Module{{
- Name: "golang.org/fake",
- Files: map[string]interface{}{
- "a/a.go": `package a; import "golang.org/fake/b"; const A = "a" + b.B`,
- "b/b.go": `package b; import "golang.org/fake/c"; const B = "b" + c.C`,
- "c/c.go": `package c; const C = "c"`,
- "d/d.go": `package d; const D = "d"`,
- }}})
+ exported := packagestest.Export(t, exporter, []packagestest.Module{
+ {
+ Name: "golang.org/fake",
+ Files: map[string]interface{}{
+ "a/a.go": `package a; import "golang.org/fake/b"; const A = "a" + b.B`,
+ "b/b.go": `package b; import "golang.org/fake/c"; const B = "b" + c.C`,
+ "c/c.go": `package c; const C = "c"`,
+ "d/d.go": `package d; const D = "d"`,
+ },
+ },
+ {
+ Name: "example.com/extramodule",
+ Files: map[string]interface{}{
+ "pkg/x.go": "package pkg\n",
+ },
+ },
+ })
defer exported.Cleanup()
dir := filepath.Dir(filepath.Dir(exported.File("golang.org/fake", "a/a.go")))
@@ -1120,7 +1129,7 @@
// This test doesn't use packagestest because we are testing ad-hoc packages,
// which are outside of $GOPATH and outside of a module.
- tmp, err := ioutil.TempDir("", "a")
+ tmp, err := ioutil.TempDir("", "testAdHocOverlays")
if err != nil {
t.Fatal(err)
}
@@ -1137,10 +1146,14 @@
Overlay: map[string][]byte{
filename: content,
},
+ Logf: t.Logf,
}
initial, err := packages.Load(config, fmt.Sprintf("file=%s", filename))
if err != nil {
- t.Error(err)
+ t.Fatal(err)
+ }
+ if len(initial) == 0 {
+ t.Fatalf("no packages for %s", filename)
}
// Check value of a.A.
a := initial[0]
