internal/fs: don't clone symlinks on windows by ibrasho · Pull Request #781 · golang/dep

ibrasho · 2017-06-21T01:51:02Z

What does this do / why do we need it?

fs.copyFile calls fs.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 fs.copySymlink to fs.cloneSymlink to clarify the intent and skips calling it when on Windows to fall back to copying the file content instead.

What should your reviewer look out for in this PR?

Generic review.

Which issue(s) does this PR fix?

Fixes #773

sdboyer · 2017-06-27T04:36:12Z

-	if err = os.Symlink(srcPath, symlinkPath); err != nil {
-		t.Fatalf("could not create symlink: %v", err)
-	}
+		if runtime.GOOS == "window" {


"windows"

sdboyer · 2017-06-27T04:57:16Z

Transforming symlinks into their referent directory and copying them is a pretty drastic change. For some of the uses of this func, it's likely fine. Others might present complications, though.

Could we please enumerate each of the places this logic will be triggered (so, calls from outside the fs package that end up in copyFile()), so that we can develop a clear sense of what all may be impacted by this?

ibrasho · 2017-06-27T09:43:04Z

Just to clarify, we don't transform the symlink into anything.
We merely copy the symlink file content instead of creating a new symlink on Windows. (Which is pretty much how Git for Windows behave when core.symlinks is false). This results in a text file containing the target of the symlink (if it was a POSIX symlink).

If false, symbolic links are checked out as small plain files that contain the link text. - core.symlinks documentation

This renders the symlink useless on Windows. So if a dependency relies on symlinks functioning, something will most likely break and that is the part that really worries me.

ibrasho · 2017-06-27T12:15:13Z

For the record,fs.copyFile can be invoked from outside internal/fs though either fs.RenameWithFallback or fs.CopyDir.

dep.*SafeWriter.Write: uses fs.RenameWithFallback to preserve vendor/.git/, replace Gopkg.toml and Gopkg.lock, and bring in the new vendor/.
dep.PruneProject uses fs.RenameWithFallback to backup the current vendor/ and bring in the new vendor/.
dep.BackupVendor: uses fs.CopyDir to backup vendor/.
gps.*baseVCSSource.exportRevisionTo and gps.*gitSource.exportRevisionTo: use fs.CopyDir to copy the repository at the specified revision to the temporary vendor/ directory.

sdboyer · 2017-07-18T04:20:35Z

+	// Creating symlinks on Windows require an additional permission regular
+	// users aren't granted usually. So we skip cloning the symlink nd copy the
+	// file content as a fall back instead.
+	if sym && runtime.GOOS != "windows" {


what if we were to try it even on windows, and if we encounter the particular error, then fall back to cloning?

Should work.

I'm only worried about not having a clear path of execution. The more conditionals paths we have the harder it's to reproduce output or debug strange behavior because the input list grows exponentially.

of course - more if branches, more cyclomatic complexity. but the side effects here worry me enough that i think it's merited 😄

sdboyer

hmm, the review looked good, but apparently we have test failures 😢

ibrasho · 2017-07-23T20:52:27Z

Fixed tests.

This change updates TestCopyFileSymlink tests and renames copySymlink to cloneSymlink to clarify the intention. Fixes golang#773 Signed-off-by: Ibrahim AshShohail <[email protected]>

sdboyer · 2017-07-26T09:46:09Z

 	} else if sym {
-		return cloneSymlink(src, dst)
+		if err := cloneSymlink(src, dst); err != nil {
+			if runtime.GOOS == "windows" && strings.HasSuffix(err.Error(), "A required privilege is not held by the client.") {


this isn't i18n-friendly. not a blocker, necessarily, but is there any way we could sniff the system actual error type here? obviously that means syscall, so this'd have to be a in a windows-tagged file.

Good catch. Didn't think of that. Updating now.

I think I solved it in an OS-independent way. Let me know if a windows-tagged file is still needed.

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

sdboyer

yeah, i think we're good here. thanks for your patience, as always! 🎉

googlebot added the cla: yes label Jun 21, 2017

ibrasho force-pushed the fix-symlink-cloning-on-windows branch 2 times, most recently from 1ab4923 to 34d3314 Compare June 21, 2017 20:12

sdboyer suggested changes Jun 27, 2017

View reviewed changes

sdboyer reviewed Jul 18, 2017

View reviewed changes

ibrasho force-pushed the fix-symlink-cloning-on-windows branch 2 times, most recently from db67dbf to 80ad660 Compare July 21, 2017 02:39

sdboyer approved these changes Jul 23, 2017

View reviewed changes

sdboyer suggested changes Jul 23, 2017

View reviewed changes

ibrasho force-pushed the fix-symlink-cloning-on-windows branch from d4803f7 to 2ea710d Compare July 23, 2017 20:20

internal/fs: update TestCopyFileSymlink and rename copySymlink

55095d2

This change updates TestCopyFileSymlink tests and renames copySymlink to cloneSymlink to clarify the intention. Fixes golang#773 Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the fix-symlink-cloning-on-windows branch 4 times, most recently from 545881f to 90c2a36 Compare July 24, 2017 06:50

sdboyer reviewed Jul 26, 2017

View reviewed changes

internal/fs: clone symlinks on Windows and fall back to file copying

ced76d2

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

ibrasho force-pushed the fix-symlink-cloning-on-windows branch from fb26cc3 to ced76d2 Compare July 28, 2017 08:34

sdboyer approved these changes Aug 1, 2017

View reviewed changes

sdboyer merged commit 44a454b into golang:master Aug 1, 2017

ibrasho deleted the fix-symlink-cloning-on-windows branch November 29, 2017 06:18

Conversation

ibrasho commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

Uh oh!

sdboyer Jun 27, 2017

Choose a reason for hiding this comment

Uh oh!

sdboyer commented Jun 27, 2017

Uh oh!

ibrasho commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibrasho commented Jun 27, 2017

Uh oh!

sdboyer Jul 18, 2017

Choose a reason for hiding this comment

Uh oh!

ibrasho Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

sdboyer Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

ibrasho Jul 22, 2017

Choose a reason for hiding this comment

Uh oh!

sdboyer left a comment

Choose a reason for hiding this comment

Uh oh!

ibrasho commented Jul 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdboyer Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

ibrasho Jul 28, 2017

Choose a reason for hiding this comment

Uh oh!

ibrasho Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdboyer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ibrasho commented Jun 21, 2017 •

edited

Loading

ibrasho commented Jun 27, 2017 •

edited

Loading

ibrasho commented Jul 23, 2017 •

edited

Loading

ibrasho Jul 28, 2017 •

edited

Loading