Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/copyright: TestToolsCopyright is falsely reporting success #68306

Closed
millerresearch opened this issue Jul 4, 2024 · 1 comment
Closed
Assignees
Labels
Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@millerresearch
Copy link
Contributor

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.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 4, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jul 4, 2024
@millerresearch millerresearch self-assigned this Jul 4, 2024
@gopherbot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

2 participants