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: TestLocalImportsHard fails on windows #11217

Closed
alexbrainman opened this issue Jun 15, 2015 · 4 comments
Closed

cmd/go: TestLocalImportsHard fails on windows #11217

alexbrainman opened this issue Jun 15, 2015 · 4 comments

Comments

@alexbrainman
Copy link
Member

From recent build http://build.golang.org/log/05a5e1e4dad8948a11ce786d85222123cbfb10ff

...
--- FAIL: TestLocalImportsHard-2 (0.00s)
go_test.go:152: GetFileAttributesEx ./hello.exe: Access is denied.
FAIL
FAIL cmd/go 37.196s

I can reproduce it here. But I have no idea what the problem is.

Alex

@ianlancetaylor
Copy link
Contributor

My best guess is that

os.RemoveAll("./hello.exe")

is failing with the error "Access is denied".

Perhaps there is some interference from some other test. Can you reproduce it with -test.v?

@ianlancetaylor ianlancetaylor added this to the Go1.5 milestone Jun 15, 2015
@alexbrainman
Copy link
Member Author

It is hit and miss, but I can reproduce the failure sometimes. Given this changes:

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index cdbdfd7..1d78fa2 100644
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -8,6 +8,7 @@ import (
    "bytes"
    "flag"
    "fmt"
+   "io"
    "io/ioutil"
    "os"
    "os/exec"
@@ -16,6 +17,7 @@ import (
    "runtime"
    "strconv"
    "strings"
+   "syscall"
    "testing"
    "time"
 )
@@ -419,6 +421,120 @@ func (tg *testgoData) grepCountBoth(match string) int {
    return tg.doGrepCount(match, &tg.stdout) + tg.doGrepCount(match, &tg.stderr)
 }

+func alexRemove(name string) error {
+   p, e := syscall.UTF16PtrFromString(name)
+   if e != nil {
+       return &os.PathError{"remove", name, e}
+   }
+
+   // Go file interface forces us to know whether
+   // name is a file or directory. Try both.
+   e = syscall.DeleteFile(p)
+   if e == nil {
+       return nil
+   }
+   e1 := syscall.RemoveDirectory(p)
+   if e1 == nil {
+       return nil
+   }
+
+   // Both failed: figure out which error to return.
+   if e1 != e {
+       println("JOHN1", e.Error())
+       println("JOHN2", e1.Error())
+       a, e2 := syscall.GetFileAttributes(p)
+       if e2 != nil {
+           println("JOHN3", e2.Error())
+           e = e2
+       } else {
+           if a&syscall.FILE_ATTRIBUTE_DIRECTORY != 0 {
+               println("JOHN4")
+               e = e1
+           }
+       }
+   }
+   println("JOHN5", e.Error())
+   return &os.PathError{"remove", name, e}
+}
+
+func alexRemoveAll(path string) error {
+   wd, _ := os.Getwd()
+   // Simple case: if Remove works, we're done.
+   println("ALEX1", path, wd)
+   err := alexRemove(path)
+   if err == nil {
+       println("ALEX2")
+       return nil
+   }
+   if os.IsNotExist(err) {
+       println("ALEX3")
+       return nil
+   }
+
+   println("ALEX4", err.Error())
+   // Otherwise, is this a directory we need to recurse into?
+   dir, serr := os.Lstat(path)
+   if serr != nil {
+       if serr, ok := serr.(*os.PathError); ok && (os.IsNotExist(serr.Err) || serr.Err == syscall.ENOTDIR) {
+           return nil
+       }
+       println("ALEX5")
+       return serr
+   }
+   if !dir.IsDir() {
+       // Not a directory; return the error from Remove.
+       println("ALEX6")
+       return err
+   }
+
+   // Directory.
+   fd, err := os.Open(path)
+   if err != nil {
+       if os.IsNotExist(err) {
+           // Race. It was deleted between the Lstat and Open.
+           // Return nil per RemoveAll's docs.
+           return nil
+       }
+       return err
+   }
+
+   // Remove contents & return first error.
+   err = nil
+   for {
+       names, err1 := fd.Readdirnames(100)
+       for _, name := range names {
+           err1 := alexRemoveAll(path + string(os.PathSeparator) + name)
+           if err == nil {
+               err = err1
+           }
+       }
+       if err1 == io.EOF {
+           break
+       }
+       // If Readdirnames returned an error, use it.
+       if err == nil {
+           err = err1
+       }
+       if len(names) == 0 {
+           break
+       }
+   }
+
+   // Close directory, because windows won't remove opened directory.
+   fd.Close()
+
+   // Remove directory.
+   err1 := os.Remove(path)
+   if err1 == nil || os.IsNotExist(err1) {
+       return nil
+   }
+   if err == nil {
+       err = err1
+   }
+   return err
+}
+
+
 // creatingTemp records that the test plans to create a temporary file
 // or directory.  If the file or directory exists already, it will be
 // removed.  When the test completes, the file or directory will be
@@ -433,7 +549,7 @@ func (tg *testgoData) creatingTemp(path string) {
    if tg.wd != "" && !filepath.IsAbs(path) {
        path = filepath.Join(tg.pwd(), path)
    }
-   tg.must(os.RemoveAll(path))
+   tg.must(alexRemoveAll(path))
    tg.temps = append(tg.temps, path)
 }

@@ -526,7 +642,7 @@ func (tg *testgoData) cleanup() {
        }
    }
    for _, path := range tg.temps {
-       tg.check(os.RemoveAll(path))
+       tg.check(alexRemoveAll(path))
    }
    if tg.tempdir != "" {
        tg.check(os.RemoveAll(tg.tempdir))

I get this output:

c:\go\root\src\cmd\go>go test -v -run=LocalImports
=== RUN   TestLocalImportsEasy
ALEX1 ./hello.exe c:\go\root\src\cmd\go
JOHN5 The system cannot find the file specified.
ALEX3
ALEX1 ./hello.exe c:\go\root\src\cmd\go
ALEX2
--- PASS: TestLocalImportsEasy (0.64s)
        go_test.go:250: running testgo [build -o hello.exe testdata\local\easy.go]
=== RUN   TestLocalImportsEasySub
ALEX1 ./hello.exe c:\go\root\src\cmd\go
JOHN5 The system cannot find the file specified.
ALEX3
ALEX1 ./hello.exe c:\go\root\src\cmd\go
ALEX2
--- PASS: TestLocalImportsEasySub (0.67s)
        go_test.go:250: running testgo [build -o hello.exe testdata\local\easysub\main.go]
=== RUN   TestLocalImportsHard
ALEX1 ./hello.exe c:\go\root\src\cmd\go
JOHN5 Access is denied.
ALEX4 remove ./hello.exe: Access is denied.
ALEX5
--- FAIL: TestLocalImportsHard (0.01s)
        go_test.go:154: GetFileAttributesEx ./hello.exe: Access is denied.
=== RUN   TestLocalImportsGoInstallShouldFail
--- PASS: TestLocalImportsGoInstallShouldFail (0.09s)
        go_test.go:250: running testgo [install testdata\local\easy.go]
        go_test.go:269: standard error:
        go_test.go:270: go install: no install location for .go files listed on command line (GOBIN not set)
        go_test.go:289: testgo failed as expected: exit status 1
FAIL
exit status 1
FAIL    cmd/go  6.054s

c:\go\root\src\cmd\go>

It looks to me that TestLocalImportsEasySub removes ./hello.exe at the end of the test (successfully). But then TestLocalImportsHard (following test) gets "Access is denied" while trying to remove ./hello.exe that does not exists now. Very confusing, but I wouldn't be surprised to discover that this is how Windows work in this situation. Removing executable file after it just finished executing has always been a problem on windows. I don't know of a sure way to know when it is safe to delete. We even have small "sleep" at the end of (*os.process).wait for that reason.

I could be missing something here, but I don't see and reasonable resolution here. Maybe we could just have different name for that file for different tests.

Alex

@ianlancetaylor
Copy link
Contributor

It would be easy to change the executable names in these tests. I can do that but I can't test that it fixes the problem on Windows. Let me know if you would like me to try it.

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants