-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Win10 symlink require privilege #175
Fix: Win10 symlink require privilege #175
Conversation
Thanks |
gobrew.go
Outdated
return "None" | ||
// https://github.com/golang/go/issues/63703 | ||
var fp string | ||
if runtime.GOOS == "windows" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a file, gobrew_windows.go
, which houses the code used only in windows.
It seems that you need to create a separate function like evalSymlinks
, for which gobrew_unix.go
will have a normal implementation, and in gobrew_windows.go
will be specific to windows.
And the code will be easier to read.
gobrew.go
Outdated
if runtime.GOOS == "windows" { | ||
cmd := fmt.Sprintf( | ||
"Get-Item -Path %s | Select-Object -ExpandProperty Target", | ||
gb.currentBinDir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need a little more time to check what is offered in
golang/go#63703
I don't quite understand why a call is made specifically ps in this case
helpers.go
Outdated
@@ -349,12 +350,26 @@ func (gb *GoBrew) downloadAndExtract(version string) { | |||
func (gb *GoBrew) changeSymblinkGoBin(version string) { | |||
goBinDst := filepath.Join(gb.versionsDir, version, "/go/bin") | |||
_ = os.RemoveAll(gb.currentBinDir) | |||
if runtime.GOOS == "windows" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also necessary to remove the checks on the operating system and add a function like symlink
, which we will implement differently depending on the type of operating system in our file.
Again, because I don't have windows at hand, I need a little more time.
And the question is, why in one case do you call ps, and here you call cmd? Why such a spread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my knowledge, there's no easy way to eval a symlink with cmd
, and I use cmd
because it doesn't require privilege to create symlinks.
I'll remove the GOOs check, I've never coded cross-platform code before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: Wrong Unix Fix: Wrong Unix
@kevincobain2000 has become much better now, but I still don't like the idea of calling third-party programs from the code. In addition, Windows has a lot of shell, that is, many launch options and each may have its own problems. In my opinion, if we have a problem with creating a simlink, we can simply copy the directory from a certain version to the For example, when it is necessary to switch the version:
If it is not available, we download from the Internet accordingly. Here you still need to think about how to solve the problem with copying the What do you think? |
Hi @juev Your suggestion on version switch is good, it can be done natively - all in GO. Having said all that, if calling a separate |
If you are ok then I am also ok for this merge. It's not like it is changing anything for unix based os. |
Let me know if you are ok with merge @juev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I disagree with the implementation. It could have been implemented much more correctly.
It's not clear if everything works in sl, why use cmd or ps?
But since we hardly change the usual functionality, so be it.
Thanks @juev will merge it after dinner |
Problem
Symlink on Windows 10 Powershell required privilege, so we need to use
cmd
to create a symlink withmklink
. See issue Bug: Windows 10 Symlink required privilege #174filepath.EvalSymlinks
doesn't work correctly on Windows. A fix is currently being proposed: os,path/filepath: make os.Readlink and filepath.EvalSymlinks on Windows (mostly) only evaluate symlinks golang/go#63703