internal/fs: don't clone symlinks on windows

copyFile calls copySymlink on Windows which fails if the user doesn't
have the required permission. This is a very common case since symlinks
are used heavily on Windows.

This change renames copySymlink to cloneSymlink to clarify the intention
and skips calling it when on Windows to fallback to copy the file
content instead.

Fixes golang#773

Signed-off-by: Ibrahim AshShohail <[email protected]>

Author: ibrasho

internal/fs/fs.go

@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"unicode"
 
@@ -269,12 +270,15 @@ func CopyDir(src, dst string) error {
 // of the source file. The file mode will be copied from the source and
 // the copied data is synced/flushed to stable storage.
 func copyFile(src, dst string) (err error) {
-	if sym, err := IsSymlink(src); err != nil {
-		return err
-	} else if sym {
-		err := copySymlink(src, dst)
+	sym, err := IsSymlink(src)
+	if err != nil {
 		return err
 	}
+	// Skip cloning the symlink on Windows and fallback to copying the file content
+	// since creating a symlink on Windows requires additional permissions.
+	if sym && runtime.GOOS != "windows" {
+		return cloneSymlink(src, dst)
+	}
 
 	in, err := os.Open(src)
 	if err != nil {
@@ -314,17 +318,17 @@ func copyFile(src, dst string) (err error) {
 	return
 }
 
-// copySymlink will resolve the src symlink and create a new symlink in dst.
-// If src is a relative symlink, dst will also be a relative symlink.
-func copySymlink(src, dst string) error {
-	resolved, err := os.Readlink(src)
+// cloneSymlink will create a new symlink that points to the resolved path of sl.
+// If sl is a relative symlink, dst will also be a relative symlink.
+func cloneSymlink(sl, dst string) error {
+	resolved, err := os.Readlink(sl)
 	if err != nil {
 		return errors.Wrap(err, "failed to resolve symlink")
 	}
 
 	err = os.Symlink(resolved, dst)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create symlink %s to %s", src, resolved)
+		return errors.Wrapf(err, "failed to create symlink %s to %s", dst, resolved)
 	}
 
 	return nil

internal/fs/fs_test.go

@@ -499,71 +499,32 @@ func TestCopyFile(t *testing.T) {
 }
 
 func TestCopyFileSymlink(t *testing.T) {
-	dir, err := ioutil.TempDir("", "dep")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(dir)
-
-	srcPath := filepath.Join(dir, "src")
-	symlinkPath := filepath.Join(dir, "symlink")
-	dstPath := filepath.Join(dir, "dst")
-
-	srcf, err := os.Create(srcPath)
-	if err != nil {
-		t.Fatal(err)
-	}
-	srcf.Close()
-
-	if err = os.Symlink(srcPath, symlinkPath); err != nil {
-		t.Fatalf("could not create symlink: %s", err)
-	}
-
-	if err = copyFile(symlinkPath, dstPath); err != nil {
-		t.Fatalf("failed to copy symlink: %s", err)
-	}
-
-	resolvedPath, err := os.Readlink(dstPath)
-	if err != nil {
-		t.Fatalf("could not resolve symlink: %s", err)
-	}
-
-	if resolvedPath != srcPath {
-		t.Fatalf("resolved path is incorrect. expected %s, got %s", srcPath, resolvedPath)
-	}
-}
-
-func TestCopyFileSymlinkToDirectory(t *testing.T) {
-	dir, err := ioutil.TempDir("", "dep")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(dir)
-
-	srcPath := filepath.Join(dir, "src")
-	symlinkPath := filepath.Join(dir, "symlink")
-	dstPath := filepath.Join(dir, "dst")
+	h := test.NewHelper(t)
+	defer h.Cleanup()
+	h.TempDir(".")
 
-	err = os.MkdirAll(srcPath, 0777)
-	if err != nil {
-		t.Fatal(err)
+	testcases := map[string]string{
+		filepath.Join(".", "testdata", "symlinks", "file-symlink"):    filepath.Join(h.Path("."), "dst-file"),
+		filepath.Join(".", "testdata", "symlinks", "dir-symlink"):     filepath.Join(h.Path("."), "dst-dir"),
+		filepath.Join(".", "testdata", "symlinks", "invalid-symlink"): filepath.Join(h.Path("."), "invalid-dir"),
 	}
 
-	if err = os.Symlink(srcPath, symlinkPath); err != nil {
-		t.Fatalf("could not create symlink: %v", err)
-	}
+	for symlink, dst := range testcases {
+		if err := copyFile(symlink, dst); err != nil {
+			t.Fatalf("failed to copy symlink: %s", err)
+		}
 
-	if err = copyFile(symlinkPath, dstPath); err != nil {
-		t.Fatalf("failed to copy symlink: %s", err)
-	}
+		want, err := os.Readlink(symlink)
+		h.Must(err)
 
-	resolvedPath, err := os.Readlink(dstPath)
-	if err != nil {
-		t.Fatalf("could not resolve symlink: %s", err)
-	}
+		got, err := os.Readlink(dst)
+		if err != nil {
+			t.Fatalf("could not resolve symlink: %s", err)
+		}
 
-	if resolvedPath != srcPath {
-		t.Fatalf("resolved path is incorrect. expected %s, got %s", srcPath, resolvedPath)
+		if want != got {
+			t.Fatalf("resolved path is incorrect. expected %s, got %s", want, got)
+		}
 	}
 }
 

internal/fs/testdata/symlinks/dir-symlink

@@ -0,0 +1 @@
+../../testdata

internal/fs/testdata/symlinks/file-symlink

@@ -0,0 +1 @@
+../test.file

internal/fs/testdata/symlinks/invalid-symlink

@@ -0,0 +1 @@
+/non/existing/file