Skip to content
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

Merged

Conversation

vunhatchuong
Copy link
Contributor

Problem

  1. Symlink on Windows 10 Powershell required privilege, so we need to use cmd to create a symlink with mklink. See issue Bug: Windows 10 Symlink required privilege #174

  2. filepath.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

@kevincobain2000
Copy link
Owner

Thanks
Please wait for @juev's review

gobrew.go Outdated
return "None"
// https://github.com/golang/go/issues/63703
var fp string
if runtime.GOOS == "windows" {
Copy link
Collaborator

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,
Copy link
Collaborator

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" {
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Fix: Wrong Unix

Fix: Wrong Unix
@juev
Copy link
Collaborator

juev commented Feb 7, 2024

@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 current directory.

For example, when it is necessary to switch the version:

  1. Remove the directory with the current version from versions
  2. Move or copy the version from current to versions
  3. Clear the current directory
  4. Copy the directory from versions/{version} to current

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 bin directory. But it seems that it is much easier than starting individual processes.

What do you think?

@kevincobain2000
Copy link
Owner

Hi @juev
Thanks for the review.
I also do not prefer shell invoked as separate exec.

Your suggestion on version switch is good, it can be done natively - all in GO.
But again, it is windows, I didn't intend to support powershell in the first place, people could just wsl instead.

Having said all that, if calling a separate powershell command, temporarily solves this issue, then it is cool.
The same implementation will never get merged if it was a unix based os, and would code in GO.
Also I have a windows machine now, but hate the idea of powershell in the first place. But people want it so be it as dirty as it can get.

@kevincobain2000
Copy link
Owner

If you are ok then I am also ok for this merge. It's not like it is changing anything for unix based os.

@kevincobain2000
Copy link
Owner

Let me know if you are ok with merge @juev

Copy link
Collaborator

@juev juev left a 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.

@kevincobain2000
Copy link
Owner

Thanks @juev will merge it after dinner

@kevincobain2000 kevincobain2000 merged commit afee1c5 into kevincobain2000:master Feb 9, 2024
12 checks passed
@vunhatchuong vunhatchuong deleted the fix/Win10-symlink branch February 9, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants