Golang x/tools/copyright: TestToolsCopyright is falsely reporting success

Go version

gotip

Output of go env in your module/workspace:

GOARCH=arm
GOBIN=
GOCACHE=/home/swarming/.swarming/w/ir/x/w/gocache
GOHOSTARCH=amd64
GOHOSTOS=linux
GOMAXPROCS=4
GOOS=plan9
GOPATH=/home/swarming/.swarming/w/ir/x/w/gopath
GOPLSCACHE=/home/swarming/.swarming/w/ir/x/w/goplscache
GOROOT=
GOROOT_BOOTSTRAP=/home/swarming/.swarming/w/ir/cache/tools/go_bootstrap
GOTOOLCHAIN=local
GO_BUILDER_NAME=x_tools-gotip-plan9-arm

What did you do?

In x/tools, ran go test -json -short ./....

What did you see happen?

On Plan 9 builder, TestToolsCopyright fails consistently:

--- FAIL: TestToolsCopyright (48.29s)
    copyright_test.go:18: The following files are missing copyright notices:
        ../gopls/doc/assets/assets.go
FAIL
FAIL    golang.org/x/tools/copyright    50.371s

On other platforms, this test is succeeding ... but it shouldn't.

What did you expect to see?

Expected to see the test failing on all platforms, because the file ../gopls/doc/assets/assets.go indeed does not contain the required copyright text (it's an empty package). However, the tree walk searching for copyright text is actually being bypassed except on Plan 9.

The test calls checkCopyright("..") to do a filepath.WalkDir of the source tree rooted at .., looking for .go files missing the required copyright text. But the walk function begins with this:

                if d.IsDir() {
                        // Skip directories like ".git".
                        if strings.HasPrefix(d.Name(), ".") {
                                return filepath.SkipDir
                        }

This bypass condition is being triggered unintentionally because the tree being walked starts at .., so the test is ineffectual.

Why is the behaviour different on Plan 9? Because the bypass condition is looking at the name of the directory as fetched from its DirEntry, not at the pathname passed to the walk. The go doc for DirEntry.Name says:

    // Name returns the name of the file (or subdirectory) described by the entry.

But on UNIX-family operating systems, "the" name of a file is ambiguous: names are attached not to files but to (possibly multiple) links from directories to files. On Plan 9, there are no links, and a file's (unique) name is attached to (the equivalent of) its inode. So on Plan 9, the DirEntry.Name function returns the "actual" name of the directory, not .. which is arguably not a name but a relationship.

Philosophical discussions about filenames aside, I would suggest two possible ways to make checkCopyright() more reliable: - narrow the bypass condition above to make an explicit exception for ".." and "." - apply filePath.Abs to the dir argument before calling filepath.WalkDir()

And the file ../gopls/doc/assets/assets.go should have a copyright notice added, to prevent the test from failing once it's fixed.

Comment From: gopherbot

Change https://go.dev/cl/596736 mentions this issue: copyright: don't skip directories "." or ".." in checkCopyright