Merge "gosh: convert Shell.Rename to Shell.Move"

Author: asadovsky

gosh/.api

@@ -43,10 +43,10 @@ pkg gosh, method (*Shell) FuncCmd(*Func, ...interface{}) *Cmd
 pkg gosh, method (*Shell) HandleError(error)
 pkg gosh, method (*Shell) MakeTempDir() string
 pkg gosh, method (*Shell) MakeTempFile() *os.File
+pkg gosh, method (*Shell) Move(string, string)
 pkg gosh, method (*Shell) Ok()
 pkg gosh, method (*Shell) Popd()
 pkg gosh, method (*Shell) Pushd(string)
-pkg gosh, method (*Shell) Rename(string, string)
 pkg gosh, method (*Shell) Wait()
 pkg gosh, type Cmd struct
 pkg gosh, type Cmd struct, Args []string

gosh/internal/gosh_example/main.go

@@ -11,6 +11,7 @@ import (
 	"v.io/x/lib/gosh/internal/gosh_example_lib"
 )
 
+// Mirrors TestCmd in shell_test.go.
 func ExampleCmd() {
 	sh := gosh.NewShell(gosh.Opts{})
 	defer sh.Cleanup()
@@ -33,6 +34,7 @@ var (
 	serveFunc = gosh.RegisterFunc("serveFunc", lib.Serve)
 )
 
+// Mirrors TestFuncCmd in shell_test.go.
 func ExampleFuncCmd() {
 	sh := gosh.NewShell(gosh.Opts{})
 	defer sh.Cleanup()

gosh/shell.go

@@ -128,19 +128,17 @@ func (sh *Shell) Wait() {
 	sh.HandleError(sh.wait())
 }
 
-// Rename renames (moves) a file. It's just like os.Rename, but retries once on
-// error.
-func (sh *Shell) Rename(oldpath, newpath string) {
+// Move moves a file.
+func (sh *Shell) Move(oldpath, newpath string) {
 	sh.Ok()
-	sh.HandleError(sh.rename(oldpath, newpath))
+	sh.HandleError(sh.move(oldpath, newpath))
 }
 
 // BuildGoPkg compiles a Go package using the "go build" command and writes the
-// resulting binary to sh.Opts.BinDir unless the -o flag was specified.
+// resulting binary to sh.Opts.BinDir, or to the -o flag location if specified.
 // Returns the absolute path to the binary.
-// Included in Shell for convenience, but could have just as easily been
-// provided as a utility function.
 func (sh *Shell) BuildGoPkg(pkg string, flags ...string) string {
+	// TODO(sadovsky): Convert BuildGoPkg into a utility function.
 	sh.Ok()
 	res, err := sh.buildGoPkg(pkg, flags...)
 	sh.HandleError(err)
@@ -347,34 +345,20 @@ func (sh *Shell) wait() error {
 	return res
 }
 
-func (sh *Shell) rename(oldpath, newpath string) error {
-	if err := os.Rename(oldpath, newpath); err != nil {
-		// Concurrent, same-directory rename operations sometimes fail on certain
-		// filesystems, so we retry once after a random backoff.
-		time.Sleep(time.Duration(rand.Int63n(1000)) * time.Millisecond)
-		if err := os.Rename(oldpath, newpath); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func copyfile(from, to string) error {
+func copyFile(from, to string) error {
 	fi, err := os.Stat(from)
 	if err != nil {
 		return err
 	}
-	mode := fi.Mode()
 	in, err := os.Open(from)
 	if err != nil {
 		return err
 	}
 	defer in.Close()
-	out, err := os.OpenFile(to, os.O_CREATE|os.O_RDWR|os.O_TRUNC, mode)
+	out, err := os.OpenFile(to, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, fi.Mode().Perm())
 	if err != nil {
 		return err
 	}
-	defer out.Close()
 	_, err = io.Copy(out, in)
 	cerr := out.Close()
 	if err != nil {
@@ -383,6 +367,24 @@ func copyfile(from, to string) error {
 	return cerr
 }
 
+func (sh *Shell) move(oldpath, newpath string) error {
+	var err error
+	if err = os.Rename(oldpath, newpath); err != nil {
+		// Concurrent, same-directory rename operations sometimes fail on certain
+		// filesystems, so we retry once after a random backoff.
+		time.Sleep(time.Duration(rand.Int63n(1000)) * time.Millisecond)
+		err = os.Rename(oldpath, newpath)
+	}
+	// If the error was a LinkError, try copying the file over.
+	if _, ok := err.(*os.LinkError); !ok {
+		return err
+	}
+	if err := copyFile(oldpath, newpath); err != nil {
+		return err
+	}
+	return os.Remove(oldpath)
+}
+
 func extractOutputFlag(flags ...string) (string, []string) {
 	for i, f := range flags {
 		if f == "-o" && len(flags) > i {
@@ -408,16 +410,14 @@ func (sh *Shell) buildGoPkg(pkg string, flags ...string) (string, error) {
 	} else if !os.IsNotExist(err) {
 		return "", err
 	}
-	// Build binary to tempBinPath, then move it to binPath.
+	// Build binary to tempBinPath (in a fresh temporary directory), then move it
+	// to binPath.
 	tempDir, err := ioutil.TempDir(sh.Opts.BinDir, "")
 	if err != nil {
 		return "", err
 	}
 	defer os.RemoveAll(tempDir)
 	tempBinPath := filepath.Join(tempDir, path.Base(pkg))
-	if outputFlag != "" {
-		tempBinPath = filepath.Join(tempDir, filepath.Base(binPath))
-	}
 	args := []string{"build", "-o", tempBinPath}
 	args = append(args, flags...)
 	args = append(args, pkg)
@@ -428,10 +428,7 @@ func (sh *Shell) buildGoPkg(pkg string, flags ...string) (string, error) {
 	if err := c.run(); err != nil {
 		return "", err
 	}
-	if err := sh.rename(tempBinPath, binPath); err != nil {
-		if _, ok := err.(*os.LinkError); ok {
-			return "", copyfile(tempBinPath, binPath)
-		}
+	if err := sh.move(tempBinPath, binPath); err != nil {
 		return "", err
 	}
 	return binPath, nil

gosh/shell_test.go

@@ -204,6 +204,7 @@ func TestPushdNoPopdCleanup(t *testing.T) {
 	eq(t, getwdEvalSymlinks(t), startDir)
 }
 
+// Mirrors ExampleCmd in internal/gosh_example/main.go.
 func TestCmd(t *testing.T) {
 	sh := gosh.NewShell(gosh.Opts{Fatalf: makeFatalf(t), Logf: t.Logf})
 	defer sh.Cleanup()
@@ -219,30 +220,14 @@ func TestCmd(t *testing.T) {
 	binPath = sh.BuildGoPkg("v.io/x/lib/gosh/internal/gosh_example_client")
 	c = sh.Cmd(binPath, "-addr="+addr)
 	eq(t, c.Stdout(), "Hello, world!\n")
-
-	// Run client built using -o with absolute and relative names
-	cwd, err := os.Getwd()
-	if err != nil {
-		t.Fatal(err)
-	}
-	absName := filepath.Join(cwd, "x")
-	binPath = sh.BuildGoPkg("v.io/x/lib/gosh/internal/gosh_example_client", "-o", absName)
-	defer os.Remove(absName)
-	c = sh.Cmd(absName, "-addr="+addr)
-	eq(t, c.Stdout(), "Hello, world!\n")
-
-	binPath = sh.BuildGoPkg("v.io/x/lib/gosh/internal/gosh_example_client", "-o", "y")
-	relname := filepath.Join(sh.Opts.BinDir, "y")
-	defer os.Remove(relname)
-	c = sh.Cmd(relname, "-addr="+addr)
-	eq(t, c.Stdout(), "Hello, world!\n")
 }
 
 var (
 	getFunc   = gosh.RegisterFunc("getFunc", lib.Get)
 	serveFunc = gosh.RegisterFunc("serveFunc", lib.Serve)
 )
 
+// Mirrors ExampleFuncCmd in internal/gosh_example/main.go.
 func TestFuncCmd(t *testing.T) {
 	sh := gosh.NewShell(gosh.Opts{Fatalf: makeFatalf(t), Logf: t.Logf})
 	defer sh.Cleanup()
@@ -258,6 +243,32 @@ func TestFuncCmd(t *testing.T) {
 	eq(t, c.Stdout(), "Hello, world!\n")
 }
 
+// Tests that BuildGoPkg works when the -o flag is passed.
+func TestBuildGoPkg(t *testing.T) {
+	sh := gosh.NewShell(gosh.Opts{Fatalf: makeFatalf(t), Logf: t.Logf})
+	defer sh.Cleanup()
+
+	binPath := sh.BuildGoPkg("v.io/x/lib/gosh/internal/gosh_example_server")
+	c := sh.Cmd(binPath)
+	c.Start()
+	addr := c.AwaitVars("addr")["addr"]
+	neq(t, addr, "")
+
+	cwd, err := os.Getwd()
+	ok(t, err)
+	absName := filepath.Join(cwd, "x")
+	binPath = sh.BuildGoPkg("v.io/x/lib/gosh/internal/gosh_example_client", "-o", absName)
+	defer os.Remove(absName)
+	c = sh.Cmd(absName, "-addr="+addr)
+	eq(t, c.Stdout(), "Hello, world!\n")
+
+	relName := filepath.Join(sh.Opts.BinDir, "y")
+	binPath = sh.BuildGoPkg("v.io/x/lib/gosh/internal/gosh_example_client", "-o", "y")
+	defer os.Remove(relName)
+	c = sh.Cmd(relName, "-addr="+addr)
+	eq(t, c.Stdout(), "Hello, world!\n")
+}
+
 var (
 	sendVarsFunc = gosh.RegisterFunc("sendVarsFunc", func(vars map[string]string) {
 		gosh.SendVars(vars)