Skip to content
This repository has been archived by the owner on Feb 3, 2018. It is now read-only.

some saftey precautions for windows #142

Closed
wants to merge 1 commit into from
Closed

some saftey precautions for windows #142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func mkNaiveSM(t *testing.T) (*SourceMgr, func()) {

func init() {
_, filename, _, _ := runtime.Caller(1)
bd = path.Dir(filename)
bd = filepath.Dir(filename)
}

func TestSourceManagerInit(t *testing.T) {
Expand All @@ -83,7 +83,7 @@ func TestSourceManagerInit(t *testing.T) {
t.Errorf("Should have gotten CouldNotCreateLockError error type, but got %T", te)
}

if _, err = os.Stat(path.Join(cpath, "sm.lock")); err != nil {
if _, err = os.Stat(filepath.Join(cpath, "sm.lock")); err != nil {
t.Errorf("Global cache lock file not created correctly")
}

Expand All @@ -93,7 +93,7 @@ func TestSourceManagerInit(t *testing.T) {
t.Errorf("removeAll failed: %s", err)
}

if _, err = os.Stat(path.Join(cpath, "sm.lock")); !os.IsNotExist(err) {
if _, err = os.Stat(filepath.Join(cpath, "sm.lock")); !os.IsNotExist(err) {
t.Errorf("Global cache lock file not cleared correctly on Release()")
t.FailNow()
}
Expand Down
3 changes: 1 addition & 2 deletions result.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package gps
import (
"fmt"
"os"
"path"
"path/filepath"
)

Expand Down Expand Up @@ -46,7 +45,7 @@ func WriteDepTree(basedir string, l Lock, sm SourceManager, sv bool) error {

// TODO(sdboyer) parallelize
for _, p := range l.Projects() {
to := path.Join(basedir, string(p.Ident().ProjectRoot))
to := filepath.Join(basedir, string(p.Ident().ProjectRoot))

err := os.MkdirAll(to, 0777)
if err != nil {
Expand Down
14 changes: 7 additions & 7 deletions result_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package gps

import (
"os"
"path"
"path/filepath"
"testing"
)

Expand Down Expand Up @@ -45,19 +45,19 @@ func TestWriteDepTree(t *testing.T) {

r := basicResult

tmp := path.Join(os.TempDir(), "vsolvtest")
tmp := filepath.Join(os.TempDir(), "vsolvtest")
os.RemoveAll(tmp)

sm, clean := mkNaiveSM(t)
defer clean()

// nil lock/result should err immediately
err := WriteDepTree(path.Join(tmp, "export"), nil, sm, true)
err := WriteDepTree(filepath.Join(tmp, "export"), nil, sm, true)
if err == nil {
t.Errorf("Should error if nil lock is passed to WriteDepTree")
}

err = WriteDepTree(path.Join(tmp, "export"), r, sm, true)
err = WriteDepTree(filepath.Join(tmp, "export"), r, sm, true)
if err != nil {
t.Errorf("Unexpected error while creating vendor tree: %s", err)
}
Expand All @@ -70,10 +70,10 @@ func BenchmarkCreateVendorTree(b *testing.B) {
b.SetParallelism(1)

r := basicResult
tmp := path.Join(os.TempDir(), "vsolvtest")
tmp := filepath.Join(os.TempDir(), "vsolvtest")

clean := true
sm, err := NewSourceManager(naiveAnalyzer{}, path.Join(tmp, "cache"))
sm, err := NewSourceManager(naiveAnalyzer{}, filepath.Join(tmp, "cache"))
if err != nil {
b.Errorf("NewSourceManager errored unexpectedly: %q", err)
clean = false
Expand All @@ -91,7 +91,7 @@ func BenchmarkCreateVendorTree(b *testing.B) {
if clean {
b.ResetTimer()
b.StopTimer()
exp := path.Join(tmp, "export")
exp := filepath.Join(tmp, "export")
for i := 0; i < b.N; i++ {
// Order the loop this way to make it easy to disable final cleanup, to
// ease manual inspection
Expand Down
14 changes: 9 additions & 5 deletions util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ func renameWithFallback(src, dest string) error {
return err
}

// Windows cannot use syscall.Rename to rename a directory
Copy link
Owner

@sdboyer sdboyer Jan 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK forgive me for being obtuse about this, but I'm trying to understand the background on this limitation, and I've yet to turn up anything super clear. From what I can gather, Go relies on the MoveFile API in general; for syscall.Rename(), it calls MoveFile(), which I guess should be capable of handling dir renames.

os.Rename, otoh, also ultimately uses the MoveFile API to do its work - which means it should also be capable of dir renames? The only difference is that it uses MoveFileEx(), with the MOVEFILE_REPLACE_EXISTING flag (I believe that's maybe new as of Go 1.6?), which makes behavior generally more similar to POSIX systems than a pure MoveFile() call.

So yeah, I'm confused on this, and pointers to some explanation of the limitation here would be super appreciated.

Copy link
Author

@jessfraz jessfraz Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is from the go 1.8 release notes:

On Unix systems, os.Rename will now return an error when used to rename a directory to an 
existing empty directory. Previously it would fail when renaming to a non-empty directory but 
succeed when renaming to an empty directory. This makes the behavior on Unix correspond to 
that of other systems.

https://tip.golang.org/doc/go1.8 so it will fail on unix in go 1.8 as well

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but that doesn't say it fails when attempting to rename a directory, period - just that it fails if attempting to rename a directory to an existing empty directory. At this point, we haven't established yet whether the target dir exists, or (if it does) whether it's empty.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see what you are saying

if runtime.GOOS == "windows" && fi.IsDir() {
if err := copyDir(src, dest); err != nil {
return err
}
return os.RemoveAll(src)
}

err = os.Rename(src, dest)
if err == nil {
return nil
Expand Down Expand Up @@ -48,11 +56,7 @@ func renameWithFallback(src, dest string) error {
// 0x11 (ERROR_NOT_SAME_DEVICE) is the windows error.
// See https://msdn.microsoft.com/en-us/library/cc231199.aspx
if ok && noerr == 0x11 {
if fi.IsDir() {
cerr = copyDir(src, dest)
} else {
cerr = copyFile(src, dest)
}
cerr = copyFile(src, dest)
}
} else {
return terr
Expand Down