Wildcard ignores #1156

darkowlzz · 2017-09-11T14:55:27Z

What does this do / why do we need it?

Adds support for ignoring packages and subpackages using wildcard notation ("/*").

What should your reviewer look out for in this PR?

Implementation and tests. All the changes are in manifest, not touching gps.

Do you need help or clarification on anything?

Do we use "/*" or "/..." for this need?

Which issue(s) does this PR fix?

fixes #691

sdboyer · 2017-09-11T15:02:37Z

we definitely want /*, as the rules we're applying aren't the same as /....

this approach of doing it out in dep isn't going to work, unfortunately. at most, it helps us with the root project, but it doesn't do anything for dependencies. we've gotta handle this in the solver.

sdboyer · 2017-09-11T15:09:17Z

specifically, we'll need to introduce some new props, maybe a slice of strings, on the rootdata struct that hold the prefixes to exclude. then, everywhere that we currently check rd.ig in the solver...well, we can convert those to a new method, rootdata.isIgnored(path string), which should check both the literal rd.ig map and do a prefix-based exclusion check on each of the strings.

actually, no, not a slice. we'll want a trie, so that the prefix search is O(1) in the number of wildcard ignores specified by the root.

i'm musing on whether or not we want the ignores to be case-insensitive, in keeping with #1079. for now, let's assume no - not only is that easier, but i think it's more consistent with shell-globbing, and so will be less confusing for people.

darkowlzz · 2017-09-11T19:08:12Z

ah! right! didn't think of the dependencies. I'll try to implement it in the solver 😊

darkowlzz · 2017-09-12T13:24:28Z

Another attempt 😅
Removed the first commit of this PR which changed dep's manifest, but kept the ensure integration test which tested recursive ignore in the main project.

sdboyer

definitely looks like we're heading in the right direction!

sdboyer · 2017-09-13T01:20:43Z

internal/gps/pkgtree/pkgtree.go

 )

+// recursive ignore suffix


"recursive" isn't quite the right word to use for this, as it suggests a specific type of self-referential algorithm. better to just call these "wildcard ignores."

sdboyer · 2017-09-13T02:33:06Z

internal/gps/pkgtree/pkgtree.go

 )

+// recursive ignore suffix
+const recIgnoreSuffix = "/*"


i don't see a reason why we actually have to restrict to immediately following a slash - i think our only criteria here is that the asterisk has to be the last character.

sdboyer · 2017-09-13T02:34:59Z

internal/gps/pkgtree/pkgtree.go

@@ -445,6 +450,15 @@ func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore map[string]bo
 		ignore = make(map[string]bool)
 	}

+	// Create a radix tree for ignore prefixes
+	xt := radix.New()


this allocs because it returns a pointer, so let's defer creating the trie until the last possible moment - if we see something with the expected suffix.

darkowlzz · 2017-09-13T13:11:20Z

Renamed to "wildcard" and changed "/" to "" only.
Create new radix tree only when wildcard suffix is found in ignore list.
Updated docs/Gopkg.toml.md with wildcard ignore.

Do we have any existing repo that could be used to test dependency package ignore?

sdboyer · 2017-09-26T02:09:07Z

internal/gps/pkgtree/pkgtree.go

+			if xt == nil {
+				xt = radix.New()
+			}
+			i = strings.TrimSuffix(i, wcIgnoreSuffix)


This ends up on the strings.HasSuffix() call. better to just do the trim operation ourselves.

You mean for cases like "github.com/x/someprefix-*-somesuffix"?
Like using regex match?

nope, i meant that this is the implementation of string.TrimSuffix():

func TrimSuffix(s, suffix string) string { if HasSuffix(s, suffix) { return s[:len(s)-len(suffix)] } return s }

and the conditional has already been checked, so it's better to just do the s[:len(s)-len(suffix)] operation ourselves rather than calling strings.TrimSuffix(), as this method is processing-bound (possibly memory, possibly CPU), and this is within a loop, so easy microoptimizations like this are worth it.

sdboyer · 2017-09-26T02:19:35Z

so the PR as-is seems like it covers the essentials. now, i think, we need to do the harder work of thinking through the models and making sure that the edge cases are covered - especially the new ones that are induced by this.

here are a few i can think of:

i think a global ignore - "*" - should probably just not be allowed.
ineffectual ignores should be something we warn on, or at the very least do not include in hash inputs. that is, if github.com/foo* is ignored, then github.com/foo/bar* is ineffectual.
we should definitely have at least one more hash inputs test.
is there anywhere that we checks against ignored lists up in dep itself? perhaps on ensure -add. if so, they'll need to be updated. and we may need to add some new ones, though i'm not exactly sure what that would look like.

these are just what i can think of right now.

also, this change will probably entail us bumping the solver version. consequently, i may want to wait to merge it until just before we're ready to make the next release (which'll be v0.4.0), as otherwise we potentially create contention between team members who're chasing tip, vs. those using a release version.

darkowlzz · 2017-09-28T12:22:28Z

ignore global ignores
ignore ineffectual wildcard ignores
do not include ineffectual wildcard ignores in hash inputs
hash inputs test to verify exclusion of ineffectual wildcard ignores

is there anywhere that we checks against ignored lists up in dep itself? perhaps on ensure -add. if so, they'll need to be updated. and we may need to add some new ones, though i'm not exactly sure what that would look like.

@sdboyer Yes, right now we use IgnoredPackages only in ensure -add. But since this is passed to pkgtree.ToReachMap(), and ToReachMap() being aware of how to handle wildcards, there shouldn't be any issues.

I have a PR that uses IgnoredPackages but I think that can be avoided by using ToReachMap() and get package maps with all the ignoring rules applied.

darkowlzz · 2017-09-29T13:07:26Z

I went through both gps and dep, and found that most of the places where we care about ignoring use PackageTree.ToReachMap() with the ignored list passed to them to get a map of packages. So, no change would be required in those places.

SolveParameters.toRootdata() checks if a package is present in required as well as ignored. That needs to be changed to check the wildcard ignores too. Will add that in the next commit.

sdboyer · 2017-10-01T00:38:54Z

internal/gps/hash.go

+
+	// Add wildcard ignores to ignore list.
+	if s.rd.igpfx != nil {
+		s.rd.igpfx.Walk(radix.WalkFn(func(s string, v interface{}) bool {


nit: it should be unnecessary to have the type conversion here - just passing the anonymous function with the correct signature is sufficient.

sdboyer

just the two nits then we can merge

sdboyer · 2017-10-06T04:37:28Z

internal/gps/pkgtree/pkgtree.go

@@ -16,8 +16,13 @@ import (
 	"strconv"
 	"strings"
 	"unicode"
+
+	radix "github.com/armon/go-radix"


the package name is already radix, so this is unnecessary.

darkowlzz · 2017-10-06T13:12:33Z

Excited for this 😃

ibrasho

Just one question and then it LGTM.

ibrasho · 2017-10-06T13:37:38Z

internal/gps/pkgtree/pkgtree.go

+// 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


Why not initialize a new tree here instead of checking for nil later on?

We had that earlier but then we decided to allocate memory only when we get a wildcard suffix. Refer #1156 (comment)

sdboyer · 2017-10-06T23:11:21Z

woohoo!

i think we should roll a release after this. speaking of, this is missing a CHANGELOG update :)

* Create trie only when wildcard is found * Rename 'recursive' to 'wildcard' ignore

Adds test for createIgnorePrefixTree() which verifies the global ignore skip. Also, unifies createIgnorePrefixTree() of solver and pkgtree, moving it to pkgtree package.

sdboyer · 2017-10-12T17:09:28Z

fix CHANGELOG conflict for this, and we're good to go on it

googlebot · 2017-10-12T17:21:26Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

sdboyer · 2017-10-12T17:21:33Z

nvm, i just fixed it directly :)

darkowlzz requested review from ibrasho and sdboyer as code owners September 11, 2017 14:55

googlebot added the cla: yes label Sep 11, 2017

darkowlzz mentioned this pull request Sep 11, 2017

Enable glob syntax (/...) in ignored packages #709

Closed

darkowlzz force-pushed the recursive-ignore branch from 33bdf18 to 2ead833 Compare September 12, 2017 13:21

sdboyer reviewed Sep 13, 2017

View reviewed changes

darkowlzz changed the title ~~Recursive ignore~~ Wildcard ignores Sep 13, 2017

darkowlzz force-pushed the recursive-ignore branch 2 times, most recently from c768bc2 to 9e652ed Compare September 13, 2017 13:01

sdboyer mentioned this pull request Sep 20, 2017

Extend ignore property in manifest #1187

Closed

sdboyer reviewed Sep 26, 2017

View reviewed changes

darkowlzz force-pushed the recursive-ignore branch 5 times, most recently from 68ac3c2 to 4f73e5f Compare September 28, 2017 12:01

darkowlzz force-pushed the recursive-ignore branch from 4f73e5f to e43c5da Compare September 28, 2017 12:59

darkowlzz force-pushed the recursive-ignore branch from e43c5da to 560c6b5 Compare September 29, 2017 13:21

sdboyer reviewed Oct 2, 2017

View reviewed changes

sdboyer suggested changes Oct 6, 2017

View reviewed changes

darkowlzz force-pushed the recursive-ignore branch from 560c6b5 to b17e759 Compare October 6, 2017 13:07

ibrasho reviewed Oct 6, 2017

View reviewed changes

sdboyer approved these changes Oct 6, 2017

View reviewed changes

darkowlzz mentioned this pull request Oct 7, 2017

Add importer for govendor #815

Merged

darkowlzz added 11 commits October 7, 2017 19:47

test: add wildcard ignore integration test

86253c2

fix(gps): add recursive ignore support

415108c

gps: create trie only when wildcard is found

23c9329

* Create trie only when wildcard is found * Rename 'recursive' to 'wildcard' ignore

docs: add wildcard ignore example

6651439

gps: skip global ignores (*) and add tests

f271bcc

Adds test for createIgnorePrefixTree() which verifies the global ignore skip. Also, unifies createIgnorePrefixTree() of solver and pkgtree, moving it to pkgtree package.

test(gps): add test for rootdata.isIgnored()

fbfbad5

gps: wildcard ignore skip ineffectual ignores

9c4acd4

gps: no ineffectual wildcard igs in hash inputs

2139b31

gps: check wildcard ignored and required conflict

0cd7862

gps: minute cleanup

ec7ae32

Update CHANGELOG.

4376248

darkowlzz force-pushed the recursive-ignore branch from b17e759 to 4376248 Compare October 7, 2017 14:22

Merge branch 'master' into recursive-ignore

276ead9

googlebot added cla: no and removed cla: yes labels Oct 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wildcard ignores #1156

Wildcard ignores #1156

darkowlzz commented Sep 11, 2017

sdboyer commented Sep 11, 2017

sdboyer commented Sep 11, 2017

darkowlzz commented Sep 11, 2017

darkowlzz commented Sep 12, 2017 •

edited

Loading

sdboyer left a comment

sdboyer Sep 13, 2017

sdboyer Sep 13, 2017

sdboyer Sep 13, 2017

darkowlzz commented Sep 13, 2017

sdboyer Sep 26, 2017

darkowlzz Sep 26, 2017

sdboyer Sep 26, 2017

sdboyer commented Sep 26, 2017

darkowlzz commented Sep 28, 2017 •

edited

Loading

darkowlzz commented Sep 29, 2017

sdboyer Oct 1, 2017

sdboyer left a comment

sdboyer Oct 6, 2017

darkowlzz commented Oct 6, 2017

ibrasho left a comment

ibrasho Oct 6, 2017

darkowlzz Oct 6, 2017 •

edited

Loading

sdboyer commented Oct 6, 2017

sdboyer commented Oct 12, 2017

googlebot commented Oct 12, 2017

sdboyer commented Oct 12, 2017

Wildcard ignores #1156

Wildcard ignores #1156

Conversation

darkowlzz commented Sep 11, 2017

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

sdboyer commented Sep 11, 2017

sdboyer commented Sep 11, 2017

darkowlzz commented Sep 11, 2017

darkowlzz commented Sep 12, 2017 • edited Loading

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Sep 26, 2017

darkowlzz commented Sep 28, 2017 • edited Loading

darkowlzz commented Sep 29, 2017

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented Oct 6, 2017

ibrasho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

sdboyer commented Oct 6, 2017

sdboyer commented Oct 12, 2017

googlebot commented Oct 12, 2017

sdboyer commented Oct 12, 2017

darkowlzz commented Sep 12, 2017 •

edited

Loading

darkowlzz commented Sep 28, 2017 •

edited

Loading

darkowlzz Oct 6, 2017 •

edited

Loading