diff --git a/internal/gps/_testdata/src/canon_confl/a.go b/internal/gps/_testdata/src/canon_confl/a.go new file mode 100644 index 0000000000..9815345118 --- /dev/null +++ b/internal/gps/_testdata/src/canon_confl/a.go @@ -0,0 +1,9 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package canonical // import "vanity1" + +var ( + A = "A" +) diff --git a/internal/gps/_testdata/src/canon_confl/b.go b/internal/gps/_testdata/src/canon_confl/b.go new file mode 100644 index 0000000000..b8b706bbd4 --- /dev/null +++ b/internal/gps/_testdata/src/canon_confl/b.go @@ -0,0 +1,9 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package canonical // import "vanity2" + +var ( + B = "B" +) diff --git a/internal/gps/_testdata/src/canonical/main.go b/internal/gps/_testdata/src/canonical/main.go new file mode 100644 index 0000000000..4fe31ce3d5 --- /dev/null +++ b/internal/gps/_testdata/src/canonical/main.go @@ -0,0 +1,9 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package pkg // import "canonical" + +var ( + A = "A" +) diff --git a/internal/gps/_testdata/src/canonical/sub/sub.go b/internal/gps/_testdata/src/canonical/sub/sub.go new file mode 100644 index 0000000000..783186dfc2 --- /dev/null +++ b/internal/gps/_testdata/src/canonical/sub/sub.go @@ -0,0 +1,5 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package sub // import "canonical/subpackage" diff --git a/internal/gps/identifier.go b/internal/gps/identifier.go index de24e88c34..d01b794755 100644 --- a/internal/gps/identifier.go +++ b/internal/gps/identifier.go @@ -41,10 +41,10 @@ import ( type ProjectRoot string // A ProjectIdentifier provides the name and source location of a dependency. It -// is related to, but differs in two keys ways from, an plain import path. +// is related to, but differs in two key ways from, a plain import path. // // First, ProjectIdentifiers do not identify a single package. Rather, they -// encompasses the whole tree of packages, including tree's root - the +// encompass the whole tree of packages, including tree's root - the // ProjectRoot. In gps' current design, this ProjectRoot almost always // corresponds to the root of a repository. // diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index f14598c286..4b15ee422b 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -5,7 +5,9 @@ package pkgtree import ( + "bytes" "fmt" + "go/ast" "go/build" "go/parser" gscan "go/scanner" @@ -123,9 +125,9 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { ip := filepath.ToSlash(filepath.Join(importRoot, strings.TrimPrefix(wp, fileRoot))) // Find all the imports, across all os/arch combos - //p, err := fullPackageInDir(wp) p := &build.Package{ - Dir: wp, + Dir: wp, + ImportPath: ip, } err = fillPackage(p) @@ -152,18 +154,26 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { } } + if pkg.CommentPath != "" && !strings.HasPrefix(pkg.CommentPath, importRoot) { + ptree.Packages[ip] = PackageOrErr{ + Err: &NonCanonicalImportRoot{ + ImportRoot: importRoot, + Canonical: pkg.CommentPath, + }, + } + return nil + } + // This area has some...fuzzy rules, but check all the imports for // local/relative/dot-ness, and record an error for the package if we // see any. var lim []string for _, imp := range append(pkg.Imports, pkg.TestImports...) { - switch { - // Do allow the single-dot, at least for now - case imp == "..": - lim = append(lim, imp) - case strings.HasPrefix(imp, "./"): - lim = append(lim, imp) - case strings.HasPrefix(imp, "../"): + if build.IsLocalImport(imp) { + // Do allow the single-dot, at least for now + if imp == "." { + continue + } lim = append(lim, imp) } } @@ -210,6 +220,7 @@ func fillPackage(p *build.Package) error { var testImports []string var imports []string + var importComments []string for _, file := range gofiles { // Skip underscore-led or dot-led files, in keeping with the rest of the toolchain. bPrefix := filepath.Base(file)[0] @@ -234,6 +245,10 @@ func fillPackage(p *build.Package) error { var ignored bool for _, c := range pf.Comments { + ic := findImportComment(pf.Name, c) + if ic != "" { + importComments = append(importComments, ic) + } if c.Pos() > pf.Package { // +build comment must come before package continue } @@ -282,7 +297,16 @@ func fillPackage(p *build.Package) error { } } } - + importComments = uniq(importComments) + if len(importComments) > 1 { + return &ConflictingImportComments{ + ImportPath: p.ImportPath, + ConflictingImportComments: importComments, + } + } + if len(importComments) > 0 { + p.ImportComment = importComments[0] + } imports = uniq(imports) testImports = uniq(testImports) p.Imports = imports @@ -290,6 +314,80 @@ func fillPackage(p *build.Package) error { return nil } +var ( + slashSlash = []byte("//") + slashStar = []byte("/*") + starSlash = []byte("*/") + importKwd = []byte("import ") +) + +func findImportComment(pkgName *ast.Ident, c *ast.CommentGroup) string { + afterPkg := pkgName.NamePos + token.Pos(len(pkgName.Name)) + 1 + commentSlash := c.List[0].Slash + if afterPkg != commentSlash { + return "" + } + text := []byte(c.List[0].Text) + switch { + case bytes.HasPrefix(text, slashSlash): + eol := bytes.IndexByte(text, '\n') + if eol < 0 { + eol = len(text) + } + text = text[2:eol] + case bytes.HasPrefix(text, slashStar): + text = text[2:] + end := bytes.Index(text, starSlash) + if end < 0 { + // malformed comment + return "" + } + text = text[:end] + if bytes.IndexByte(text, '\n') > 0 { + // multiline comment, can't be an import comment + return "" + } + } + text = bytes.TrimSpace(text) + if !bytes.HasPrefix(text, importKwd) { + return "" + } + quotedPath := bytes.TrimSpace(text[len(importKwd):]) + return string(bytes.Trim(quotedPath, `"`)) +} + +// ConflictingImportComments indicates that the package declares more than one +// different canonical path. +type ConflictingImportComments struct { + ImportPath string // An import path refering to this package + ConflictingImportComments []string // All distinct "canonical" paths encountered in the package files +} + +func (e *ConflictingImportComments) Error() string { + return fmt.Sprintf("import path %s had conflicting import comments: %s", + e.ImportPath, quotedPaths(e.ConflictingImportComments)) +} + +// NonCanonicalImportRoot reports the situation when the dependee imports a +// package via something other than the package's declared canonical path. +type NonCanonicalImportRoot struct { + ImportRoot string // A root path that is being used to import a package + Canonical string // A canonical path declared by the package being imported +} + +func (e *NonCanonicalImportRoot) Error() string { + return fmt.Sprintf("import root %q is not a prefix for the package's declared canonical path %q", + e.ImportRoot, e.Canonical) +} + +func quotedPaths(ps []string) string { + quoted := make([]string, 0, len(ps)) + for _, p := range ps { + quoted = append(quoted, fmt.Sprintf("%q", p)) + } + return strings.Join(quoted, ", ") +} + // LocalImportsError indicates that a package contains at least one relative // import that will prevent it from compiling. // @@ -308,7 +406,7 @@ func (e *LocalImportsError) Error() string { case 1: return fmt.Sprintf("import path %s had a local import: %q", e.ImportPath, e.LocalImports[0]) default: - return fmt.Sprintf("import path %s had local imports: %q", e.ImportPath, strings.Join(e.LocalImports, "\", \"")) + return fmt.Sprintf("import path %s had local imports: %s", e.ImportPath, quotedPaths(e.LocalImports)) } } diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index ab134f02e7..4e3f27268a 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -1292,6 +1292,64 @@ func TestListPackages(t *testing.T) { }, }, }, + "canonical": { + fileRoot: j("canonical"), + importRoot: "canonical", + out: PackageTree{ + ImportRoot: "canonical", + Packages: map[string]PackageOrErr{ + "canonical": { + P: Package{ + ImportPath: "canonical", + CommentPath: "canonical", + Name: "pkg", + Imports: []string{}, + }, + }, + "canonical/sub": { + P: Package{ + ImportPath: "canonical/sub", + CommentPath: "canonical/subpackage", + Name: "sub", + Imports: []string{}, + }, + }, + }, + }, + }, + "conflicting canonical comments": { + fileRoot: j("canon_confl"), + importRoot: "canon_confl", + out: PackageTree{}, + err: &ConflictingImportComments{ + ImportPath: "canon_confl", + ConflictingImportComments: []string{ + "vanity1", + "vanity2", + }, + }, + }, + "non-canonical": { + fileRoot: j("canonical"), + importRoot: "noncanonical", + out: PackageTree{ + ImportRoot: "noncanonical", + Packages: map[string]PackageOrErr{ + "noncanonical": PackageOrErr{ + Err: &NonCanonicalImportRoot{ + ImportRoot: "noncanonical", + Canonical: "canonical", + }, + }, + "noncanonical/sub": PackageOrErr{ + Err: &NonCanonicalImportRoot{ + ImportRoot: "noncanonical", + Canonical: "canonical/subpackage", + }, + }, + }, + }, + }, } for name, fix := range table {