Fix ListPackages to not skip "."-prefixed dirs

[ascandella]

Follow up to #551 after getting some clarity on golang/go#20337

We need ListPackages to descend into .-prefixed directories, even though the go tool ignores them they are still valid import paths, and are in use in the wild.

Filing this for reference -- I will fix this.

[ascandella]

Current code, from pkgtree.go:

    // Skip dirs that are known to hold non-local/dependency code.
    //
    // We don't skip _*, or testdata dirs because, while it may be poor
    // form, importing them is not a compilation error.
    switch fi.Name() {
    case "vendor", "Godeps":
      return filepath.SkipDir
    }
    // We do skip dot-dirs, though, because it's such a ubiquitous standard
    // that they not be visited by normal commands, and because things get
    // really weird if we don't.
    if strings.HasPrefix(fi.Name(), ".") {
      return filepath.SkipDir
    }

Not sure what "really weird" means -- I guess I will find out :) Unless somebody here has insight.

[davecheney]

I'm sorry, I think you have misinterpreted my reply, please see golang/go#20337 (comment)

…

On Fri, May 12, 2017 at 1:40 PM, Aiden Scandella ***@***.***> wrote: Follow up to #551 <#551> after getting some clarity on golang/go#20337 <golang/go#20337> We need ListPackages to descend into .-prefixed directories, even though the go tool ignores them they are still valid import paths, and are in use in the wild. Filing this for reference -- I will fix this. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#564>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAcA4I7Or4MjkXz0LJpa3lqBe50pUOFks5r49SygaJpZM4NYzmS> .

[ascandella]

Ah, GitHub tag :) Thanks for the followup. Let's consolidate on the golang/go issue, or in Slack. Thanks again for your help!

[ascandella]

Looks like this is also the wrong approach. Going to close for now and play around to see what the right solution is. Also submitted https://go-review.googlesource.com/#/c/43299/ since I think the documentation could use some clarification.

[sdboyer]

Picking up from golang/go#20337 (comment) :

It sounds like dep is conflating two different operations

• for a tree of directories; figure out which contain valid Go packages
• for a directory of source files; figure out which ones are are in scope
  according to build rules.

It's actually (intended to be) the opposite - we're trying very diligently to keep these two exact concerns separate.

@sectioneight is entirely correct that

Ok. In that case dep will fail if a dependency has a package named ".foo" since it doesn't see the source code.

The segment of the code he's looking at in ListPackages() is intended to build a representation of the filesystem containing everything that would be possible to reference via an import declaration. If it's not picked up by that function, and something references it later, then it will be inaccurately reported as missing.

The rules that are consistent with how e.g. go list behaves are applied by another operation, used in different situations.

The operant thing is this:

Not sure what "really weird" means -- I guess I will find out :) Unless somebody here has insight.

The dot-led exclusion is probably wrong, and should be removed. IIRC, it's in there because, a year ago when this whole subsystem was still nascent, I fell into a strange loop while trying to solve k8s, got frustrated, and added the rule 😀

That said, when we remove it, we need to add specific exclusions for .git, .bzr, .hg, and .svn dirs. There's no reason to penalize literally every single fs walk the tool makes for someone who wanted to be cute and put a go package inside a place that their VCS couldn't possibly record it.

[ascandella]

To help others, I created a repro case here:

https://gist.github.com/sectioneight/18a9fa2a5ce1961ca4a91f01775e95ea

Using: https://github.com/sectioneight/dot-import-example

When I remove the .dot import and just reference _underscore things work fine, so this issue is limited to .-prefixed paths:

https://gist.github.com/sectioneight/d8204487f39cd0a786a82c1b9c6c04e9

Note that the code does compile:

scratch - master! ❯ go build
scratch - master! ❯ cat main.go
package main

import (
        "fmt"

        "github.com/sectioneight/dot-import-example/.dot"
        "github.com/sectioneight/dot-import-example/_underscore"
)

func main() {
        fmt.Println(dot.Dot)
        fmt.Println(underscore.Underscore)
}
scratch - master! ❯ ./scratch
.
_

[ascandella]

Also, I can confirm that the following patch fixes the issue for my repro case. I don't have a full testbed set up locally, so I'm not ready to submit a PR yet until I can figure out how to write tests:

dep - master! ❯ git --no-pager diff --cached
diff --git c/internal/gps/pkgtree/pkgtree.go i/internal/gps/pkgtree/pkgtree.go
index a6b4527..1b2e94f 100644
--- c/internal/gps/pkgtree/pkgtree.go
+++ i/internal/gps/pkgtree/pkgtree.go
@@ -28,6 +28,15 @@ type Package struct {
        TestImports []string // Imports from all go test files (in go/build parlance: both TestImports and XTestImports)
 }

+// svnRoots is a set of directories we should not descend into in ListPackages when
+// searching for Go packages
+var svnRoots = map[string]struct{}{
+       ".git": struct{}{},
+       ".bzr": struct{}{},
+       ".svn": struct{}{},
+       ".hg":  struct{}{},
+}
+
 // ListPackages reports Go package information about all directories in the tree
 // at or below the provided fileRoot.
 //
@@ -78,10 +87,13 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) {
                case "vendor", "Godeps":
                        return filepath.SkipDir
                }
-               // We do skip dot-dirs, though, because it's such a ubiquitous standard
-               // that they not be visited by normal commands, and because things get
-               // really weird if we don't.
-               if strings.HasPrefix(fi.Name(), ".") {
+
+               // Skip dirs that are known to be VCS roots.
+               //
+               // Note that there are some pathological edge cases this doesn't cover,
+               // such as a user using Git for version control, but having a package
+               // named "svn" in a directory named ".svn".
+               if _, ok := svnRoots[fi.Name()]; ok {
                        return filepath.SkipDir
                }