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

Symlink issue on Windows: A required privilege is not held by the client. #773

Closed
F21 opened this issue Jun 20, 2017 · 7 comments · Fixed by #781
Closed

Symlink issue on Windows: A required privilege is not held by the client. #773

F21 opened this issue Jun 20, 2017 · 7 comments · Fixed by #781
Assignees
Labels

Comments

@F21
Copy link

F21 commented Jun 20, 2017

What version of Go (go version) and dep (git describe --tags) are you using?

Go 1.9-Beta1 and dep c79b048 on Windows 10 64-bit

What dep command did you run?

$ dep init

I am using dep init to import the dependency github.com/grpc-ecosystem/go-grpc-middleware into my project. The dependency has a symlink which breaks dep: https://github.com/grpc-ecosystem/go-grpc-middleware/blob/master/auth/README.md

What did you expect to see?

There should be no error and init should work correctly.

What did you see instead?

safe write of manifest and lock: rename fallback failed: cannot rename C:\Users\user\AppData\Local\Temp\dep845567475\vendor to C:\Work\src\github.com\F21\some_project\vendor: copying directory failed: copying directory failed: copying directory failed: copying directory failed: copying directory failed: copying file failed: failed to create symlink C:\Users\user\AppData\Local\Temp\dep845567475\vendor\github.com\mwitkow\go-grpc-middleware\auth\README.md to DOC.md: symlink DOC.md C:\Work\src\github.com\F21\some_project\vendor\github.com\mwitkow\go-grpc-middleware\auth\README.md: A required privilege is not held by the client.

@F21 F21 changed the title Symlink issue: A required privilege is not held by the client. Symlink issue on Windows: A required privilege is not held by the client. Jun 20, 2017
@ibrasho
Copy link
Collaborator

ibrasho commented Jun 20, 2017

@sdboyer This one is definitely caused by #657.

Creating a symlink on Windows requires additional permissions, so maybe we should fall back to copying?

I would change https://github.com/ibrasho-forks/dep/blob/4d224bd556135823bbc4512aac32a206244c4ab0/internal/fs/fs.go#L244-L250 to:

func copyFile(src, dst string) (err error) {
	sym, err := IsSymlink(src)
	if err != nil {
		return err
	}
	// Skip creating the symlink on Windows and fallback to copying the file content
	// since creating a symlink on Windows requires admin permissions.
	if sym && runtime.GOOS != "windows" {
		return copySymlink(src, dst)
	}

to use fs.copySymlink only when not on Windows.

@F21 Can you try this change locally and see if it resolves the issue?

@ibrasho ibrasho added the bug label Jun 20, 2017
@F21
Copy link
Author

F21 commented Jun 20, 2017

@ibrasho

I replaced https://github.com/golang/dep/blob/master/internal/fs/fs.go#L271-L277 with your snippet and built a copy using go install.

With the path, the symlink does not appear to be a problem, but it throws another error:

safe write of manifest and lock: rename fallback failed: cannot rename C:\Users\user\AppData\Local\Temp\dep659169247\vendor to D:\Work\src\F21\some_project\vendor: copying directory failed: copying directory failed: copying directory failed: copying directory failed: copying directory failed: copying directory failed: copying file failed: open C:\Users\user\AppData\Local\Temp\dep659169247\vendor\github.com\prometheus\procfs\fixtures\26231\exe: The system cannot find the path specified.

@ibrasho
Copy link
Collaborator

ibrasho commented Jun 21, 2017

Does C:\Users\user\AppData\Local\Temp\dep659169247\vendor\github.com\prometheus\procfs\fixtures\26231\exe exist?

@F21
Copy link
Author

F21 commented Jun 21, 2017

I just tried a dep init from scratch again. It seems that after the command terminates, the temp folder C:\Users\user\AppData\Local\Temp\dep283388003 is removed, so there doesn't seem to be a way to inspect it.

@ibrasho
Copy link
Collaborator

ibrasho commented Jun 21, 2017

I just noticed this seems to be similar to the error in #774. I'll assume the fix I proposed resolved the bug with symlinks.

ibrasho added a commit to ibrasho-forks/dep that referenced this issue Jun 21, 2017
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]>
ibrasho added a commit to ibrasho-forks/dep that referenced this issue Jun 21, 2017
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]>
ibrasho added a commit to ibrasho-forks/dep that referenced this issue Jun 21, 2017
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]>
ibrasho added a commit to ibrasho-forks/dep that referenced this issue Jun 21, 2017
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]>
@sdboyer
Copy link
Member

sdboyer commented Jun 27, 2017

...ugh. Falling back to copying as a general solution here makes me very uncomfortable.

Will discuss more on the PR.

@ibrasho
Copy link
Collaborator

ibrasho commented Jun 27, 2017

I don't like it either 😞. I would prefer to come up with a solution that adapts to the current constraints. So if the user is privileged, clone the symlinks files. Otherwise, fall back to copying the symlinks.

I think a good starting point might be to replicate git behavior since that what go get currently relies on (at least in respect to how symlinks are persisted on the fs).

I'm skipping how other VCS systems deal with this until git is working properly, and then we can expand the same approach. I've not researched how hg or bzr handle symlinks on Windows yet, but I hope it's not far off from git.

ibrasho added a commit to ibrasho-forks/dep that referenced this issue Jul 21, 2017
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 added a commit to ibrasho-forks/dep that referenced this issue Jul 21, 2017
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 added a commit to ibrasho-forks/dep that referenced this issue Jul 23, 2017
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 ibrasho self-assigned this Jul 29, 2017
ibrasho added a commit to ibrasho-forks/dep that referenced this issue Aug 9, 2017
This change updates TestCopyFileSymlink tests and renames copySymlink to
cloneSymlink to clarify the intention.

Fixes golang#773

Signed-off-by: Ibrahim AshShohail <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants