From cb51cd094990f4a8d5414841dc66175f3be8eecc Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 01:37:47 -0400 Subject: [PATCH 1/9] gps: Introduce IgnoredRuleset This provides a single type for handling both literal and wildcard ignore patterns, allowing us to tighten up a few interfaces. --- internal/gps/pkgtree/pkgtree.go | 73 ++++++++++++ internal/gps/pkgtree/pkgtree_test.go | 168 +++++++++++++++++++++++++++ 2 files changed, 241 insertions(+) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index fd4d10b7bc..4d8bb14941 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -1081,3 +1081,76 @@ func CreateIgnorePrefixTree(ig map[string]bool) *radix.Tree { return xt } + +// 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, resulting in the most efficient +// representation. +func NewIgnoredRuleset(ig map[string]struct{}) IgnoredRuleset { + if len(ig) == 0 { + return IgnoredRuleset{} + } + + ir := IgnoredRuleset{ + t: radix.New(), + } + + // Create a sorted list of all the ignores in order to ensure that wildcard + // precedence is recorded correctly in the trie. + sortedIgnores := make([]string, len(ig)) + for k := range ig { + sortedIgnores = append(sortedIgnores, k) + } + sort.Strings(sortedIgnores) + + for _, i := range sortedIgnores { + // Skip global ignore. + if i == wcIgnoreSuffix { + 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, wcIgnoreSuffix) { + // 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)-len(wcIgnoreSuffix)], true) + } else if !has || !wild { + ir.t.Insert(i, false) + } + } + + // The radix tree implementation is initialized with a single element + // representing the empty string. + if ir.t.Len() == 1 { + 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 ir.t == nil { + return false + } + + prefix, wildi, has := ir.t.LongestPrefix(path) + return has && (wildi.(bool) || path == prefix) +} diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index 554d9cac0e..474ba483c0 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -2127,3 +2127,171 @@ func TestCreateIgnorePrefixTree(t *testing.T) { }) } } + +func TestIgnoredRuleset(t *testing.T) { + type tfixm []struct { + path string + wild bool + } + cases := []struct { + name string + inputs map[string]struct{} + wantInTree tfixm + wantEmptyTree bool + shouldIgnore []string + shouldNotIgnore []string + }{ + { + name: "only skip global ignore", + inputs: map[string]struct{}{wcIgnoreSuffix: struct{}{}}, + wantEmptyTree: true, + }, + { + name: "ignores without ignore suffix", + inputs: map[string]struct{}{ + "x/y/z": struct{}{}, + "*a/b/c": struct{}{}, + "gophers": struct{}{}, + }, + 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: map[string]struct{}{ + "x/y/z*": struct{}{}, + "a/b/c": struct{}{}, + "gophers": struct{}{}, + }, + 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: map[string]struct{}{ + "*": struct{}{}, + "gophers*": struct{}{}, + "x/y/z*": struct{}{}, + "a/b/c": struct{}{}, + }, + 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: map[string]struct{}{ + "a/b*": struct{}{}, + "a/b/c*": struct{}{}, + "a/b/x/y": struct{}{}, + "a/c*": struct{}{}, + }, + 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: map[string]struct{}{ + "a/b*": struct{}{}, + "a/b": struct{}{}, + }, + wantInTree: tfixm{ + {path: "a/b", wild: true}, + }, + shouldIgnore: []string{ + "a/b", + "a/bb", + }, + shouldNotIgnore: []string{ + "a/", + "a/d", + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + igm := NewIgnoredRuleset(c.inputs) + + if c.wantEmptyTree { + if igm != nil && igm.t != nil { + t.Fatalf("wanted empty tree, got non-nil tree") + } + } + + // 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) + } + } + }) + } +} From 5be0c0000c77bc2de7c96269d3c62b637f0f8316 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 04:16:12 -0400 Subject: [PATCH 2/9] gps: Fully refactor pkgtree to use IgnoredRuleset --- internal/gps/pkgtree/pkgtree.go | 87 +++++++------ internal/gps/pkgtree/pkgtree_test.go | 175 +++++++++++++++------------ 2 files changed, 149 insertions(+), 113 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 4d8bb14941..3028d2b65a 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -550,14 +550,7 @@ type PackageTree struct { // "A": []string{}, // "A/bar": []string{"B/baz"}, // } -func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore map[string]bool) (ReachMap, map[string]*ProblemImportError) { - if ignore == nil { - ignore = make(map[string]bool) - } - - // Radix tree for ignore prefixes. - xt := CreateIgnorePrefixTree(ignore) - +func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore *IgnoredRuleset) (ReachMap, map[string]*ProblemImportError) { // world's simplest adjacency list workmap := make(map[string]wm) @@ -576,18 +569,10 @@ func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore map[string]bo continue } // Skip ignored packages - if ignore[ip] { + if ignore.IsIgnored(ip) { continue } - if xt != nil { - // Skip ignored packages prefix matches - _, _, ok := xt.LongestPrefix(ip) - if ok { - continue - } - } - // TODO (kris-nova) Disable to get staticcheck passing //imps = imps[:0] @@ -605,7 +590,7 @@ func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore map[string]bo // For each import, decide whether it should be ignored, or if it // belongs in the external or internal imports list. for _, imp := range imps { - if ignore[imp] || imp == "." { + if ignore.IsIgnored(imp) || imp == "." { continue } @@ -1093,28 +1078,23 @@ type IgnoredRuleset struct { // 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, resulting in the most efficient -// representation. -func NewIgnoredRuleset(ig map[string]struct{}) IgnoredRuleset { +// 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{} + return &IgnoredRuleset{} } - ir := IgnoredRuleset{ + ir := &IgnoredRuleset{ t: radix.New(), } - // Create a sorted list of all the ignores in order to ensure that wildcard + // Sort the list of all the ignores in order to ensure that wildcard // precedence is recorded correctly in the trie. - sortedIgnores := make([]string, len(ig)) - for k := range ig { - sortedIgnores = append(sortedIgnores, k) - } - sort.Strings(sortedIgnores) - - for _, i := range sortedIgnores { - // Skip global ignore. - if i == wcIgnoreSuffix { + sort.Strings(ig) + for _, i := range ig { + // Skip global ignore and empty string. + if i == wcIgnoreSuffix || i == "" { continue } @@ -1135,9 +1115,7 @@ func NewIgnoredRuleset(ig map[string]struct{}) IgnoredRuleset { } } - // The radix tree implementation is initialized with a single element - // representing the empty string. - if ir.t.Len() == 1 { + if ir.t.Len() == 0 { ir.t = nil } @@ -1146,11 +1124,44 @@ func NewIgnoredRuleset(ig map[string]struct{}) IgnoredRuleset { // IsIgnored indicates whether the provided path should be ignored, according to // the ruleset. -func (ir IgnoredRuleset) IsIgnored(path string) bool { - if ir.t == nil { +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+wcIgnoreSuffix) + } 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 474ba483c0..0a05b09aad 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -1524,10 +1524,10 @@ func TestToReachMap(t *testing.T) { var want ReachMap var name string var main, tests bool - var ignore map[string]bool + var ignore []string validate := func() { - got, em := vptree.ToReachMap(main, tests, true, ignore) + got, em := vptree.ToReachMap(main, tests, true, NewIgnoredRuleset(ignore)) if len(em) != 0 { t.Errorf("Should not have any error packages from ToReachMap, got %s", em) } @@ -1687,9 +1687,7 @@ func TestToReachMap(t *testing.T) { // ignoring the "varied" pkg has same effect as disabling main pkgs name = "ignore root" - ignore = map[string]bool{ - b(""): true, - } + ignore = []string{b("")} main = true validate() @@ -1723,9 +1721,7 @@ func TestToReachMap(t *testing.T) { // now, the fun stuff. punch a hole in the middle by cutting out // varied/simple name = "ignore varied/simple" - ignore = map[string]bool{ - b("simple"): true, - } + ignore = []string{b("simple")} except( // root pkg loses on everything in varied/simple/another // FIXME this is a bit odd, but should probably exclude m1p as well, @@ -1738,9 +1734,9 @@ func TestToReachMap(t *testing.T) { // widen the hole by excluding otherpath name = "ignore varied/{otherpath,simple}" - ignore = map[string]bool{ - b("otherpath"): true, - b("simple"): true, + ignore = []string{ + b("otherpath"), + b("simple"), } except( // root pkg loses on everything in varied/simple/another and varied/m1p @@ -1752,7 +1748,7 @@ func TestToReachMap(t *testing.T) { // remove namemismatch, though we're mostly beating a dead horse now name = "ignore varied/{otherpath,simple,namemismatch}" - ignore[b("namemismatch")] = true + ignore = append(ignore, b("namemismatch")) except( // root pkg loses on everything in varied/simple/another and varied/m1p bl("", "simple", "simple/another", "m1p", "otherpath", "namemismatch")+" hash encoding/binary go/parser github.com/golang/dep/internal/gps sort os github.com/Masterminds/semver", @@ -1851,9 +1847,9 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "nomatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "nomatch", + }), }, // should have the same effect as ignoring main { @@ -1862,9 +1858,9 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/example/varied": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied", + }), }, // now drop a more interesting one // we get github.com/golang/dep/internal/gps from m1p, too, so it should still be there @@ -1874,9 +1870,9 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + }), }, // now drop two { @@ -1885,10 +1881,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/namemismatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/namemismatch", + }), }, // make sure tests and main play nice with ignore { @@ -1897,10 +1893,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: false, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/namemismatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/namemismatch", + }), }, { name: "ignore simple and namemismatch, and no main", @@ -1908,10 +1904,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: false, tests: true, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/namemismatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/namemismatch", + }), }, { name: "ignore simple and namemismatch, and no main or tests", @@ -1919,10 +1915,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: false, tests: false, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/namemismatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/namemismatch", + }), }, // ignore two that should knock out gps { @@ -1931,10 +1927,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/m1p": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/m1p", + }), }, // finally, directly ignore some external packages { @@ -1943,11 +1939,11 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/golang/dep/internal/gps": true, - "go/parser": true, - "sort": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/golang/dep/internal/gps", + "go/parser", + "sort", + }), }, } { t.Run(testCase.name, testFlattenReachMap(&vptree, testCase)) @@ -1971,7 +1967,7 @@ func TestFlattenReachMap(t *testing.T) { type flattenReachMapCase struct { expect []string name string - ignore map[string]bool + ignore *IgnoredRuleset main, tests bool isStdLibFn func(string) bool } @@ -2135,7 +2131,7 @@ func TestIgnoredRuleset(t *testing.T) { } cases := []struct { name string - inputs map[string]struct{} + inputs []string wantInTree tfixm wantEmptyTree bool shouldIgnore []string @@ -2143,15 +2139,15 @@ func TestIgnoredRuleset(t *testing.T) { }{ { name: "only skip global ignore", - inputs: map[string]struct{}{wcIgnoreSuffix: struct{}{}}, + inputs: []string{wcIgnoreSuffix}, wantEmptyTree: true, }, { name: "ignores without ignore suffix", - inputs: map[string]struct{}{ - "x/y/z": struct{}{}, - "*a/b/c": struct{}{}, - "gophers": struct{}{}, + inputs: []string{ + "x/y/z", + "*a/b/c", + "gophers", }, wantInTree: tfixm{ {path: "x/y/z", wild: false}, @@ -2171,10 +2167,10 @@ func TestIgnoredRuleset(t *testing.T) { }, { name: "ignores with ignore suffix", - inputs: map[string]struct{}{ - "x/y/z*": struct{}{}, - "a/b/c": struct{}{}, - "gophers": struct{}{}, + inputs: []string{ + "x/y/z*", + "a/b/c", + "gophers", }, wantInTree: tfixm{ {path: "x/y/z", wild: true}, @@ -2194,11 +2190,11 @@ func TestIgnoredRuleset(t *testing.T) { }, { name: "global ignore with other strings", - inputs: map[string]struct{}{ - "*": struct{}{}, - "gophers*": struct{}{}, - "x/y/z*": struct{}{}, - "a/b/c": struct{}{}, + inputs: []string{ + "*", + "gophers*", + "x/y/z*", + "a/b/c", }, wantInTree: tfixm{ {path: "x/y/z", wild: true}, @@ -2220,11 +2216,11 @@ func TestIgnoredRuleset(t *testing.T) { }, { name: "ineffectual ignore with wildcard", - inputs: map[string]struct{}{ - "a/b*": struct{}{}, - "a/b/c*": struct{}{}, - "a/b/x/y": struct{}{}, - "a/c*": struct{}{}, + inputs: []string{ + "a/b*", + "a/b/c*", + "a/b/x/y", + "a/c*", }, wantInTree: tfixm{ {path: "a/c", wild: true}, @@ -2240,9 +2236,9 @@ func TestIgnoredRuleset(t *testing.T) { }, { name: "same path with and without wildcard", - inputs: map[string]struct{}{ - "a/b*": struct{}{}, - "a/b": struct{}{}, + inputs: []string{ + "a/b*", + "a/b", }, wantInTree: tfixm{ {path: "a/b", wild: true}, @@ -2256,15 +2252,40 @@ func TestIgnoredRuleset(t *testing.T) { "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 { - t.Run(c.name, func(t *testing.T) { - igm := NewIgnoredRuleset(c.inputs) + igm := NewIgnoredRuleset(c.inputs) + f := func(t *testing.T) { if c.wantEmptyTree { - if igm != nil && igm.t != nil { - t.Fatalf("wanted empty tree, got non-nil tree") + if igm.Len() != 0 { + t.Fatalf("wanted empty tree, but had %v elements", igm.Len()) } } @@ -2292,6 +2313,10 @@ func TestIgnoredRuleset(t *testing.T) { 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 4eb59de7fe3ef38e91ce2d256ac7172bf1ab7ce3 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 04:18:54 -0400 Subject: [PATCH 3/9] gps: Remove CreateIgnorePrefixTree --- internal/gps/pkgtree/pkgtree.go | 39 ------------ internal/gps/pkgtree/pkgtree_test.go | 89 ---------------------------- 2 files changed, 128 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 3028d2b65a..998de5fdd5 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -1028,45 +1028,6 @@ func uniq(a []string) []string { return a[:i] } -// CreateIgnorePrefixTree takes a set of strings to be ignored and returns a -// trie consisting of strings prefixed with wildcard ignore suffix (*). -func CreateIgnorePrefixTree(ig map[string]bool) *radix.Tree { - var xt *radix.Tree - - // Create a sorted list of all the ignores to have a proper order in - // ignores parsing. - sortedIgnores := make([]string, len(ig)) - for k := range ig { - sortedIgnores = append(sortedIgnores, k) - } - sort.Strings(sortedIgnores) - - for _, i := range sortedIgnores { - // Skip global ignore. - if i == "*" { - continue - } - - // Check if it's a recursive ignore. - if strings.HasSuffix(i, wcIgnoreSuffix) { - // Create trie if it doesn't exists. - if xt == nil { - xt = radix.New() - } - // Check if it is ineffectual. - _, _, ok := xt.LongestPrefix(i) - if ok { - // Skip ineffectual wildcard ignore. - continue - } - // Create the ignore prefix and insert in the radix tree. - xt.Insert(i[:len(i)-len(wcIgnoreSuffix)], true) - } - } - - return xt -} - // IgnoredRuleset comprises a set of rules for ignoring import paths. It can // manage both literal and prefix-wildcard matches. type IgnoredRuleset struct { diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index 0a05b09aad..b22f1dfa8e 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -2035,95 +2035,6 @@ func getTestdataRootDir(t *testing.T) string { return filepath.Join(cwd, "..", "_testdata") } -func TestCreateIgnorePrefixTree(t *testing.T) { - cases := []struct { - name string - ignoreMap map[string]bool - wantInTree []string - notWantInTree []string - wantNilTree bool - }{ - { - name: "empty ignore", - wantNilTree: true, - }, - { - name: "ignores without ignore suffix", - ignoreMap: map[string]bool{ - "x/y/z": true, - "*a/b/c": true, - "gophers": true, - }, - wantNilTree: true, - }, - { - name: "ignores with ignore suffix", - ignoreMap: map[string]bool{ - "x/y/z*": true, - "a/b/c": true, - "gophers": true, - }, - wantInTree: []string{"x/y/z"}, - notWantInTree: []string{"a/b/c", "gophers"}, - }, - { - name: "only skip global ignore", - ignoreMap: map[string]bool{"*": true}, - wantNilTree: true, - }, - { - name: "global ignore with other strings", - ignoreMap: map[string]bool{ - "*": true, - "gophers*": true, - "x/y/z*": true, - "a/b/c": true, - }, - wantInTree: []string{"gophers", "x/y/z"}, - notWantInTree: []string{"*", "a/b/c"}, - }, - { - name: "ineffectual ignore with wildcard", - ignoreMap: map[string]bool{ - "a/b*": true, - "a/b/c*": true, - "a/b/x/y": true, - "a/c*": true, - }, - wantInTree: []string{"a/b", "a/c"}, - notWantInTree: []string{"a/b/c", "a/b/x/y"}, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - gotTree := CreateIgnorePrefixTree(c.ignoreMap) - - if c.wantNilTree { - if gotTree != nil { - t.Fatalf("expected nil tree, got non-nil tree") - } - } - - // Check if the wildcard suffix ignores are in the tree. - for _, p := range c.wantInTree { - _, has := gotTree.Get(p) - if !has { - t.Fatalf("expected %q to be in the tree", p) - } - } - - // Check if non-wildcard suffix ignores are not in the tree. - for _, p := range c.notWantInTree { - _, has := gotTree.Get(p) - if has { - t.Fatalf("expected %q to not be in the tree", p) - } - } - }) - } -} - func TestIgnoredRuleset(t *testing.T) { type tfixm []struct { path string From b0face73a0b6d0b9627d28d2defabbce2c437163 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 09:11:24 -0400 Subject: [PATCH 4/9] gps: Convert to use IgnoredRuleset --- internal/gps/hash.go | 25 ++-------- internal/gps/hash_test.go | 19 ++++---- internal/gps/manifest.go | 27 ++++++----- internal/gps/rootdata.go | 24 ++-------- internal/gps/rootdata_test.go | 59 +----------------------- internal/gps/solve_bimodal_test.go | 5 +- internal/gps/solver.go | 15 +++--- internal/gps/solver_inputs_test.go | 6 +-- internal/gps/source_cache_bolt_encode.go | 19 ++++---- internal/gps/source_cache_test.go | 13 ++---- internal/gps/trace.go | 2 +- 11 files changed, 59 insertions(+), 155 deletions(-) diff --git a/internal/gps/hash.go b/internal/gps/hash.go index be35a17a2d..56c7fc62e1 100644 --- a/internal/gps/hash.go +++ b/internal/gps/hash.go @@ -81,30 +81,13 @@ func (s *solver) writeHashingInputs(w io.Writer) { // those will have already been implicitly incorporated by the import // lister. writeString(hhIgnores) - ig := make([]string, 0, len(s.rd.ig)) - for pkg := range s.rd.ig { - // Skip wildcard ignore. They are handled separately below. - if strings.HasSuffix(pkg, wcIgnoreSuffix) { - continue - } - - if !strings.HasPrefix(pkg, s.rd.rpt.ImportRoot) || !isPathPrefixOrEqual(s.rd.rpt.ImportRoot, pkg) { - ig = append(ig, pkg) - } - } - - // Add wildcard ignores to ignore list. - if s.rd.igpfx != nil { - s.rd.igpfx.Walk(func(s string, v interface{}) bool { - ig = append(ig, s+"*") - return false - }) - } + ig := s.rd.ir.ToSlice() sort.Strings(ig) - for _, igp := range ig { - writeString(igp) + if !strings.HasPrefix(igp, s.rd.rpt.ImportRoot) || !isPathPrefixOrEqual(s.rd.rpt.ImportRoot, igp) { + writeString(igp) + } } // Overrides *also* need their own special entry distinct from basic diff --git a/internal/gps/hash_test.go b/internal/gps/hash_test.go index aa5113962f..52898e50ae 100644 --- a/internal/gps/hash_test.go +++ b/internal/gps/hash_test.go @@ -11,6 +11,8 @@ import ( "strings" "testing" "text/tabwriter" + + "github.com/golang/dep/internal/gps/pkgtree" ) func TestHashInputs(t *testing.T) { @@ -64,10 +66,7 @@ func TestHashInputsReqsIgs(t *testing.T) { fix := basicFixtures["shared dependency with overlapping constraints"] rm := fix.rootmanifest().(simpleRootManifest).dup() - rm.ig = map[string]bool{ - "foo": true, - "bar": true, - } + rm.ig = pkgtree.NewIgnoredRuleset([]string{"foo", "bar"}) params := SolveParameters{ RootDir: string(fix.ds[0].n), @@ -611,7 +610,7 @@ func TestHashInputsIneffectualWildcardIgs(t *testing.T) { cases := []struct { name string - ignoreMap map[string]bool + ignoreMap []string elems []string }{ { @@ -634,10 +633,10 @@ func TestHashInputsIneffectualWildcardIgs(t *testing.T) { }, { name: "different wildcard ignores", - ignoreMap: map[string]bool{ - "foobar*": true, - "foobarbaz*": true, - "foozapbar*": true, + ignoreMap: []string{ + "foobar*", + "foobarbaz*", + "foozapbar*", }, elems: []string{ hhConstraints, @@ -662,7 +661,7 @@ func TestHashInputsIneffectualWildcardIgs(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - rm.ig = c.ignoreMap + rm.ig = pkgtree.NewIgnoredRuleset(c.ignoreMap) params.Manifest = rm diff --git a/internal/gps/manifest.go b/internal/gps/manifest.go index 625088f94b..e13ff95971 100644 --- a/internal/gps/manifest.go +++ b/internal/gps/manifest.go @@ -4,6 +4,8 @@ package gps +import "github.com/golang/dep/internal/gps/pkgtree" + // Manifest represents manifest-type data for a project at a particular version. // The constraints expressed in a manifest determine the set of versions that // are acceptable to try for a given project. @@ -36,14 +38,15 @@ type RootManifest interface { // them can harm the ecosystem as a whole. Overrides() ProjectConstraints - // IngoredPackages returns a set of import paths to ignore. These import - // paths can be within the root project, or part of other projects. Ignoring - // a package means that both it and its (unique) imports will be disregarded - // by all relevant solver operations. + // IngoredPackages returns a pkgtree.IgnoredRuleset, which comprises a set + // of import paths, or import path patterns, that are to be ignored during + // solving. These ignored import paths can be within the root project, or + // part of other projects. Ignoring a package means that both it and its + // (unique) imports will be disregarded by all relevant solver operations. // // It is an error to include a package in both the ignored and required // sets. - IgnoredPackages() map[string]bool + IgnoredPackages() *pkgtree.IgnoredRuleset // RequiredPackages returns a set of import paths to require. These packages // are required to be present in any solution. The list can include main @@ -76,8 +79,9 @@ func (m SimpleManifest) DependencyConstraints() ProjectConstraints { // simpleRootManifest exists so that we have a safe value to swap into solver // params when a nil Manifest is provided. type simpleRootManifest struct { - c, ovr ProjectConstraints - ig, req map[string]bool + c, ovr ProjectConstraints + ig *pkgtree.IgnoredRuleset + req map[string]bool } func (m simpleRootManifest) DependencyConstraints() ProjectConstraints { @@ -86,7 +90,7 @@ func (m simpleRootManifest) DependencyConstraints() ProjectConstraints { func (m simpleRootManifest) Overrides() ProjectConstraints { return m.ovr } -func (m simpleRootManifest) IgnoredPackages() map[string]bool { +func (m simpleRootManifest) IgnoredPackages() *pkgtree.IgnoredRuleset { return m.ig } func (m simpleRootManifest) RequiredPackages() map[string]bool { @@ -96,7 +100,6 @@ func (m simpleRootManifest) dup() simpleRootManifest { m2 := simpleRootManifest{ c: make(ProjectConstraints, len(m.c)), ovr: make(ProjectConstraints, len(m.ovr)), - ig: make(map[string]bool, len(m.ig)), req: make(map[string]bool, len(m.req)), } @@ -106,13 +109,13 @@ func (m simpleRootManifest) dup() simpleRootManifest { for k, v := range m.ovr { m2.ovr[k] = v } - for k, v := range m.ig { - m2.ig[k] = v - } for k, v := range m.req { m2.req[k] = v } + // IgnoredRulesets are immutable, and safe to reuse. + m2.ig = m.ig + return m2 } diff --git a/internal/gps/rootdata.go b/internal/gps/rootdata.go index eeae1c04f8..c4a7d6f357 100644 --- a/internal/gps/rootdata.go +++ b/internal/gps/rootdata.go @@ -17,6 +17,9 @@ type rootdata struct { // Path to the root of the project on which gps is operating. dir string + // Ruleset for ignored import paths. + ir *pkgtree.IgnoredRuleset + // Map of packages to ignore. ig map[string]bool @@ -57,7 +60,7 @@ type rootdata struct { // Ignores and requires are taken into consideration, stdlib is excluded, and // errors within the local set of package are not backpropagated. func (rd rootdata) externalImportList(stdLibFn func(string) bool) []string { - rm, _ := rd.rpt.ToReachMap(true, true, false, rd.ig) + rm, _ := rd.rpt.ToReachMap(true, true, false, rd.ir) reach := rm.FlattenFn(stdLibFn) // If there are any requires, slide them into the reach list, as well. @@ -194,7 +197,7 @@ func (rd rootdata) rootAtom() atomWithPackages { list := make([]string, 0, len(rd.rpt.Packages)) for path, pkg := range rd.rpt.Packages { - if pkg.Err != nil && !rd.ig[path] { + if pkg.Err != nil && !rd.ir.IsIgnored(path) { list = append(list, path) } } @@ -205,20 +208,3 @@ func (rd rootdata) rootAtom() atomWithPackages { pl: list, } } - -// isIgnored checks if a given path is ignored, comparing with the list of -// ignore packages and ignore prefixes. -func (rd rootdata) isIgnored(path string) bool { - // Check if the path is present in ignore packages. - if rd.ig[path] { - return true - } - - if rd.igpfx != nil { - // Check if the path matches any of the ignore prefixes. - _, _, ok := rd.igpfx.LongestPrefix(path) - return ok - } - - return false -} diff --git a/internal/gps/rootdata_test.go b/internal/gps/rootdata_test.go index 8578c12394..7890669b8b 100644 --- a/internal/gps/rootdata_test.go +++ b/internal/gps/rootdata_test.go @@ -58,7 +58,7 @@ func TestRootdataExternalImports(t *testing.T) { // Add an ignore, but not on the required path (Prepare makes that // combination impossible) - rd.ig["b"] = true + rd.ir = pkgtree.NewIgnoredRuleset([]string{"b"}) want = []string{"a", "c"} got = rd.externalImportList(params.stdLibFn) if !reflect.DeepEqual(want, got) { @@ -194,7 +194,7 @@ func TestGetApplicableConstraints(t *testing.T) { // violate the principle of least surprise? name: "ignore imported and overridden pkg", mut: func() { - rd.ig["d"] = true + rd.ir = pkgtree.NewIgnoredRuleset([]string{"d"}) }, result: []workingConstraint{ { @@ -224,58 +224,3 @@ func TestGetApplicableConstraints(t *testing.T) { }) } } - -func TestIsIgnored(t *testing.T) { - cases := []struct { - name string - ignorePkgs map[string]bool - wantIgnored []string - wantNotIgnored []string - }{ - { - name: "no ignore", - }, - { - name: "ignores without wildcard", - ignorePkgs: map[string]bool{ - "a/b/c": true, - "m/n": true, - "gophers": true, - }, - wantIgnored: []string{"a/b/c", "m/n", "gophers"}, - wantNotIgnored: []string{"somerandomstring"}, - }, - { - name: "ignores with wildcard", - ignorePkgs: map[string]bool{ - "a/b/c*": true, - "m/n*/o": true, - "*x/y/z": true, - "A/B*/C/D*": true, - }, - wantIgnored: []string{"a/b/c", "a/b/c/d", "a/b/c-d", "m/n*/o", "*x/y/z", "A/B*/C/D", "A/B*/C/D/E"}, - wantNotIgnored: []string{"m/n/o", "m/n*", "x/y/z", "*x/y/z/a", "*x", "A/B", "A/B*/C"}, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - rd := rootdata{ - ig: c.ignorePkgs, - igpfx: pkgtree.CreateIgnorePrefixTree(c.ignorePkgs), - } - - for _, p := range c.wantIgnored { - if !rd.isIgnored(p) { - t.Fatalf("expected %q to be ignored", p) - } - } - - for _, p := range c.wantNotIgnored { - if rd.isIgnored(p) { - t.Fatalf("expected %q to be not ignored", p) - } - } - }) - } -} diff --git a/internal/gps/solve_bimodal_test.go b/internal/gps/solve_bimodal_test.go index 668ab1dff2..e55e875415 100644 --- a/internal/gps/solve_bimodal_test.go +++ b/internal/gps/solve_bimodal_test.go @@ -1345,12 +1345,9 @@ func (f bimodalFixture) rootmanifest() RootManifest { m := simpleRootManifest{ c: pcSliceToMap(f.ds[0].deps), ovr: f.ovr, - ig: make(map[string]bool), + ig: pkgtree.NewIgnoredRuleset(f.ignore), req: make(map[string]bool), } - for _, ig := range f.ignore { - m.ig[ig] = true - } for _, req := range f.require { m.req[req] = true } diff --git a/internal/gps/solver.go b/internal/gps/solver.go index 106ab5351a..c6065de3a4 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -184,7 +184,7 @@ func (params SolveParameters) toRootdata() (rootdata, error) { } rd := rootdata{ - ig: params.Manifest.IgnoredPackages(), + ir: params.Manifest.IgnoredPackages(), req: params.Manifest.RequiredPackages(), ovr: params.Manifest.Overrides(), rpt: params.RootPackageTree.Copy(), @@ -206,13 +206,10 @@ func (params SolveParameters) toRootdata() (rootdata, error) { rd.ovr = make(ProjectConstraints) } - // Create ignore prefix tree using the provided ignore packages - rd.igpfx = pkgtree.CreateIgnorePrefixTree(rd.ig) - - if len(rd.ig) != 0 { + if rd.ir.Len() > 0 { var both []string for pkg := range params.Manifest.RequiredPackages() { - if rd.isIgnored(pkg) { + if rd.ir.IsIgnored(pkg) { both = append(both, pkg) } } @@ -633,7 +630,7 @@ func (s *solver) getImportsAndConstraintsOf(a atomWithPackages) ([]string, []com return nil, nil, err } - rm, em := ptree.ToReachMap(true, false, true, s.rd.ig) + rm, em := ptree.ToReachMap(true, false, true, s.rd.ir) // Use maps to dedupe the unique internal and external packages. exmap, inmap := make(map[string]struct{}), make(map[string]struct{}) @@ -661,14 +658,14 @@ func (s *solver) getImportsAndConstraintsOf(a atomWithPackages) ([]string, []com // explicitly listed in the atom for _, pkg := range a.pl { // Skip ignored packages - if s.rd.isIgnored(pkg) { + if s.rd.ir.IsIgnored(pkg) { continue } ie, exists := rm[pkg] if !exists { // Missing package here *should* only happen if the target pkg was - // poisoned. Check the errors map + // poisoned; check the errors map. if importErr, eexists := em[pkg]; eexists { return nil, nil, importErr } diff --git a/internal/gps/solver_inputs_test.go b/internal/gps/solver_inputs_test.go index 3784c2fdd0..0393597fdc 100644 --- a/internal/gps/solver_inputs_test.go +++ b/internal/gps/solver_inputs_test.go @@ -94,7 +94,7 @@ func TestBadSolveOpts(t *testing.T) { } params.Manifest = simpleRootManifest{ - ig: map[string]bool{"foo": true}, + ig: pkgtree.NewIgnoredRuleset([]string{"foo"}), req: map[string]bool{"foo": true}, } _, err = Prepare(params, sm) @@ -105,7 +105,7 @@ func TestBadSolveOpts(t *testing.T) { } params.Manifest = simpleRootManifest{ - ig: map[string]bool{"foo": true, "bar": true}, + ig: pkgtree.NewIgnoredRuleset([]string{"foo", "bar"}), req: map[string]bool{"foo": true, "bar": true}, } _, err = Prepare(params, sm) @@ -116,7 +116,7 @@ func TestBadSolveOpts(t *testing.T) { } params.Manifest = simpleRootManifest{ - ig: map[string]bool{"foo*": true}, + ig: pkgtree.NewIgnoredRuleset([]string{"foo*"}), req: map[string]bool{"foo/bar": true}, } _, err = Prepare(params, sm) diff --git a/internal/gps/source_cache_bolt_encode.go b/internal/gps/source_cache_bolt_encode.go index c00513afed..006c6ca4de 100644 --- a/internal/gps/source_cache_bolt_encode.go +++ b/internal/gps/source_cache_bolt_encode.go @@ -104,7 +104,7 @@ func cachePutManifest(b *bolt.Bucket, m Manifest) error { return nil } - ignored := rm.IgnoredPackages() + ignored := rm.IgnoredPackages().ToSlice() if len(ignored) > 0 { ig, err := b.CreateBucket(cacheKeyIgnored) if err != nil { @@ -112,13 +112,11 @@ func cachePutManifest(b *bolt.Bucket, m Manifest) error { } key := make(nuts.Key, nuts.KeyLen(uint64(len(ignored)-1))) var i uint64 - for ip, ok := range ignored { - if ok { - key.Put(i) - i++ - if err := ig.Put(key, []byte(ip)); err != nil { - return err - } + for _, ip := range ignored { + key.Put(i) + i++ + if err := ig.Put(key, []byte(ip)); err != nil { + return err } } } @@ -173,7 +171,6 @@ func cacheGetManifest(b *bolt.Bucket) (RootManifest, error) { m := &simpleRootManifest{ c: make(ProjectConstraints), ovr: make(ProjectConstraints), - ig: make(map[string]bool), req: make(map[string]bool), } @@ -198,10 +195,12 @@ func cacheGetManifest(b *bolt.Bucket) (RootManifest, error) { // Ignored if ig := b.Bucket(cacheKeyIgnored); ig != nil { + var igslice []string err := ig.ForEach(func(_, v []byte) error { - m.ig[string(v)] = true + igslice = append(igslice, string(v)) return nil }) + m.ig = pkgtree.NewIgnoredRuleset(igslice) if err != nil { return nil, errors.Wrap(err, "failed to get ignored") } diff --git a/internal/gps/source_cache_test.go b/internal/gps/source_cache_test.go index 7331b4396a..e219f4858f 100644 --- a/internal/gps/source_cache_test.go +++ b/internal/gps/source_cache_test.go @@ -7,6 +7,7 @@ package gps import ( "io/ioutil" "log" + "reflect" "sort" "testing" "time" @@ -100,14 +101,11 @@ func (test singleSourceCacheTest) run(t *testing.T) { Constraint: testSemverConstraint(t, "2.0.0"), }, }, - ig: map[string]bool{ - "a": true, - "b": true, - }, req: map[string]bool{ "c": true, "d": true, }, + ig: pkgtree.NewIgnoredRuleset([]string{"a", "b"}), } var l Lock = &safeLock{ h: []byte("test_hash"), @@ -149,14 +147,11 @@ func (test singleSourceCacheTest) run(t *testing.T) { Constraint: testSemverConstraint(t, "2.0.0"), }, }, - ig: map[string]bool{ - "c": true, - "d": true, - }, req: map[string]bool{ "a": true, "b": true, }, + ig: pkgtree.NewIgnoredRuleset([]string{"c", "d"}), } l = &safeLock{ h: []byte("different_test_hash"), @@ -416,7 +411,7 @@ func compareManifests(t *testing.T, want, got Manifest) { { want, got := wantRM.IgnoredPackages(), gotRM.IgnoredPackages() - if !mapStringBoolEqual(want, got) { + if !reflect.DeepEqual(want.ToSlice(), got.ToSlice()) { t.Errorf("unexpected ignored packages:\n\t(GOT): %#v\n\t(WNT): %#v", got, want) } } diff --git a/internal/gps/trace.go b/internal/gps/trace.go index 2f75b6ee47..05976573b6 100644 --- a/internal/gps/trace.go +++ b/internal/gps/trace.go @@ -117,7 +117,7 @@ func (s *solver) traceSelectRoot(ptree pkgtree.PackageTree, cdeps []completeDep) // This duplicates work a bit, but we're in trace mode and it's only once, // so who cares - rm, _ := ptree.ToReachMap(true, true, false, s.rd.ig) + rm, _ := ptree.ToReachMap(true, true, false, s.rd.ir) s.tl.Printf("Root project is %q", s.rd.rpt.ImportRoot) From c20530499e110bcec223e75e8738ea7066c72dd3 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 10:11:56 -0400 Subject: [PATCH 5/9] gps: Fix up odds and ends around IgnoreRulesets The most crucial piece is fixing the hasher to make its prefix checks without the wildcard suffix for the purpose of determining whether the ignore affects the root project; otherwise, wildcards on the root of the current project (why would you do this?) would be erroneously included in hash inputs. --- internal/gps/hash.go | 5 ++++- internal/gps/rootdata.go | 6 ------ internal/gps/solver.go | 5 +---- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/internal/gps/hash.go b/internal/gps/hash.go index 56c7fc62e1..47b691e7a4 100644 --- a/internal/gps/hash.go +++ b/internal/gps/hash.go @@ -85,7 +85,10 @@ func (s *solver) writeHashingInputs(w io.Writer) { ig := s.rd.ir.ToSlice() sort.Strings(ig) for _, igp := range ig { - if !strings.HasPrefix(igp, s.rd.rpt.ImportRoot) || !isPathPrefixOrEqual(s.rd.rpt.ImportRoot, igp) { + // Typical prefix comparison checks will erroneously fail if the wildcard + // is present. Trim it off, if present. + tigp := strings.TrimSuffix(igp, "*") + if !strings.HasPrefix(tigp, s.rd.rpt.ImportRoot) || !isPathPrefixOrEqual(s.rd.rpt.ImportRoot, tigp) { writeString(igp) } } diff --git a/internal/gps/rootdata.go b/internal/gps/rootdata.go index c4a7d6f357..22e7fecad1 100644 --- a/internal/gps/rootdata.go +++ b/internal/gps/rootdata.go @@ -20,12 +20,6 @@ type rootdata struct { // Ruleset for ignored import paths. ir *pkgtree.IgnoredRuleset - // Map of packages to ignore. - ig map[string]bool - - // Radix tree of ignore prefixes. - igpfx *radix.Tree - // Map of packages to require. req map[string]bool diff --git a/internal/gps/solver.go b/internal/gps/solver.go index c6065de3a4..a325101a9d 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -195,10 +195,7 @@ func (params SolveParameters) toRootdata() (rootdata, error) { an: params.ProjectAnalyzer, } - // Ensure the required, ignore and overrides maps are at least initialized - if rd.ig == nil { - rd.ig = make(map[string]bool) - } + // Ensure the required and overrides maps are at least initialized if rd.req == nil { rd.req = make(map[string]bool) } From cd42cef0c0a2ba12b389bb8342f5dd271544de94 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 10:14:03 -0400 Subject: [PATCH 6/9] dep: update Manifest with IgnoredRuleset logic --- manifest.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/manifest.go b/manifest.go index 2140992210..87ea79292a 100644 --- a/manifest.go +++ b/manifest.go @@ -14,6 +14,7 @@ import ( "sync" "github.com/golang/dep/internal/gps" + "github.com/golang/dep/internal/gps/pkgtree" "github.com/pelletier/go-toml" "github.com/pkg/errors" ) @@ -53,7 +54,7 @@ type rawProject struct { Source string `toml:"source,omitempty"` } -// NewManifest instantites a new manifest. +// NewManifest instantiates a new manifest. func NewManifest() *Manifest { return &Manifest{ Constraints: make(gps.ProjectConstraints), @@ -376,17 +377,8 @@ func (m *Manifest) Overrides() gps.ProjectConstraints { } // IgnoredPackages returns a set of import paths to ignore. -func (m *Manifest) IgnoredPackages() map[string]bool { - if len(m.Ignored) == 0 { - return nil - } - - mp := make(map[string]bool, len(m.Ignored)) - for _, i := range m.Ignored { - mp[i] = true - } - - return mp +func (m *Manifest) IgnoredPackages() *pkgtree.IgnoredRuleset { + return pkgtree.NewIgnoredRuleset(m.Ignored) } // HasConstraintsOn checks if the manifest contains either constraints or From 9139a33e62cfa2ccaf5629c7eeee83d7dc7c42e4 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 10:36:12 -0400 Subject: [PATCH 7/9] dep: Add more harness tests for wildcard ignores This also adds `dep hash-inputs` output to both of the harness tests, so that it's easier for developers to keep track of the expected output in these cases. --- cmd/dep/integration_test.go | 8 +++-- .../wildcard-ignore/final/Gopkg.lock | 2 +- .../pkg-ignored/wildcard-ignore/stdout.txt | 11 +++++++ .../pkg-ignored/wildcard-ignore/testcase.json | 3 +- .../wildcard-other-root/final/Gopkg.lock | 9 ++++++ .../wildcard-other-root/final/Gopkg.toml | 1 + .../wildcard-other-root/initial/Gopkg.lock | 9 ++++++ .../wildcard-other-root/initial/Gopkg.toml | 1 + .../wildcard-other-root/initial/main.go | 12 ++++++++ .../initial/samples/samples.go | 7 +++++ .../initial/samples/subsamples/subsamples.go | 7 +++++ .../wildcard-other-root/stdout.txt | 9 ++++++ .../wildcard-other-root/testcase.json | 8 +++++ internal/test/integration/testcase.go | 29 +++++++++++++++---- 14 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/stdout.txt create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/final/Gopkg.lock create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/final/Gopkg.toml create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/Gopkg.lock create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/Gopkg.toml create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/main.go create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/samples/samples.go create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/samples/subsamples/subsamples.go create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/stdout.txt create mode 100644 cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/testcase.json diff --git a/cmd/dep/integration_test.go b/cmd/dep/integration_test.go index 5548cba516..cc9f959cc9 100644 --- a/cmd/dep/integration_test.go +++ b/cmd/dep/integration_test.go @@ -123,8 +123,12 @@ func testIntegration(name, relPath, wd string, run integration.RunFunc) func(t * // Check error raised in final command testCase.CompareError(err, testProj.GetStderr()) - // Check output - testCase.CompareOutput(testProj.GetStdout()) + if *test.UpdateGolden { + testCase.UpdateOutput(testProj.GetStdout()) + } else { + // Check output + testCase.CompareOutput(testProj.GetStdout()) + } // Check vendor paths testProj.CompareImportPaths() diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/final/Gopkg.lock b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/final/Gopkg.lock index 6dffa9af2d..1e637dd503 100644 --- a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/final/Gopkg.lock +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/final/Gopkg.lock @@ -10,6 +10,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "2381e3b3c6973c22f589e8f7cdf4fa63d009cfb815f86c7987a57b6b3661ebc3" + inputs-digest = "5210e61a67f6e64dabb1eb8f28df2dbeeedfca1588c102067a6ec8a35e0b15f9" solver-name = "gps-cdcl" solver-version = 1 diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/stdout.txt b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/stdout.txt new file mode 100644 index 0000000000..74542e41e5 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/stdout.txt @@ -0,0 +1,11 @@ +-CONSTRAINTS- +github.com/sdboyer/deptest +b-master +-IMPORTS/REQS- +github.com/sdboyer/deptest +-IGNORES- +-OVERRIDES- +-ANALYZER- +dep +1 + diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/testcase.json b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/testcase.json index 729de9d0f4..5641e85616 100644 --- a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/testcase.json +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-ignore/testcase.json @@ -1,6 +1,7 @@ { "commands": [ - ["ensure"] + ["ensure"], + ["hash-inputs"] ], "error-expected": "", "vendor-final": [ diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/final/Gopkg.lock b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/final/Gopkg.lock new file mode 100644 index 0000000000..53e42dcc48 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/final/Gopkg.lock @@ -0,0 +1,9 @@ +# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. + + +[solve-meta] + analyzer-name = "dep" + analyzer-version = 1 + inputs-digest = "b02b7a80e20404724ba5dbffab28e772017b03800916327f58bff0da86071b6a" + solver-name = "gps-cdcl" + solver-version = 1 diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/final/Gopkg.toml b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/final/Gopkg.toml new file mode 100644 index 0000000000..f54b63d573 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/final/Gopkg.toml @@ -0,0 +1 @@ +ignored = ["github.com/sdboyer/deptest*", "github.com/golang/notexist/samples*"] diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/Gopkg.lock b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/Gopkg.lock new file mode 100644 index 0000000000..bef2d0092e --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/Gopkg.lock @@ -0,0 +1,9 @@ +# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. + + +[solve-meta] + analyzer-name = "dep" + analyzer-version = 1 + inputs-digest = "ab4fef131ee828e96ba67d31a7d690bd5f2f42040c6766b1b12fe856f87e0ff7" + solver-name = "gps-cdcl" + solver-version = 1 diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/Gopkg.toml b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/Gopkg.toml new file mode 100644 index 0000000000..f54b63d573 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/Gopkg.toml @@ -0,0 +1 @@ +ignored = ["github.com/sdboyer/deptest*", "github.com/golang/notexist/samples*"] diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/main.go b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/main.go new file mode 100644 index 0000000000..e23fcf34c5 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/main.go @@ -0,0 +1,12 @@ +// 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 ( + _ "github.com/sdboyer/deptest" +) + +func main() { +} diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/samples/samples.go b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/samples/samples.go new file mode 100644 index 0000000000..d07de170f7 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/samples/samples.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 samples + +import _ "github.com/sdboyer/deptestdos" diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/samples/subsamples/subsamples.go b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/samples/subsamples/subsamples.go new file mode 100644 index 0000000000..5136538e6d --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/initial/samples/subsamples/subsamples.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 subsamples + +import _ "github.com/sdboyer/dep-test" diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/stdout.txt b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/stdout.txt new file mode 100644 index 0000000000..a273de0e56 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/stdout.txt @@ -0,0 +1,9 @@ +-CONSTRAINTS- +-IMPORTS/REQS- +-IGNORES- +github.com/sdboyer/deptest* +-OVERRIDES- +-ANALYZER- +dep +1 + diff --git a/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/testcase.json b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/testcase.json new file mode 100644 index 0000000000..4f16d1c611 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/pkg-ignored/wildcard-other-root/testcase.json @@ -0,0 +1,8 @@ +{ + "commands": [ + ["ensure"], + ["hash-inputs"] + ], + "error-expected": "", + "vendor-final": [] +} diff --git a/internal/test/integration/testcase.go b/internal/test/integration/testcase.go index 6e2934eb71..bd290e1498 100644 --- a/internal/test/integration/testcase.go +++ b/internal/test/integration/testcase.go @@ -83,21 +83,38 @@ func (tc *TestCase) CompareFile(goldenPath, working string) { gotExists, got, err := getFile(working) if err != nil { - tc.t.Fatalf("Error reading project file %s: %s", goldenPath, err) + tc.t.Fatalf("Error reading project file %q: %s", goldenPath, err) } wantExists, want, err := getFile(golden) if err != nil { - tc.t.Fatalf("Error reading testcase file %s: %s", goldenPath, err) + tc.t.Fatalf("Error reading testcase file %q: %s", goldenPath, err) } if wantExists && gotExists { if want != got { - tc.t.Errorf("expected %s, got %s", want, got) + tc.t.Errorf("%s was not as expected\n(WNT):\n%s\n(GOT):\n%s", filepath.Base(goldenPath), want, got) } } else if !wantExists && gotExists { - tc.t.Errorf("%s created where none was expected", goldenPath) + tc.t.Errorf("%q created where none was expected", goldenPath) } else if wantExists && !gotExists { - tc.t.Errorf("%s not created where one was expected", goldenPath) + tc.t.Errorf("%q not created where one was expected", goldenPath) + } +} + +// UpdateFile updates the golden file with the working result. +func (tc *TestCase) UpdateOutput(stdout string) { + stdoutPath := filepath.Join(tc.rootPath, "stdout.txt") + _, err := os.Stat(stdoutPath) + if err != nil { + if os.IsNotExist(err) { + // Don't update the stdout.txt file if it doesn't exist. + return + } + panic(err) + } + + if err := tc.WriteFile(stdoutPath, stdout); err != nil { + tc.t.Fatal(err) } } @@ -116,7 +133,7 @@ func (tc *TestCase) CompareOutput(stdout string) { stdout = normalizeLines(stdout) if expStr != stdout { - tc.t.Errorf("(WNT):\n%s\n(GOT):\n%s\n", expStr, stdout) + tc.t.Errorf("stdout was not as expected\n(WNT):\n%s\n(GOT):\n%s\n", expStr, stdout) } } From 50326d7382746cc1abb7b56df92f44b1ccfeba13 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 14:17:42 -0400 Subject: [PATCH 8/9] dep: Minor test and lint nits --- .../testdata/harness_tests/init/glide/case1/final/Gopkg.toml | 2 +- internal/test/integration/testcase.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml b/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml index f9d63b74dc..ba8a4248b5 100644 --- a/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml +++ b/cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml @@ -1,4 +1,4 @@ -ignored = ["github.com/sdboyer/dep-test","github.com/golang/notexist/samples"] +ignored = ["github.com/golang/notexist/samples","github.com/sdboyer/dep-test"] [[constraint]] name = "github.com/carolynvs/deptest-subpkg" diff --git a/internal/test/integration/testcase.go b/internal/test/integration/testcase.go index bd290e1498..bd772da7d8 100644 --- a/internal/test/integration/testcase.go +++ b/internal/test/integration/testcase.go @@ -101,7 +101,7 @@ func (tc *TestCase) CompareFile(goldenPath, working string) { } } -// UpdateFile updates the golden file with the working result. +// UpdateOutput updates the golden file for stdout with the working result. func (tc *TestCase) UpdateOutput(stdout string) { stdoutPath := filepath.Join(tc.rootPath, "stdout.txt") _, err := os.Stat(stdoutPath) From da9b75028dce6c649fd2c020fb7735c6a8b9b517 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 22:03:41 -0400 Subject: [PATCH 9/9] gps: remove wcIgnoreSuffix const It's clearer just to use the string literal in this case. --- internal/gps/hash.go | 2 -- internal/gps/pkgtree/pkgtree.go | 11 ++++------- internal/gps/pkgtree/pkgtree_test.go | 2 +- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/internal/gps/hash.go b/internal/gps/hash.go index 47b691e7a4..556933bcf3 100644 --- a/internal/gps/hash.go +++ b/internal/gps/hash.go @@ -13,8 +13,6 @@ import ( "strings" ) -const wcIgnoreSuffix = "*" - // string headers used to demarcate sections in hash input creation const ( hhConstraints = "-CONSTRAINTS-" diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 998de5fdd5..78be8b42f4 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -22,9 +22,6 @@ import ( "github.com/armon/go-radix" ) -// wildcard ignore suffix -const wcIgnoreSuffix = "*" - // Package represents a Go package. It contains a subset of the information // go/build.Package does. type Package struct { @@ -1055,7 +1052,7 @@ func NewIgnoredRuleset(ig []string) *IgnoredRuleset { sort.Strings(ig) for _, i := range ig { // Skip global ignore and empty string. - if i == wcIgnoreSuffix || i == "" { + if i == "*" || i == "" { continue } @@ -1063,14 +1060,14 @@ func NewIgnoredRuleset(ig []string) *IgnoredRuleset { // 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, wcIgnoreSuffix) { + 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)-len(wcIgnoreSuffix)], true) + ir.t.Insert(i[:len(i)-1], true) } else if !has || !wild { ir.t.Insert(i, false) } @@ -1116,7 +1113,7 @@ func (ir *IgnoredRuleset) ToSlice() []string { ir.t.Walk(func(s string, v interface{}) bool { if s != "" { if v.(bool) { - items = append(items, s+wcIgnoreSuffix) + items = append(items, s+"*") } else { items = append(items, s) } diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index b22f1dfa8e..bb97c11c6b 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -2050,7 +2050,7 @@ func TestIgnoredRuleset(t *testing.T) { }{ { name: "only skip global ignore", - inputs: []string{wcIgnoreSuffix}, + inputs: []string{"*"}, wantEmptyTree: true, }, {