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.
Go version
gotip
Output of
go env
in your module/workspace:What did you do?
In
x/tools,
rango test -json -short ./...
.What did you see happen?
On Plan 9 builder,
TestToolsCopyright
fails consistently: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 afilepath.WalkDir
of the source tree rooted at..
, looking for.go
files missing the required copyright text. But the walk function begins with this: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. Thego doc
forDirEntry.Name
says: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:".."
and"."
filePath.Abs
to thedir
argument before callingfilepath.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.