Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

fix error on AppVeyor #157

Merged
merged 1 commit into from
Jan 26, 2017
Merged

fix error on AppVeyor #157

merged 1 commit into from
Jan 26, 2017

Conversation

mattn
Copy link
Member

@mattn mattn commented Jan 25, 2017

As far as I can see, It seeks git.exe call fork(2) to do something about ls-remote or git-remote-http on Windows. But it often quit without waiting the child process. So the git.exe hold the handle of directory which should be removed. This occured while removing temporary directory in tg.cleanup. I don't know how to fix this git process. For example:

diff --git a/dep_test.go b/dep_test.go
index ecec35f..9f518be 100644
--- a/dep_test.go
+++ b/dep_test.go
@@ -16,6 +16,7 @@ import (
 	"runtime"
 	"strings"
 	"testing"
+	"time"
 )
 
 var (
@@ -483,8 +484,16 @@ func (tg *testgoData) cleanup() {
 		tg.check(os.RemoveAll(path))
 	}
 	if tg.tempdir != "" {
-		exec.Command(`taskkill`, `/F`, `/IM`, `git.exe`).Run()
-		tg.check(os.RemoveAll(tg.tempdir))
+		if runtime.GOOS == "windows" {
+			for {
+				if os.RemoveAll(tg.tempdir) == nil {
+					break
+				}
+				time.Sleep(time.Second)
+			}
+		} else {
+			tg.check(os.RemoveAll(tg.tempdir))
+		}
 	}
 }

Waiting something (maybe git.exe) fixes this issue. But no reason to wait the bad child. Then, terminate all git.exe in tg.This patch should work.

@mattn
Copy link
Member Author

mattn commented Jan 25, 2017

@zchee
Copy link
Contributor

zchee commented Jan 25, 2017

@mattn Thanks fixed, and I totally understand why AppVeyor failed.
It seems good to merge my #156 after merging this PR.

@sdboyer
Copy link
Member

sdboyer commented Jan 25, 2017

oh, awesome! i think i've run into similar issues in the past in gps. there isn't the same kind of testing harness in gps as dep has (though maybe there should be), so it's not as central of a fix as this. still, though, it would be great to adapt this to over there.

i'm going to wait on this until we can get those travis failures fixed, though. hopefully #156 will do that for us.

@mattn
Copy link
Member Author

mattn commented Jan 25, 2017

This should treat parallel tests. So please wait.

@mattn
Copy link
Member Author

mattn commented Jan 25, 2017

still bad

@jessfraz
Copy link
Contributor

ya this was super racey before it would fail like 40% of the time but i'd just rebuild, thanks for putting the time into fixing this :)

@sdboyer
Copy link
Member

sdboyer commented Jan 26, 2017

i don't know why travis can't get its act together 😡

@sdboyer sdboyer mentioned this pull request Jan 26, 2017
terminate all git.exe and git-remote-https.exe

aaa
@sdboyer
Copy link
Member

sdboyer commented Jan 26, 2017

Whatever, it works on appveyor - that's the only place where a change actually occurred.

@sdboyer sdboyer merged commit b07f69b into golang:master Jan 26, 2017
@mattn
Copy link
Member Author

mattn commented Jan 26, 2017

Thank you. I just figure out what process hold the handle of the directory. handle command from sysinternal is very useful to do.

https://technet.microsoft.com/en-us/sysinternals

------------------------------------------------------------------------------
git.exe pid: 14016 XXXXXXXXXXXX
   68: Section       \BaseNamedObjects\msys-2.0S5-dd50a72ab4668b33\shared.5
   6C: Section       \BaseNamedObjects\msys-2.0S5-dd50a72ab4668b33\S-1-5-21-1292428093-1004336348-682003330-12675.1
   98: File  (---)   C:\Users\mattn\AppData\Local\Temp\gotest538495084\src\github.com\golang\notexist
   A8: Section       \BaseNamedObjects\msys-2.0S5-dd50a72ab4668b33\cygpid.14016

@mattn mattn deleted the fix-appveyor branch January 26, 2017 02:20
krisnova pushed a commit to krisnova/dep that referenced this pull request Apr 21, 2017
Read the destination of named symbolic link
krisnova pushed a commit to krisnova/dep that referenced this pull request Apr 21, 2017
This reverts commit 67b7104, reversing
changes made to db8fd63.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants