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

cmd/go: lowercase package directory comparison is wrong #25878

Closed
rsc opened this issue Jun 13, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@rsc
Copy link
Contributor

commented Jun 13, 2018

https://golang.org/cl/109235 changed the meaning of directory patterns to do case-insensitive matches on Windows. This is not correct behavior. Maybe it is slightly more correct than not, but it's still wrong. I believe that Windows can have case-sensitive file systems, and certainly there are other systems with case-insensitive file systems (most Macs, for example). The general solution can't be to use strings.EqualFold based on runtime.GOOS.

I suspect the answer is to do something like

if p.Dir == dir {
	return true
}
if !strings.EqualFold(p.Dir, dir) { // easy out
	return false
}
info1, err1 := os.Stat(p.Dir)
info2, err2 := os.Stat(dir)
return err1 == nil && err2 == nil && os.SameFile(info1, info2)

but I am not sure whether os.SameFile works on directories on Windows.

Does anyone know?

@rsc rsc added this to the Go1.11 milestone Jun 13, 2018

@rsc rsc added the release-blocker label Jun 13, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2018

CL in question is https://golang.org/cl/109235 .

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2018

testDirLinks in os/os_windows_test.go certainly implies that os.SameFile works for directories on WIndows.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

/cc @alexbrainman @bradfitz (from the CL)

@gopherbot

This comment has been minimized.

Copy link

commented Jun 13, 2018

Change https://golang.org/cl/118738 mentions this issue: cmd/go: don't assume that GOOS="windows" implies case insensitive filesystem

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

send reply to the issue:

https://golang.org/cl/109235 changed the meaning of directory patterns to do case-insensitive matches on Windows. This is not correct behavior.

Why CL 109235 is not correct behavior? Do you know of a setup where TestCDAndGOPATHAreDifferent fails?

I did some experiment at home. I have Linux computer that runs Samba software, that allows some Linux directories accessible to Windows computers on the network. so I created directory with 2 sub-directories having same name, just different case, and have different contents. Just like:

$ find
.
./test
./test/small
./TEST
./TEST/LARGE
$

And then I mounted that directory on my Windows computer. That is what I see:

U:\>dir /b u:\z\
TEST
test

U:\>dir /b u:\z\TEST\
small

U:\>dir /b u:\z\test
small

U:\>

cmd.exe uses same API as Go programs do, so Go programs will see exactly the same thing.

Something https://blogs.msdn.microsoft.com/commandline/2018/02/28/per-directory-case-sensitivity-and-wsl/ I have found while googling for this issue. Perhaps it is related.

Alex

@andybons

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

@rsc can you take a look at the CL that @bradfitz sent out and see if his solution is the one you're looking for? There seems to be disagreement on the right solution here.

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

The original CL 109235 was addressing a real bug (in its commit message) but the problem is not that the pattern match algorithm was wrong. The problem is that the pattern going into the match was wrong. Bad inputs not bad outputs. I'll take a look at an alternate fix for that real issue.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 10, 2018

Change https://golang.org/cl/129059 mentions this issue: cmd/go: fix -gcflags, -ldflags not applying to current directory

@gopherbot gopherbot closed this in 2ce6da0 Aug 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.