Move bad go code under a vendor/ subdirectory.

[darkowlzz]

What does this do / why do we need it?

This fixes an issue where running dep status on dep itself resulted in
error from ListPackages due to parsing the bad go code that exists in
the testdata.

analysis of local packages failed: import path github.com/golang/dep/internal/gps/_testdata/src/canon_confl had conflicting import comments: "vanity1", "vanity2"

This started happening after adding tests from #1017 .

What should your reviewer look out for in this PR?

The solution to the issue. Any other better way to fix the same issue?

Do you need help or clarification on anything?

No.

[sdboyer]

we can't abuse vendor in this way - we need to fix the underlying problem in the logic that we're caring about these errors at all. same basic issue as in #1251, I think.

[darkowlzz]

@sdboyer yes, #1251 is the same issue and the inability to run dep status in dep itself is sickening me 🤢
I wanna fix it 😁

From #1251

we have to adaptively determine from the import set in non-dot/underscore-led dirs whether or not it's needed to actually explore those, then restart work if we determine that it is.

So, I think I know how to do this partially. Like, defer parsing dot and underscore-led dirs and keeping a record of all the dirs that might require parsing once we determine if we need them.
But I'm not sure how is that determined? Is it based on the import path? like relative import to dot or underscore-led packages?

Maybe from the PackageTree obtained after parsing all the non-dot/underscore-led dirs, check if any of the import paths have dot or underscore in them. If they do, parse that specific package only?

[sdboyer]

so there's two basic approaches i can picture. one involves new a new parameter to ListPackages() that decides whether it should descend into typically-ignored dirs - .*, _*, testdata. but let's defer that for a moment.

the other approach could be a method on PackageTree, perhaps func (t PackageTree) TrimHiddenAndUnreachable() PackageTree, the essence of which would go to answer your question:

But I'm not sure how is that determined? Is it based on the import path? like relative import to dot or underscore-led packages?

the idea here would be creating a new PackageTree that only contains package entries which either:

1. are not excluded by one of the hidden naming rules, .*, _*, testdata
2. are transitively reachable from a package in the first group

i think this should be doable by relying on the existing logic in PackageTree.ToReachMap(). something like this:

func (t PackageTree) TrimHiddenAndUnreachable(main, tests, backprop bool, ignore map[string]bool) PackageTree {
	// while it is probably correct to surface all these params, it is not
	// necessarily the case that we will want to use them all the same way as we
	// currently do with ToReachMap() in cmd/dep.
	rm, pie := t.ToReachMap(main, tests, backprop, 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
}

that's more or less right, but it glaringly it does not incorporate the pie var (the errors map), which is a big question. but i think it's probably a good starting point, as at least we don't have to change ListPackages()' parameters, or parse more than once (though we still have the potentially wasteful disk activity).

[pengux]

What about an --exclude flag?

[sdboyer]

@pengux we generally try to avoid introducing new flags, especially for situations like this. reliance on a flag would necessitate that it be passed for all operations for their correct behavior, which means that whole repos would only work if you use that flag. some would need it, some wouldn't,
and there'd be no way to know a priori. (and, if that flag takes parameters about which dirs to exclude, then it gets even worse - teams would end up writing this down in docs, the reason for it would be lost over time, and exclude parameters would eventually just be cargo-culted)

we can resolve this situation unambiguously and correctly without the need for additional input from the user. we just have to pay a bit of a complexity cost to do it.

[sdboyer]

actually, i'm gonna just tackle this one, having started on the sketched-out implementation above. lot of subtle things here.

[sdboyer]

(closing this, will make new PR)

[darkowlzz]

😆 I spent a few hours understanding, experimenting and implementing, but it's fine. 🤘

[sdboyer]

@darkowlzz sorry about that - happy to discuss it in more detail when my PR is up :)