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 #71: old symlink should be removed on Windows #72

Merged
merged 7 commits into from
Apr 11, 2024

Conversation

guidoschmidt
Copy link
Contributor

@guidoschmidt guidoschmidt commented Apr 10, 2024

Seems to be a longer journey 😅 ...

just tested version 0.6.2 and unfortunately the old symlink to .zvm/bin is not removed, the change to !errors.Is(err, os.ErrNotExist) seems to fix it. found os.Lstat which checks symbolic links.

Maybe there's a more appropriate way to do this? Looks like there's a newer fs package that can be used for these kind of filesystem checks.

cli/use.go Outdated
// which is specifically to check symbolic links.
// Seemed like the more appropriate solution here
stat, err := os.Lstat(bin_dir);
if errors.Is(err, os.ErrExist) || stat != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

Why back to os.ErrExist instead of fs.ErrExist?

Copy link
Owner

Choose a reason for hiding this comment

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

We could just check to see if the error is nil. If true the file exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can't mix up the os with the fs stuff, as they are different packages? But it's only my educated guess. Honestly it feels like there's way too many ways to do this in golang 😓 the best way maybe is to use os.Lstat and just see if the link is nil? 🤔

@tristanisham
Copy link
Owner

Thanks for making another PR. These system specific bugs are always a pain but we'll fix this one. I'll be on my Windows developer environment tonight so I can work on this bug directly.

@guidoschmidt
Copy link
Contributor Author

Oh nice, hope it doesn't give you too much of a headache

@tristanisham
Copy link
Owner

New bug unlocked. Totally breaks the progress bar when running in command prompt.
image

Removes forced upgrade to Stat for Windows users who are already admin or have enabled symlinks for regular users.
@tristanisham
Copy link
Owner

@guidoschmidt could you please test the changes I've pushed to the PR. For I modified the Symlink function again because for users who are already admins or have enabled symlinks for regular users it was uneccessary and degraded the experience. I also tweaked the Lstat to use the err == nil method since just parsing the errors wasn't working for us.

Test for if we can find Symlinks
@tristanisham
Copy link
Owner

Added a test for our symlink code. It passes so that bit in setBin should be good.

func TestSymlinkExists(t *testing.T) {
	if err := os.Symlink("use_test.go", "symlink.test"); err != nil {
		t.Error(err)
	}

	stat, err := os.Lstat("symlink.test");
	if err != nil {
		t.Errorf("%q: %s", err, stat.Name())
	}

	defer os.Remove("symlink.test")
}

@tristanisham tristanisham self-assigned this Apr 10, 2024
@tristanisham tristanisham added the bug Something isn't working label Apr 10, 2024
@tristanisham tristanisham merged commit 25200dc into tristanisham:master Apr 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants