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.

Sign up for GitHub

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

Jump to bottom

cmd/go: TestShadowingLogic fails when GOROOT path has spaces #14671

Closed
nadiasvertex opened this issue Mar 6, 2016 · 5 comments
Closed

cmd/go: TestShadowingLogic fails when GOROOT path has spaces #14671

nadiasvertex opened this issue Mar 6, 2016 · 5 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@nadiasvertex
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?

1.5 compiling tip + change 18057 (https://go-review.googlesource.com/#/c/18057/20) from Gerrit

  1. What operating system and processor architecture are you using (go env)?
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GO15VENDOREXPERIMENT=
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
  1. What did you do?

Ran all.bat

  1. What did you expect to see?

No errors, ALL TESTS PASSED

  1. What did you see instead?
ok      cmd/fix 0.189s
--- FAIL: TestShadowingLogic (1.27s)
        go_test.go:244: running testgo [list -f ({{.ImportPath}}) ({{.ConflictDir}}) ./testdata/shadow/root1/src/math]
        go_test.go:259: standard output:
        go_test.go:260: (_/C_/Users/Christopher_Nelson/t/src/cmd/go/testdata/shadow/root1/src/math) (C:\Users\Christopher Nelson\t\src\math)

        go_test.go:1884: shadowed math is not shadowed; looking for (_/C_/Users/Christopher Nelson/t/src/cmd/go/testdata/shadow/root1/src/math) (C:\Users\Christopher Nelson\t\src\math)
        go_test.go:244: running testgo [list -f ({{.ImportPath}}) ({{.ConflictDir}}) ./testdata/shadow/root1/src/foo]
        go_test.go:259: standard output:
        go_test.go:260: (foo) ()

        go_test.go:244: running testgo [list -f ({{.ImportPath}}) ({{.ConflictDir}}) ./testdata/shadow/root2/src/foo]
        go_test.go:259: standard output:
        go_test.go:260: (_/C_/Users/Christopher_Nelson/t/src/cmd/go/testdata/shadow/root2/src/foo) (C:\Users\Christopher Nelson\t\src\cmd\go\testdata\shadow\root1\src\foo)

        go_test.go:1897: shadowed foo is not shadowed; looking for (_/C_/Users/Christopher Nelson/t/src/cmd/go/testdata/shadow/root2/src/foo) (C:\Users\Christopher Nelson\t\src\cmd\go\testdata\shadow\root1\src\foo)
        go_test.go:244: running testgo [install ./testdata/shadow/root2/src/foo]
        go_test.go:263: standard error:
        go_test.go:264: go install: no install location for C:\Users\Christopher Nelson\t\src\cmd\go\testdata\shadow\root2\src\foo: hidden by C:\Users\Christopher Nelson\t\src\cmd\go\testdata\shadow\root1\src\foo

        go_test.go:283: testgo failed as expected: exit status 1
FAIL
FAIL    cmd/go  193.519s
@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Mar 7, 2016
@alexbrainman
Copy link
Member

I cannot reproduce your problem with your patch or without. I suppose you have to debug this yourself.

Alex

@nadiasvertex
Copy link
Contributor Author

It looks like spaces are getting replaced with underscores in go list, but the test does not account for this behavior:

The expected output was:

(_/C_/Users/Christopher Nelson/t/src/cmd/go/testdata/shadow/root1/src/math) (C:\Users\Christopher Nelson\t\src\math)

Actual output was:

(_/C_/Users/Christopher_Nelson/t/src/cmd/go/testdata/shadow/root1/src/math) (C:\Users\Christopher Nelson\t\src\math)

It turns out that this behavior in go list is generic:

C:\test with spaces\another test with spaces>"c:\Users\Christopher Nelson\t\bin\go" list -f "({{.ImportPath}}) ({{.ConflictDir}})" ./shadow/root1/src/math (_/C_/test_with_spaces/another_test_with_spaces/shadow/root1/src/math) ()

The test in src/cmd/go/go_test.go:1873 is:

pwdForwardSlash = strings.Replace(pwdForwardSlash, ":", "_", -1)

I added

pwdForwardSlash = strings.Replace(pwdForwardSlash, " ", "_", -1)

and it seems to solve the problem. I don't know whether the test is wrong, or whether go list is wrong (although I suspect it is the test.) Of course, this simple change might not be a complete enough fix. Someone who knows what should happen ought to look at this and decide what to do.

@alexbrainman
Copy link
Member

@nadiasvertex I confirm that TestShadowingLogic is broken if your GOROOT has space in it:

# pwd                                                
/root/hello world/src/cmd/go                                
# go test -v -run=TestShadowingLogic
=== RUN   TestShadowingLogic
--- FAIL: TestShadowingLogic (0.15s)
    go_test.go:244: running testgo [list -f ({{.ImportPath}}) ({{.ConflictDir}}) ./testdata/shadow/root1/src/math]
    go_test.go:259: standard output:
    go_test.go:260: (_/root/hello_world/src/cmd/go/testdata/shadow/root1/src/math) (/root/hello world/src/math)

    go_test.go:1876: shadowed math is not shadowed; looking for (_/root/hello world/src/cmd/go/testdata/shadow/root1/src/math) (/root/hello world/src/math)
    go_test.go:244: running testgo [list -f ({{.ImportPath}}) ({{.ConflictDir}}) ./testdata/shadow/root1/src/foo]
    go_test.go:259: standard output:
    go_test.go:260: (foo) ()

    go_test.go:244: running testgo [list -f ({{.ImportPath}}) ({{.ConflictDir}}) ./testdata/shadow/root2/src/foo]
    go_test.go:259: standard output:
    go_test.go:260: (_/root/hello_world/src/cmd/go/testdata/shadow/root2/src/foo) (/root/hello world/src/cmd/go/testdata/shadow/root1/src/foo)

    go_test.go:1889: shadowed foo is not shadowed; looking for (_/root/hello world/src/cmd/go/testdata/shadow/root2/src/foo) (/root/hello world/src/cmd/go/testdata/shadow/root1/src/foo)
    go_test.go:244: running testgo [install ./testdata/shadow/root2/src/foo]
    go_test.go:263: standard error:
    go_test.go:264: go install: no install location for /root/hello world/src/cmd/go/testdata/shadow/root2/src/foo: hidden by /root/hello world/src/cmd/go/testdata/shadow/root1/src/foo

    go_test.go:283: testgo failed as expected: exit status 1
FAIL
exit status 1
FAIL    cmd/go  4.465s
#

and changing space into underscore fixes the test:

# git diff
diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index 5526aec..056daf7 100644
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -1871,6 +1871,7 @@ func TestShadowingLogic(t *testing.T) {
    // The output will have makeImportValid applies, but we only
    // bother to deal with characters we might reasonably see.
    pwdForwardSlash = strings.Replace(pwdForwardSlash, ":", "_", -1)
+   pwdForwardSlash = strings.Replace(pwdForwardSlash, " ", "_", -1)
    want := "(_" + pwdForwardSlash + "/testdata/shadow/root1/src/math) (" + filepath.Join(runtime.GOROOT(), "src", "math") + ")"
    if strings.TrimSpace(tg.getStdout()) != want {
        t.Error("shadowed math is not shadowed; looking for", want)
# go test -run=TestShadowingLogic
PASS                                    
ok      cmd/go  4.494s                  
#

I think we want to change the test as per your suggestion. Feel free to send a CL.

Alex

@nadiasvertex nadiasvertex changed the title cmd/go: TestShadowingLogic fails on Windows/amd64 cmd/go: TestShadowingLogic fails when GOROOT path has spaces Mar 15, 2016
@nadiasvertex
Copy link
Contributor Author

Fix is here: https://go-review.googlesource.com/20714

@gopherbot
Copy link

CL https://golang.org/cl/20714 mentions this issue.

@golang golang locked and limited conversation to collaborators Mar 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
4 participants
@nadiasvertex @ianlancetaylor @gopherbot @alexbrainman