From ce78a4731e48f22f526a13a688579afe29870197 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 12 Oct 2017 13:57:44 -0400 Subject: [PATCH 01/11] pkgtree: add TrimHiddenPackages() method This has general utility, independent of ListPackages() being improved to avoid unnecessary disk reads in typically-hidden directories. --- internal/gps/pkgtree/pkgtree.go | 42 +++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 78be8b42f4..6e2e3a45ec 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -634,6 +634,48 @@ func (t PackageTree) Copy() PackageTree { return t2 } +// TrimHiddenPackages returns a new PackageTree with packages removed that +// are either ignored, or both hidden and unreachable. +// +// The package list is partitioned into two sets: normal, and hidden, where +// packages are considered hidden if they are within or beneath directories +// with: +// +// * leading dots +// * leading underscores +// * the exact name "testdata" +// +// Packages in the hidden set are dropped from the returned PackageTree, unless +// they are transitively reachable from imports in the normal set. +// +// The "main", "tests" and "ignored" parameters have the same behavior as with +// PackageTree.ToReachMap(): the first two determine, respectively, whether +// imports from main packages, and imports from tests, should be considered for +// reachability checks. "ignored" designates packages, or patterns of packages, +// whose imports should be excluded from reachability checks, if encountered. +func (t PackageTree) TrimHiddenPackages(main, tests bool, ignore map[string]bool) PackageTree { + rm, _ := t.ToReachMap(main, tests, false, ignore) + t2 := t.Copy() + + preserve := make(map[string]bool) + for pkg, ie := range rm { + if !pkgFilter(pkg) { + preserve[pkg] = true + for _, in := range ie.Internal { + preserve[in] = true + } + } + } + + for ip := range t.Packages { + if !preserve[pkg] { + delete(t2.Packages, ip) + } + } + + return t2 +} + // wmToReach takes an internal "workmap" constructed by // PackageTree.ExternalReach(), transitively walks (via depth-first traversal) // all internal imports until they reach an external path or terminate, then From 89bd85d1cbce9f76013e7a91e4e20dae415536ff Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 01:21:39 -0400 Subject: [PATCH 02/11] gps: Put finishing touches on TrimHiddenPackages --- internal/gps/pkgtree/pkgtree.go | 26 +++++++++++++++++--------- internal/gps/pkgtree/pkgtree_test.go | 21 --------------------- 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 6e2e3a45ec..cd3c64b080 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -634,10 +634,11 @@ func (t PackageTree) Copy() PackageTree { return t2 } -// TrimHiddenPackages returns a new PackageTree with packages removed that -// are either ignored, or both hidden and unreachable. +// TrimHiddenPackages returns a new PackageTree where packages that are both +// hidden and unreachable have been removed. Ignored packages are optionally +// removed, as well. // -// The package list is partitioned into two sets: normal, and hidden, where +// The package list is partitioned into two sets: visible, and hidden, where // packages are considered hidden if they are within or beneath directories // with: // @@ -646,20 +647,27 @@ func (t PackageTree) Copy() PackageTree { // * the exact name "testdata" // // Packages in the hidden set are dropped from the returned PackageTree, unless -// they are transitively reachable from imports in the normal set. +// they are transitively reachable from imports in the visible set. // // The "main", "tests" and "ignored" parameters have the same behavior as with // PackageTree.ToReachMap(): the first two determine, respectively, whether // imports from main packages, and imports from tests, should be considered for -// reachability checks. "ignored" designates packages, or patterns of packages, -// whose imports should be excluded from reachability checks, if encountered. -func (t PackageTree) TrimHiddenPackages(main, tests bool, ignore map[string]bool) PackageTree { +// reachability checks. +// +// "ignored" designates import paths, or patterns of import paths, where the +// corresponding packages should be excluded from reachability checks, if +// encountered. Ignored packages are also removed from the final set. +// +// Note that it is not recommended to call this method if the goal is to obtain +// a set of tree-external imports; calling ToReachMap and FlattenFn will achieve +// the same effect. +func (t PackageTree) TrimHiddenPackages(main, tests, keepIgnored bool, ignore *IgnoredRuleset) PackageTree { rm, _ := t.ToReachMap(main, tests, false, ignore) t2 := t.Copy() preserve := make(map[string]bool) for pkg, ie := range rm { - if !pkgFilter(pkg) { + if pkgFilter(pkg) && (keepIgnored || !ignore.IsIgnored(pkg)) { preserve[pkg] = true for _, in := range ie.Internal { preserve[in] = true @@ -668,7 +676,7 @@ func (t PackageTree) TrimHiddenPackages(main, tests bool, ignore map[string]bool } for ip := range t.Packages { - if !preserve[pkg] { + if !preserve[ip] { delete(t2.Packages, ip) } } diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index bb97c11c6b..cabb877da8 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -936,27 +936,6 @@ func TestListPackages(t *testing.T) { }, }, }, - // New code allows this because it doesn't care if the code compiles (kinda) or not, - // so maybe this is actually not an error anymore? - // - // TODO re-enable this case after the full and proper ListPackages() - // refactor in #99 - /*"two pkgs": { - fileRoot: j("twopkgs"), - importRoot: "twopkgs", - out: PackageTree{ - ImportRoot: "twopkgs", - Packages: map[string]PackageOrErr{ - "twopkgs": { - Err: &build.MultiplePackageError{ - Dir: j("twopkgs"), - Packages: []string{"simple", "m1p"}, - Files: []string{"a.go", "b.go"}, - }, - }, - }, - }, - }, */ // imports a missing pkg "missing import": { fileRoot: j("missing"), From 8e423e67359fbaf0a97d639592212f9f92da65c1 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 02:03:35 -0400 Subject: [PATCH 03/11] dep: Use TrimHiddenPackages in a standard way This consolidates calls to parse the root project's code tree into a single place, so as to avoid confusion about the standard, expected way that dep's root project tree is to be created. It may not be fully suitable for all status cases, but standardization is crucial for init and ensure. --- cmd/dep/ensure.go | 14 +++++++++----- cmd/dep/init.go | 4 ++-- cmd/dep/status.go | 9 +++++---- project.go | 14 ++++++++++++++ 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/cmd/dep/ensure.go b/cmd/dep/ensure.go index ef35e7f069..4b4c623052 100644 --- a/cmd/dep/ensure.go +++ b/cmd/dep/ensure.go @@ -178,12 +178,12 @@ func (cmd *ensureCommand) Run(ctx *dep.Ctx, args []string) error { return cmd.runVendorOnly(ctx, args, p, sm, params) } - params.RootPackageTree, err = pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) + params.RootPackageTree, err = p.ParseRootPackageTree() if err != nil { - return errors.Wrap(err, "ensure ListPackage for project") + return err } - if fatal, err := checkErrors(params.RootPackageTree.Packages); err != nil { + if fatal, err := checkErrors(params.RootPackageTree.Packages, p.Manifest.IgnoredPackages()); err != nil { if fatal { return err } else if ctx.Verbose { @@ -761,13 +761,17 @@ func getProjectConstraint(arg string, sm gps.SourceManager) (gps.ProjectConstrai return gps.ProjectConstraint{Ident: pi, Constraint: c}, arg, nil } -func checkErrors(m map[string]pkgtree.PackageOrErr) (fatal bool, err error) { +func checkErrors(m map[string]pkgtree.PackageOrErr, ignore *pkgtree.IgnoredRuleset) (fatal bool, err error) { var ( noGoErrors int pkgtreeErrors = make(pkgtreeErrs, 0, len(m)) ) - for _, poe := range m { + for ip, poe := range m { + if ignore.IsIgnored(ip) { + continue + } + if poe.Err != nil { switch poe.Err.(type) { case *build.NoGoError: diff --git a/cmd/dep/init.go b/cmd/dep/init.go index 4cebf4ae34..a4f6637d55 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -227,9 +227,9 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { } func getDirectDependencies(sm gps.SourceManager, p *dep.Project) (pkgtree.PackageTree, map[string]bool, error) { - pkgT, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) + pkgT, err := p.ParseRootPackageTree() if err != nil { - return pkgtree.PackageTree{}, nil, errors.Wrap(err, "gps.ListPackages") + return pkgtree.PackageTree{}, nil, err } directDeps := map[string]bool{} diff --git a/cmd/dep/status.go b/cmd/dep/status.go index 93b1e73a10..ff5355f700 100644 --- a/cmd/dep/status.go +++ b/cmd/dep/status.go @@ -171,7 +171,8 @@ type dotOutput struct { func (out *dotOutput) BasicHeader() { out.g = new(graphviz).New() - ptree, _ := pkgtree.ListPackages(out.p.ResolvedAbsRoot, string(out.p.ImportRoot)) + ptree, _ := out.p.ParseRootPackageTree() + // TODO(sdboyer) should be true, true, false, out.p.Manifest.IgnoredPackages() prm, _ := ptree.ToReachMap(true, false, false, nil) out.g.createNode(string(out.p.ImportRoot), "", prm.FlattenFn(paths.IsStandardImportPath)) @@ -358,9 +359,9 @@ type MissingStatus struct { func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceManager) (hasMissingPkgs bool, errCount int, err error) { // While the network churns on ListVersions() requests, statically analyze // code from the current project. - ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) + ptree, err := p.ParseRootPackageTree() if err != nil { - return false, 0, errors.Wrapf(err, "analysis of local packages failed") + return false, 0, err } // Set up a solver in order to check the InputHash. @@ -437,7 +438,7 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana errListPkgCh <- err } - prm, _ := ptr.ToReachMap(true, false, false, nil) + prm, _ := ptr.ToReachMap(true, true, false, p.Manifest.IgnoredPackages()) bs.Children = prm.FlattenFn(paths.IsStandardImportPath) } diff --git a/project.go b/project.go index 9749a723bf..61fdc21768 100644 --- a/project.go +++ b/project.go @@ -11,6 +11,7 @@ import ( "github.com/golang/dep/internal/fs" "github.com/golang/dep/internal/gps" + "github.com/golang/dep/internal/gps/pkgtree" "github.com/pkg/errors" ) @@ -133,6 +134,19 @@ func (p *Project) MakeParams() gps.SolveParameters { return params } +// ParseRootPackageTree analyzes the root project's disk contents to create a +// PackageTree, trimming out packages that are not relevant for root projects +// along the way. +func (p *Project) ParseRootPackageTree() (pkgtree.PackageTree, error) { + ptree, err := pkgtree.ListPackages(p.ResolvedAbsRoot, string(p.ImportRoot)) + if err != nil { + return pkgtree.PackageTree{}, errors.Wrap(err, "analysis of current project's packages failed") + } + // We don't care about (unreachable) hidden packages for the root project, + // so drop all of those. + return ptree.TrimHiddenPackages(true, true, false, p.Manifest.IgnoredPackages()), nil +} + // BackupVendor looks for existing vendor directory and if it's not empty, // creates a backup of it to a new directory with the provided suffix. func BackupVendor(vpath, suffix string) (string, error) { From c1d177d9db57eebf3b564d37211d6d1baf0bef20 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 02:05:35 -0400 Subject: [PATCH 04/11] gps: Demote import comment conflicts to pkg-level These should never have been global failures within ListPackages(), as they are entirely package-specific information. --- internal/gps/pkgtree/pkgtree.go | 25 ++++++++++++------------- internal/gps/pkgtree/pkgtree_test.go | 18 ++++++++++++------ 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index cd3c64b080..71273ec28d 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -140,20 +140,11 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { } err = fillPackage(p) - var pkg Package - if err == nil { - pkg = Package{ - ImportPath: ip, - CommentPath: p.ImportComment, - Name: p.Name, - Imports: p.Imports, - TestImports: dedupeStrings(p.TestImports, p.XTestImports), - } - } else { + if err != nil { switch err.(type) { - case gscan.ErrorList, *gscan.Error, *build.NoGoError: - // This happens if we encounter malformed or nonexistent Go - // source code + case gscan.ErrorList, *gscan.Error, *build.NoGoError, *ConflictingImportComments: + // Assorted cases in which we've encounter malformed or + // nonexistent Go source code. ptree.Packages[ip] = PackageOrErr{ Err: err, } @@ -163,6 +154,14 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { } } + pkg := Package{ + ImportPath: ip, + CommentPath: p.ImportComment, + Name: p.Name, + Imports: p.Imports, + TestImports: dedupeStrings(p.TestImports, p.XTestImports), + } + if pkg.CommentPath != "" && !strings.HasPrefix(pkg.CommentPath, importRoot) { ptree.Packages[ip] = PackageOrErr{ Err: &NonCanonicalImportRoot{ diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index cabb877da8..c82d9f2af9 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -1296,12 +1296,18 @@ func TestListPackages(t *testing.T) { "conflicting canonical comments": { fileRoot: j("canon_confl"), importRoot: "canon_confl", - out: PackageTree{}, - err: &ConflictingImportComments{ - ImportPath: "canon_confl", - ConflictingImportComments: []string{ - "vanity1", - "vanity2", + out: PackageTree{ + ImportRoot: "canon_confl", + Packages: map[string]PackageOrErr{ + "canon_confl": { + Err: &ConflictingImportComments{ + ImportPath: "canon_confl", + ConflictingImportComments: []string{ + "vanity1", + "vanity2", + }, + }, + }, }, }, }, From b9626438ec7a92aeb53c2e1e1a0b178c91281ef2 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 13:13:40 -0400 Subject: [PATCH 05/11] dep: Protect against some nil cases --- cmd/dep/ensure_test.go | 2 +- project.go | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cmd/dep/ensure_test.go b/cmd/dep/ensure_test.go index c4fb0c7a24..e8cf300e87 100644 --- a/cmd/dep/ensure_test.go +++ b/cmd/dep/ensure_test.go @@ -135,7 +135,7 @@ func TestCheckErrors(t *testing.T) { for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { - fatal, err := checkErrors(tc.pkgOrErrMap) + fatal, err := checkErrors(tc.pkgOrErrMap, nil) if tc.fatal != fatal { t.Fatalf("expected fatal flag to be %T, got %T", tc.fatal, fatal) } diff --git a/project.go b/project.go index 61fdc21768..f6639eb6f1 100644 --- a/project.go +++ b/project.go @@ -144,7 +144,11 @@ func (p *Project) ParseRootPackageTree() (pkgtree.PackageTree, error) { } // We don't care about (unreachable) hidden packages for the root project, // so drop all of those. - return ptree.TrimHiddenPackages(true, true, false, p.Manifest.IgnoredPackages()), nil + var ig *pkgtree.IgnoredRuleset + if p.Manifest != nil { + ig = p.Manifest.IgnoredPackages() + } + return ptree.TrimHiddenPackages(true, true, false, ig), nil } // BackupVendor looks for existing vendor directory and if it's not empty, From 09ee5b74537431cb55882ca6ced938db678d36bc Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 14:13:36 -0400 Subject: [PATCH 06/11] gps: Move IgnoredRuleset into its own file --- internal/gps/pkgtree/ignored_ruleset.go | 111 ++++++++++ internal/gps/pkgtree/ignored_ruleset_test.go | 204 +++++++++++++++++++ internal/gps/pkgtree/pkgtree.go | 102 ---------- internal/gps/pkgtree/pkgtree_test.go | 197 ------------------ 4 files changed, 315 insertions(+), 299 deletions(-) create mode 100644 internal/gps/pkgtree/ignored_ruleset.go create mode 100644 internal/gps/pkgtree/ignored_ruleset_test.go diff --git a/internal/gps/pkgtree/ignored_ruleset.go b/internal/gps/pkgtree/ignored_ruleset.go new file mode 100644 index 0000000000..30b92bd8e9 --- /dev/null +++ b/internal/gps/pkgtree/ignored_ruleset.go @@ -0,0 +1,111 @@ +// 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 pkgtree + +import ( + "sort" + "strings" + + "github.com/armon/go-radix" +) + +// IgnoredRuleset comprises a set of rules for ignoring import paths. It can +// manage both literal and prefix-wildcard matches. +type IgnoredRuleset struct { + t *radix.Tree +} + +// NewIgnoredRuleset processes a set of strings into an IgnoredRuleset. Strings +// that end in "*" are treated as wildcards, where any import path with a +// matching prefix will be ignored. IgnoredRulesets are immutable once created. +// +// Duplicate and redundant (i.e. a literal path that has a prefix of a wildcard +// path) declarations are discarded. Consequently, it is possible that the +// returned IgnoredRuleset may have a smaller Len() than the input slice. +func NewIgnoredRuleset(ig []string) *IgnoredRuleset { + if len(ig) == 0 { + return &IgnoredRuleset{} + } + + ir := &IgnoredRuleset{ + t: radix.New(), + } + + // Sort the list of all the ignores in order to ensure that wildcard + // precedence is recorded correctly in the trie. + sort.Strings(ig) + for _, i := range ig { + // Skip global ignore and empty string. + if i == "*" || i == "" { + continue + } + + _, wildi, has := ir.t.LongestPrefix(i) + // We may not always have a value here, but if we do, then it's a bool. + wild, _ := wildi.(bool) + // Check if it's a wildcard ignore. + if strings.HasSuffix(i, "*") { + // Check if it is ineffectual. + if has && wild { + // Skip ineffectual wildcard ignore. + continue + } + // Create the ignore prefix and insert in the radix tree. + ir.t.Insert(i[:len(i)-1], true) + } else if !has || !wild { + ir.t.Insert(i, false) + } + } + + if ir.t.Len() == 0 { + ir.t = nil + } + + return ir +} + +// IsIgnored indicates whether the provided path should be ignored, according to +// the ruleset. +func (ir *IgnoredRuleset) IsIgnored(path string) bool { + if path == "" || ir == nil || ir.t == nil { + return false + } + + prefix, wildi, has := ir.t.LongestPrefix(path) + return has && (wildi.(bool) || path == prefix) +} + +// Len indicates the number of rules in the ruleset. +func (ir *IgnoredRuleset) Len() int { + if ir == nil || ir.t == nil { + return 0 + } + + return ir.t.Len() +} + +// ToSlice converts the contents of the IgnoredRuleset to a string slice. +// +// This operation is symmetrically dual to NewIgnoredRuleset. +func (ir *IgnoredRuleset) ToSlice() []string { + irlen := ir.Len() + if irlen == 0 { + return nil + } + + items := make([]string, 0, irlen) + ir.t.Walk(func(s string, v interface{}) bool { + if s != "" { + if v.(bool) { + items = append(items, s+"*") + } else { + items = append(items, s) + } + } + return false + }) + + return items +} diff --git a/internal/gps/pkgtree/ignored_ruleset_test.go b/internal/gps/pkgtree/ignored_ruleset_test.go new file mode 100644 index 0000000000..0dab4309c4 --- /dev/null +++ b/internal/gps/pkgtree/ignored_ruleset_test.go @@ -0,0 +1,204 @@ +// 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 pkgtree + +import "testing" + +func TestIgnoredRuleset(t *testing.T) { + type tfixm []struct { + path string + wild bool + } + cases := []struct { + name string + inputs []string + wantInTree tfixm + wantEmptyTree bool + shouldIgnore []string + shouldNotIgnore []string + }{ + { + name: "only skip global ignore", + inputs: []string{"*"}, + wantEmptyTree: true, + }, + { + name: "ignores without ignore suffix", + inputs: []string{ + "x/y/z", + "*a/b/c", + "gophers", + }, + wantInTree: tfixm{ + {path: "x/y/z", wild: false}, + {path: "*a/b/c", wild: false}, + {path: "gophers", wild: false}, + }, + shouldIgnore: []string{ + "x/y/z", + "gophers", + }, + shouldNotIgnore: []string{ + "x/y/z/q", + "x/y", + "gopher", + "gopherss", + }, + }, + { + name: "ignores with ignore suffix", + inputs: []string{ + "x/y/z*", + "a/b/c", + "gophers", + }, + wantInTree: tfixm{ + {path: "x/y/z", wild: true}, + {path: "a/b/c", wild: false}, + {path: "gophers", wild: false}, + }, + shouldIgnore: []string{ + "x/y/z", + "x/y/zz", + "x/y/z/", + "x/y/z/q", + }, + shouldNotIgnore: []string{ + "x/y", + "gopher", + }, + }, + { + name: "global ignore with other strings", + inputs: []string{ + "*", + "gophers*", + "x/y/z*", + "a/b/c", + }, + wantInTree: tfixm{ + {path: "x/y/z", wild: true}, + {path: "a/b/c", wild: false}, + {path: "gophers", wild: true}, + }, + shouldIgnore: []string{ + "x/y/z", + "x/y/z/", + "x/y/z/q", + "gophers", + "gopherss", + "gophers/foo", + }, + shouldNotIgnore: []string{ + "x/y", + "gopher", + }, + }, + { + name: "ineffectual ignore with wildcard", + inputs: []string{ + "a/b*", + "a/b/c*", + "a/b/x/y", + "a/c*", + }, + wantInTree: tfixm{ + {path: "a/c", wild: true}, + {path: "a/b", wild: true}, + }, + shouldIgnore: []string{ + "a/cb", + }, + shouldNotIgnore: []string{ + "a/", + "a/d", + }, + }, + { + name: "same path with and without wildcard", + inputs: []string{ + "a/b*", + "a/b", + }, + wantInTree: tfixm{ + {path: "a/b", wild: true}, + }, + shouldIgnore: []string{ + "a/b", + "a/bb", + }, + shouldNotIgnore: []string{ + "a/", + "a/d", + }, + }, + { + name: "empty paths", + inputs: []string{ + "", + "a/b*", + }, + wantInTree: tfixm{ + {path: "a/b", wild: true}, + }, + shouldNotIgnore: []string{ + "", + }, + }, + { + name: "single wildcard", + inputs: []string{ + "a/b*", + }, + wantInTree: tfixm{ + {path: "a/b", wild: true}, + }, + shouldIgnore: []string{ + "a/b/c", + }, + }, + } + + for _, c := range cases { + igm := NewIgnoredRuleset(c.inputs) + f := func(t *testing.T) { + + if c.wantEmptyTree { + if igm.Len() != 0 { + t.Fatalf("wanted empty tree, but had %v elements", igm.Len()) + } + } + + // Check if the wildcard suffix ignores are in the tree. + for _, p := range c.wantInTree { + wildi, has := igm.t.Get(p.path) + if !has { + t.Fatalf("expected %q to be in the tree", p.path) + } else if wildi.(bool) != p.wild { + if p.wild { + t.Fatalf("expected %q to be a wildcard matcher, but it was not", p.path) + } else { + t.Fatalf("expected %q not to be a wildcard matcher, but it was", p.path) + } + } + } + + for _, p := range c.shouldIgnore { + if !igm.IsIgnored(p) { + t.Fatalf("%q should be ignored, but it was not", p) + } + } + for _, p := range c.shouldNotIgnore { + if igm.IsIgnored(p) { + t.Fatalf("%q should not be ignored, but it was", p) + } + } + } + t.Run(c.name, f) + + igm = NewIgnoredRuleset(igm.ToSlice()) + t.Run(c.name+"/inandout", f) + } +} diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 71273ec28d..ec31d6afb5 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -18,8 +18,6 @@ import ( "strconv" "strings" "unicode" - - "github.com/armon/go-radix" ) // Package represents a Go package. It contains a subset of the information @@ -663,7 +661,6 @@ func (t PackageTree) Copy() PackageTree { func (t PackageTree) TrimHiddenPackages(main, tests, keepIgnored bool, ignore *IgnoredRuleset) PackageTree { rm, _ := t.ToReachMap(main, tests, false, ignore) t2 := t.Copy() - preserve := make(map[string]bool) for pkg, ie := range rm { if pkgFilter(pkg) && (keepIgnored || !ignore.IsIgnored(pkg)) { @@ -1073,102 +1070,3 @@ func uniq(a []string) []string { } return a[:i] } - -// IgnoredRuleset comprises a set of rules for ignoring import paths. It can -// manage both literal and prefix-wildcard matches. -type IgnoredRuleset struct { - t *radix.Tree -} - -// NewIgnoredRuleset processes a set of strings into an IgnoredRuleset. Strings -// that end in "*" are treated as wildcards, where any import path with a -// matching prefix will be ignored. IgnoredRulesets are immutable once created. -// -// Duplicate and redundant (i.e. a literal path that has a prefix of a wildcard -// path) declarations are discarded. Consequently, it is possible that the -// returned IgnoredRuleset may have a smaller Len() than the input slice. -func NewIgnoredRuleset(ig []string) *IgnoredRuleset { - if len(ig) == 0 { - return &IgnoredRuleset{} - } - - ir := &IgnoredRuleset{ - t: radix.New(), - } - - // Sort the list of all the ignores in order to ensure that wildcard - // precedence is recorded correctly in the trie. - sort.Strings(ig) - for _, i := range ig { - // Skip global ignore and empty string. - if i == "*" || i == "" { - continue - } - - _, wildi, has := ir.t.LongestPrefix(i) - // We may not always have a value here, but if we do, then it's a bool. - wild, _ := wildi.(bool) - // Check if it's a wildcard ignore. - if strings.HasSuffix(i, "*") { - // Check if it is ineffectual. - if has && wild { - // Skip ineffectual wildcard ignore. - continue - } - // Create the ignore prefix and insert in the radix tree. - ir.t.Insert(i[:len(i)-1], true) - } else if !has || !wild { - ir.t.Insert(i, false) - } - } - - if ir.t.Len() == 0 { - ir.t = nil - } - - return ir -} - -// IsIgnored indicates whether the provided path should be ignored, according to -// the ruleset. -func (ir *IgnoredRuleset) IsIgnored(path string) bool { - if path == "" || ir == nil || ir.t == nil { - return false - } - - prefix, wildi, has := ir.t.LongestPrefix(path) - return has && (wildi.(bool) || path == prefix) -} - -// Len indicates the number of rules in the ruleset. -func (ir *IgnoredRuleset) Len() int { - if ir == nil || ir.t == nil { - return 0 - } - - return ir.t.Len() -} - -// ToSlice converts the contents of the IgnoredRuleset to a string slice. -// -// This operation is symmetrically dual to NewIgnoredRuleset. -func (ir *IgnoredRuleset) ToSlice() []string { - irlen := ir.Len() - if irlen == 0 { - return nil - } - - items := make([]string, 0, irlen) - ir.t.Walk(func(s string, v interface{}) bool { - if s != "" { - if v.(bool) { - items = append(items, s+"*") - } else { - items = append(items, s) - } - } - return false - }) - - return items -} diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index c82d9f2af9..30860329fb 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -2019,200 +2019,3 @@ func getTestdataRootDir(t *testing.T) string { } return filepath.Join(cwd, "..", "_testdata") } - -func TestIgnoredRuleset(t *testing.T) { - type tfixm []struct { - path string - wild bool - } - cases := []struct { - name string - inputs []string - wantInTree tfixm - wantEmptyTree bool - shouldIgnore []string - shouldNotIgnore []string - }{ - { - name: "only skip global ignore", - inputs: []string{"*"}, - wantEmptyTree: true, - }, - { - name: "ignores without ignore suffix", - inputs: []string{ - "x/y/z", - "*a/b/c", - "gophers", - }, - wantInTree: tfixm{ - {path: "x/y/z", wild: false}, - {path: "*a/b/c", wild: false}, - {path: "gophers", wild: false}, - }, - shouldIgnore: []string{ - "x/y/z", - "gophers", - }, - shouldNotIgnore: []string{ - "x/y/z/q", - "x/y", - "gopher", - "gopherss", - }, - }, - { - name: "ignores with ignore suffix", - inputs: []string{ - "x/y/z*", - "a/b/c", - "gophers", - }, - wantInTree: tfixm{ - {path: "x/y/z", wild: true}, - {path: "a/b/c", wild: false}, - {path: "gophers", wild: false}, - }, - shouldIgnore: []string{ - "x/y/z", - "x/y/zz", - "x/y/z/", - "x/y/z/q", - }, - shouldNotIgnore: []string{ - "x/y", - "gopher", - }, - }, - { - name: "global ignore with other strings", - inputs: []string{ - "*", - "gophers*", - "x/y/z*", - "a/b/c", - }, - wantInTree: tfixm{ - {path: "x/y/z", wild: true}, - {path: "a/b/c", wild: false}, - {path: "gophers", wild: true}, - }, - shouldIgnore: []string{ - "x/y/z", - "x/y/z/", - "x/y/z/q", - "gophers", - "gopherss", - "gophers/foo", - }, - shouldNotIgnore: []string{ - "x/y", - "gopher", - }, - }, - { - name: "ineffectual ignore with wildcard", - inputs: []string{ - "a/b*", - "a/b/c*", - "a/b/x/y", - "a/c*", - }, - wantInTree: tfixm{ - {path: "a/c", wild: true}, - {path: "a/b", wild: true}, - }, - shouldIgnore: []string{ - "a/cb", - }, - shouldNotIgnore: []string{ - "a/", - "a/d", - }, - }, - { - name: "same path with and without wildcard", - inputs: []string{ - "a/b*", - "a/b", - }, - wantInTree: tfixm{ - {path: "a/b", wild: true}, - }, - shouldIgnore: []string{ - "a/b", - "a/bb", - }, - shouldNotIgnore: []string{ - "a/", - "a/d", - }, - }, - { - name: "empty paths", - inputs: []string{ - "", - "a/b*", - }, - wantInTree: tfixm{ - {path: "a/b", wild: true}, - }, - shouldNotIgnore: []string{ - "", - }, - }, - { - name: "single wildcard", - inputs: []string{ - "a/b*", - }, - wantInTree: tfixm{ - {path: "a/b", wild: true}, - }, - shouldIgnore: []string{ - "a/b/c", - }, - }, - } - - for _, c := range cases { - igm := NewIgnoredRuleset(c.inputs) - f := func(t *testing.T) { - - if c.wantEmptyTree { - if igm.Len() != 0 { - t.Fatalf("wanted empty tree, but had %v elements", igm.Len()) - } - } - - // Check if the wildcard suffix ignores are in the tree. - for _, p := range c.wantInTree { - wildi, has := igm.t.Get(p.path) - if !has { - t.Fatalf("expected %q to be in the tree", p.path) - } else if wildi.(bool) != p.wild { - if p.wild { - t.Fatalf("expected %q to be a wildcard matcher, but it was not", p.path) - } else { - t.Fatalf("expected %q not to be a wildcard matcher, but it was", p.path) - } - } - } - - for _, p := range c.shouldIgnore { - if !igm.IsIgnored(p) { - t.Fatalf("%q should be ignored, but it was not", p) - } - } - for _, p := range c.shouldNotIgnore { - if igm.IsIgnored(p) { - t.Fatalf("%q should not be ignored, but it was", p) - } - } - } - t.Run(c.name, f) - - igm = NewIgnoredRuleset(igm.ToSlice()) - t.Run(c.name+"/inandout", f) - } -} From 0951d5d7a351f937ad920681dcdbda92ba161750 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 14:36:05 -0400 Subject: [PATCH 07/11] gps: Don't drop visible packages with errors --- internal/gps/pkgtree/pkgtree.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index ec31d6afb5..3723981840 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -659,9 +659,10 @@ func (t PackageTree) Copy() PackageTree { // a set of tree-external imports; calling ToReachMap and FlattenFn will achieve // the same effect. func (t PackageTree) TrimHiddenPackages(main, tests, keepIgnored bool, ignore *IgnoredRuleset) PackageTree { - rm, _ := t.ToReachMap(main, tests, false, ignore) + rm, pie := t.ToReachMap(main, tests, false, ignore) t2 := t.Copy() preserve := make(map[string]bool) + for pkg, ie := range rm { if pkgFilter(pkg) && (keepIgnored || !ignore.IsIgnored(pkg)) { preserve[pkg] = true @@ -671,6 +672,14 @@ func (t PackageTree) TrimHiddenPackages(main, tests, keepIgnored bool, ignore *I } } + // Also process the problem map, as packages in the visible set with errors + // need to be included in the return values. + for pkg := range pie { + if pkgFilter(pkg) && (keepIgnored || !ignore.IsIgnored(pkg)) { + preserve[pkg] = true + } + } + for ip := range t.Packages { if !preserve[ip] { delete(t2.Packages, ip) From b672a9975a2e2983158ae3c1a2f3720b282f355b Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 22:53:45 -0400 Subject: [PATCH 08/11] gps: Make PackageTree.Copy more careful, efficient We only perform a single allocation for all []string, and are careful to copy all possible error values via reflection. --- internal/gps/pkgtree/pkgtree.go | 37 +++++++++++++++++++--------- internal/gps/pkgtree/pkgtree_test.go | 37 +++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 13 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 3723981840..2ad9215b1d 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -14,6 +14,7 @@ import ( "go/token" "os" "path/filepath" + "reflect" "sort" "strconv" "strings" @@ -608,21 +609,33 @@ func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore *IgnoredRules func (t PackageTree) Copy() PackageTree { t2 := PackageTree{ ImportRoot: t.ImportRoot, - Packages: map[string]PackageOrErr{}, + Packages: make(map[string]PackageOrErr, len(t.Packages)), } + // Walk through and count up the total number of string slice elements we'll + // need, then allocate them all at once. + strcount := 0 + for _, poe := range t.Packages { + strcount = strcount + len(poe.P.Imports) + len(poe.P.TestImports) + } + pool := make([]string, strcount) + for path, poe := range t.Packages { - poe2 := PackageOrErr{ - Err: poe.Err, - P: poe.P, - } - if len(poe.P.Imports) > 0 { - poe2.P.Imports = make([]string, len(poe.P.Imports)) - copy(poe2.P.Imports, poe.P.Imports) - } - if len(poe.P.TestImports) > 0 { - poe2.P.TestImports = make([]string, len(poe.P.TestImports)) - copy(poe2.P.TestImports, poe.P.TestImports) + var poe2 PackageOrErr + + if poe.Err != nil { + poe2.Err = reflect.New(reflect.ValueOf(poe.Err).Elem().Type()).Interface().(error) + } else { + poe2.P = poe.P + il, til := len(poe.P.Imports), len(poe.P.TestImports) + if il > 0 { + poe2.P.Imports, pool = pool[:il], pool[il:] + copy(poe2.P.Imports, poe.P.Imports) + } + if til > 0 { + poe2.P.TestImports, pool = pool[:til], pool[til:] + copy(poe2.P.TestImports, poe.P.TestImports) + } } t2.Packages[path] = poe2 diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index 30860329fb..d70cab7524 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -1409,7 +1409,7 @@ func TestListPackagesNoPerms(t *testing.T) { // It's not a big deal, though, because the os.IsPermission() call we // use in the real code is effectively what's being tested here, and // that's designed to be cross-platform. So, if the unix tests pass, we - // have every reason to believe windows tests would to, if the situation + // have every reason to believe windows tests would too, if the situation // arises. t.Skip() } @@ -2019,3 +2019,38 @@ func getTestdataRootDir(t *testing.T) string { } return filepath.Join(cwd, "..", "_testdata") } + +// Canary regression test to make sure that if PackageTree ever gains new +// fields, we update the Copy method accordingly. +func TestCanaryPackageTreeCopy(t *testing.T) { + ptreeFields := []string{ + "ImportRoot", + "Packages", + } + packageFields := []string{ + "Name", + "ImportPath", + "CommentPath", + "Imports", + "TestImports", + } + + fieldNames := func(typ reflect.Type) []string { + var names []string + for i := 0; i < typ.NumField(); i++ { + names = append(names, typ.Field(i).Name) + } + return names + } + + ptreeRefl := fieldNames(reflect.TypeOf(PackageTree{})) + packageRefl := fieldNames(reflect.TypeOf(Package{})) + + if !reflect.DeepEqual(ptreeFields, ptreeRefl) { + t.Errorf("PackageTree.Copy is designed to work with an exact set of fields in the PackageTree struct - make sure it (and this test) have been updated!\n\t(GOT):%s\n\t(WNT):%s", ptreeFields, ptreeRefl) + } + + if !reflect.DeepEqual(packageFields, packageRefl) { + t.Errorf("PackageTree.Copy is designed to work with an exact set of fields in the Package struct - make sure it (and this test) have been updated!\n\t(GOT):%s\n\t(WNT):%s", packageFields, packageRefl) + } +} From adedbf9c46586f3929ef27b6ea830a0023c18fa0 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 23:24:27 -0400 Subject: [PATCH 09/11] gps: Remove keepIgnored from TrimHiddenPackages While this would be possible to implement, it would require writing entirely new entry logic into wmToReach(). That seems like a sufficiently strong indication of breaking domain assumptions that it's preferable just to drop the parameter, which was questionable in the first place. --- internal/gps/pkgtree/pkgtree.go | 14 +++++++------- project.go | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 2ad9215b1d..39a4fb6498 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -644,9 +644,8 @@ func (t PackageTree) Copy() PackageTree { return t2 } -// TrimHiddenPackages returns a new PackageTree where packages that are both -// hidden and unreachable have been removed. Ignored packages are optionally -// removed, as well. +// TrimHiddenPackages returns a new PackageTree where packages that are ignored, +// or both hidden and unreachable, have been removed. // // The package list is partitioned into two sets: visible, and hidden, where // packages are considered hidden if they are within or beneath directories @@ -662,7 +661,8 @@ func (t PackageTree) Copy() PackageTree { // The "main", "tests" and "ignored" parameters have the same behavior as with // PackageTree.ToReachMap(): the first two determine, respectively, whether // imports from main packages, and imports from tests, should be considered for -// reachability checks. +// reachability checks. Setting 'main' to true will additionally result in main +// packages being trimmed. // // "ignored" designates import paths, or patterns of import paths, where the // corresponding packages should be excluded from reachability checks, if @@ -671,13 +671,13 @@ func (t PackageTree) Copy() PackageTree { // Note that it is not recommended to call this method if the goal is to obtain // a set of tree-external imports; calling ToReachMap and FlattenFn will achieve // the same effect. -func (t PackageTree) TrimHiddenPackages(main, tests, keepIgnored bool, ignore *IgnoredRuleset) PackageTree { +func (t PackageTree) TrimHiddenPackages(main, tests bool, ignore *IgnoredRuleset) PackageTree { rm, pie := t.ToReachMap(main, tests, false, ignore) t2 := t.Copy() preserve := make(map[string]bool) for pkg, ie := range rm { - if pkgFilter(pkg) && (keepIgnored || !ignore.IsIgnored(pkg)) { + if pkgFilter(pkg) && !ignore.IsIgnored(pkg) { preserve[pkg] = true for _, in := range ie.Internal { preserve[in] = true @@ -688,7 +688,7 @@ func (t PackageTree) TrimHiddenPackages(main, tests, keepIgnored bool, ignore *I // Also process the problem map, as packages in the visible set with errors // need to be included in the return values. for pkg := range pie { - if pkgFilter(pkg) && (keepIgnored || !ignore.IsIgnored(pkg)) { + if pkgFilter(pkg) && !ignore.IsIgnored(pkg) { preserve[pkg] = true } } diff --git a/project.go b/project.go index f6639eb6f1..4dd8ecda77 100644 --- a/project.go +++ b/project.go @@ -148,7 +148,7 @@ func (p *Project) ParseRootPackageTree() (pkgtree.PackageTree, error) { if p.Manifest != nil { ig = p.Manifest.IgnoredPackages() } - return ptree.TrimHiddenPackages(true, true, false, ig), nil + return ptree.TrimHiddenPackages(true, true, ig), nil } // BackupVendor looks for existing vendor directory and if it's not empty, From e4909ed240b7da9696f91c74c3d1c9b7e3325087 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sat, 14 Oct 2017 23:26:12 -0400 Subject: [PATCH 10/11] gps: Add PackageTree.TrimHiddenPackages tests --- .../src/varied_hidden/.onlyfromtests/a.go | 18 ++ .../src/varied_hidden/.onlyfromtests/b.go | 15 ++ .../src/varied_hidden/_frommain/a.go | 16 ++ .../_testdata/src/varied_hidden/_never/a.go | 16 ++ .../varied_hidden/_secondorder/secondorder.go | 11 + .../src/varied_hidden/always/always_test.go | 7 + .../src/varied_hidden/dotdotslash/a.go | 13 ++ .../gps/_testdata/src/varied_hidden/locals.go | 14 ++ .../gps/_testdata/src/varied_hidden/main.go | 13 ++ .../src/varied_hidden/simple/locals.go | 11 + .../src/varied_hidden/simple/simple.go | 16 ++ .../varied_hidden/simple/testdata/another.go | 7 + internal/gps/pkgtree/pkgtree_test.go | 216 ++++++++++++++++++ 13 files changed, 373 insertions(+) create mode 100644 internal/gps/_testdata/src/varied_hidden/.onlyfromtests/a.go create mode 100644 internal/gps/_testdata/src/varied_hidden/.onlyfromtests/b.go create mode 100644 internal/gps/_testdata/src/varied_hidden/_frommain/a.go create mode 100644 internal/gps/_testdata/src/varied_hidden/_never/a.go create mode 100644 internal/gps/_testdata/src/varied_hidden/_secondorder/secondorder.go create mode 100644 internal/gps/_testdata/src/varied_hidden/always/always_test.go create mode 100644 internal/gps/_testdata/src/varied_hidden/dotdotslash/a.go create mode 100644 internal/gps/_testdata/src/varied_hidden/locals.go create mode 100644 internal/gps/_testdata/src/varied_hidden/main.go create mode 100644 internal/gps/_testdata/src/varied_hidden/simple/locals.go create mode 100644 internal/gps/_testdata/src/varied_hidden/simple/simple.go create mode 100644 internal/gps/_testdata/src/varied_hidden/simple/testdata/another.go diff --git a/internal/gps/_testdata/src/varied_hidden/.onlyfromtests/a.go b/internal/gps/_testdata/src/varied_hidden/.onlyfromtests/a.go new file mode 100644 index 0000000000..66e136f943 --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/.onlyfromtests/a.go @@ -0,0 +1,18 @@ +// 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 onlyfromtests + +import ( + "sort" + + _ "varied/_secondorder" + + "github.com/golang/dep/internal/gps" +) + +var ( + M = sort.Strings + _ = gps.Solve +) diff --git a/internal/gps/_testdata/src/varied_hidden/.onlyfromtests/b.go b/internal/gps/_testdata/src/varied_hidden/.onlyfromtests/b.go new file mode 100644 index 0000000000..fa353864bf --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/.onlyfromtests/b.go @@ -0,0 +1,15 @@ +// 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 onlyfromtests + +import ( + "os" + "sort" +) + +var ( + _ = sort.Strings + _ = os.PathSeparator +) diff --git a/internal/gps/_testdata/src/varied_hidden/_frommain/a.go b/internal/gps/_testdata/src/varied_hidden/_frommain/a.go new file mode 100644 index 0000000000..28ee51be7a --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/_frommain/a.go @@ -0,0 +1,16 @@ +// 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 frommain + +import ( + "sort" + + "github.com/golang/dep/internal/gps" +) + +var ( + M = sort.Strings + _ = gps.Solve +) diff --git a/internal/gps/_testdata/src/varied_hidden/_never/a.go b/internal/gps/_testdata/src/varied_hidden/_never/a.go new file mode 100644 index 0000000000..72c7c99245 --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/_never/a.go @@ -0,0 +1,16 @@ +// 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 never + +import ( + "sort" + + "github.com/golang/dep/internal/gps" +) + +var ( + M = sort.Strings + _ = gps.Solve +) diff --git a/internal/gps/_testdata/src/varied_hidden/_secondorder/secondorder.go b/internal/gps/_testdata/src/varied_hidden/_secondorder/secondorder.go new file mode 100644 index 0000000000..4224b81e1e --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/_secondorder/secondorder.go @@ -0,0 +1,11 @@ +// 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 secondorder + +import "hash" + +var ( + H = hash.Hash +) diff --git a/internal/gps/_testdata/src/varied_hidden/always/always_test.go b/internal/gps/_testdata/src/varied_hidden/always/always_test.go new file mode 100644 index 0000000000..043805b4bf --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/always/always_test.go @@ -0,0 +1,7 @@ +// 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 always + +import _ "varied/.onlyfromtests" diff --git a/internal/gps/_testdata/src/varied_hidden/dotdotslash/a.go b/internal/gps/_testdata/src/varied_hidden/dotdotslash/a.go new file mode 100644 index 0000000000..f1ffb55bdd --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/dotdotslash/a.go @@ -0,0 +1,13 @@ +// 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 dotslash + +import ( + "../github.com/golang/dep/internal/gps" +) + +var ( + A = gps.Solver +) diff --git a/internal/gps/_testdata/src/varied_hidden/locals.go b/internal/gps/_testdata/src/varied_hidden/locals.go new file mode 100644 index 0000000000..4afaa8bfd3 --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/locals.go @@ -0,0 +1,14 @@ +// 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 main + +import ( + _ "varied/_frommain" + "varied/simple" +) + +var ( + _ = simple.S +) diff --git a/internal/gps/_testdata/src/varied_hidden/main.go b/internal/gps/_testdata/src/varied_hidden/main.go new file mode 100644 index 0000000000..0812e3ca60 --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/main.go @@ -0,0 +1,13 @@ +// 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 main + +import ( + "net/http" +) + +var ( + _ = http.Client +) diff --git a/internal/gps/_testdata/src/varied_hidden/simple/locals.go b/internal/gps/_testdata/src/varied_hidden/simple/locals.go new file mode 100644 index 0000000000..7d682c4e75 --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/simple/locals.go @@ -0,0 +1,11 @@ +// 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 simple + +import "varied/simple/testdata" + +var ( + _ = testdata.H +) diff --git a/internal/gps/_testdata/src/varied_hidden/simple/simple.go b/internal/gps/_testdata/src/varied_hidden/simple/simple.go new file mode 100644 index 0000000000..9d0c3445f6 --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/simple/simple.go @@ -0,0 +1,16 @@ +// 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 simple + +import ( + "go/parser" + + "github.com/golang/dep/internal/gps" +) + +var ( + _ = parser.ParseFile + S = gps.Prepare +) diff --git a/internal/gps/_testdata/src/varied_hidden/simple/testdata/another.go b/internal/gps/_testdata/src/varied_hidden/simple/testdata/another.go new file mode 100644 index 0000000000..02eab2a200 --- /dev/null +++ b/internal/gps/_testdata/src/varied_hidden/simple/testdata/another.go @@ -0,0 +1,7 @@ +// 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 testdata + +import _ "varied/dotdotslash" diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index d70cab7524..8b61a61d67 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -11,6 +11,7 @@ import ( "go/token" "io/ioutil" "os" + "path" "path/filepath" "reflect" "runtime" @@ -1208,6 +1209,114 @@ func TestListPackages(t *testing.T) { }, }, }, + "varied with hidden dirs": { + fileRoot: j("varied_hidden"), + importRoot: "varied", + out: PackageTree{ + ImportRoot: "varied", + Packages: map[string]PackageOrErr{ + "varied": { + P: Package{ + ImportPath: "varied", + CommentPath: "", + Name: "main", + Imports: []string{ + "net/http", + "varied/_frommain", + "varied/simple", + }, + }, + }, + "varied/always": { + P: Package{ + ImportPath: "varied/always", + CommentPath: "", + Name: "always", + Imports: []string{}, + TestImports: []string{ + "varied/.onlyfromtests", + }, + }, + }, + "varied/.onlyfromtests": { + P: Package{ + ImportPath: "varied/.onlyfromtests", + CommentPath: "", + Name: "onlyfromtests", + Imports: []string{ + "github.com/golang/dep/internal/gps", + "os", + "sort", + "varied/_secondorder", + }, + }, + }, + "varied/simple": { + P: Package{ + ImportPath: "varied/simple", + CommentPath: "", + Name: "simple", + Imports: []string{ + "github.com/golang/dep/internal/gps", + "go/parser", + "varied/simple/testdata", + }, + }, + }, + "varied/simple/testdata": { + P: Package{ + ImportPath: "varied/simple/testdata", + CommentPath: "", + Name: "testdata", + Imports: []string{ + "varied/dotdotslash", + }, + }, + }, + "varied/_secondorder": { + P: Package{ + ImportPath: "varied/_secondorder", + CommentPath: "", + Name: "secondorder", + Imports: []string{ + "hash", + }, + }, + }, + "varied/_never": { + P: Package{ + ImportPath: "varied/_never", + CommentPath: "", + Name: "never", + Imports: []string{ + "github.com/golang/dep/internal/gps", + "sort", + }, + }, + }, + "varied/_frommain": { + P: Package{ + ImportPath: "varied/_frommain", + CommentPath: "", + Name: "frommain", + Imports: []string{ + "github.com/golang/dep/internal/gps", + "sort", + }, + }, + }, + "varied/dotdotslash": { + Err: &LocalImportsError{ + Dir: j("varied_hidden/dotdotslash"), + ImportPath: "varied/dotdotslash", + LocalImports: []string{ + "../github.com/golang/dep/internal/gps", + }, + }, + }, + }, + }, + }, "invalid buildtag like comments should be ignored": { fileRoot: j("buildtag"), importRoot: "buildtag", @@ -1399,6 +1508,113 @@ func TestListPackages(t *testing.T) { } } +// Transform Table Test that operates solely on the varied_hidden fixture. +func TestTrimHiddenPackages(t *testing.T) { + base, err := ListPackages(filepath.Join(getTestdataRootDir(t), "src", "varied_hidden"), "varied") + if err != nil { + panic(err) + } + + table := map[string]struct { + main, tests bool // literal params to TrimHiddenPackages + ignore []string // transformed into IgnoredRuleset param to TrimHiddenPackages + trimmed []string // list of packages that should be trimmed in result PackageTree + }{ + // All of these implicitly verify that the varied/_never pkg is always + // trimmed, and that the varied/dotdotslash pkg is not trimmed even + // though it has errors. + "minimal trim": { + main: true, + tests: true, + }, + "ignore simple, lose testdata": { + main: true, + tests: true, + ignore: []string{"simple"}, + trimmed: []string{"simple", "simple/testdata"}, + }, + "no tests": { + main: true, + tests: false, + trimmed: []string{".onlyfromtests", "_secondorder"}, + }, + "ignore a reachable hidden": { + main: true, + tests: true, + ignore: []string{"_secondorder"}, + trimmed: []string{"_secondorder"}, + }, + "ignore a reachable hidden with another hidden solely reachable through it": { + main: true, + tests: true, + ignore: []string{".onlyfromtests"}, + trimmed: []string{".onlyfromtests", "_secondorder"}, + }, + "no main": { + main: false, + tests: true, + trimmed: []string{"", "_frommain"}, + }, + "no main or tests": { + main: false, + tests: false, + trimmed: []string{"", "_frommain", ".onlyfromtests", "_secondorder"}, + }, + "no main or tests and ignore simple": { + main: false, + tests: false, + ignore: []string{"simple"}, + trimmed: []string{"", "_frommain", ".onlyfromtests", "_secondorder", "simple", "simple/testdata"}, + }, + } + + for name, fix := range table { + t.Run(name, func(t *testing.T) { + want := base.Copy() + + var ig []string + for _, v := range fix.ignore { + ig = append(ig, path.Join("varied", v)) + } + got := base.TrimHiddenPackages(fix.main, fix.tests, NewIgnoredRuleset(ig)) + + for _, ip := range append(fix.trimmed, "_never") { + ip = path.Join("varied", ip) + if _, has := want.Packages[ip]; !has { + panic(fmt.Sprintf("bad input, %s does not exist in fixture ptree", ip)) + } + delete(want.Packages, ip) + } + + if !reflect.DeepEqual(want, got) { + if len(want.Packages) < 2 { + t.Errorf("Did not get expected PackageOrErrs:\n\t(GOT): %#v\n\t(WNT): %#v", got, want) + } else { + seen := make(map[string]bool) + for path, perr := range want.Packages { + seen[path] = true + if operr, exists := got.Packages[path]; !exists { + t.Errorf("Expected PackageOrErr for path %s was missing from output:\n\t%s", path, perr) + } else { + if !reflect.DeepEqual(perr, operr) { + t.Errorf("PkgOrErr for path %s was not as expected:\n\t(GOT): %#v\n\t(WNT): %#v", path, operr, perr) + } + } + } + + for path, operr := range got.Packages { + if seen[path] { + continue + } + + t.Errorf("Got PackageOrErr for path %s, but none was expected:\n\t%s", path, operr) + } + } + } + }) + } +} + // Test that ListPackages skips directories for which it lacks permissions to // enter and files it lacks permissions to read. func TestListPackagesNoPerms(t *testing.T) { From ecd7b87e518292e6b6bc2cac6a9e3597b4c29a6d Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sun, 15 Oct 2017 00:00:26 -0400 Subject: [PATCH 11/11] gps: Fix reflection handling in PackageTree.Copy() --- internal/gps/pkgtree/pkgtree.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 39a4fb6498..f622d61156 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -624,7 +624,21 @@ func (t PackageTree) Copy() PackageTree { var poe2 PackageOrErr if poe.Err != nil { - poe2.Err = reflect.New(reflect.ValueOf(poe.Err).Elem().Type()).Interface().(error) + refl := reflect.ValueOf(poe.Err) + switch refl.Kind() { + case reflect.Ptr: + poe2.Err = reflect.New(refl.Elem().Type()).Interface().(error) + case reflect.Slice: + err2 := reflect.MakeSlice(refl.Type(), refl.Len(), refl.Len()) + reflect.Copy(err2, refl) + poe2.Err = err2.Interface().(error) + default: + // This shouldn't be too onerous to maintain - the set of errors + // we can get here is restricted by what ListPackages() allows. + // So just panic if one is outside the expected kinds of ptr or + // slice, as that would mean we've missed something notable. + panic(fmt.Sprintf("unrecognized PackgeOrErr error type, %T", poe.Err)) + } } else { poe2.P = poe.P il, til := len(poe.P.Imports), len(poe.P.TestImports)