Skip to content

fix(current): Remove 'current' while it's not a junction#4687

Merged
niheaven merged 3 commits intodevelopfrom
fix-current-junction
Jan 25, 2022
Merged

fix(current): Remove 'current' while it's not a junction#4687
niheaven merged 3 commits intodevelopfrom
fix-current-junction

Conversation

@niheaven
Copy link
Copy Markdown
Member

@niheaven niheaven commented Jan 25, 2022

Description

Junction is copied as native folder when we copy apps folder, and this may break scoop's reset, uninstall, cleanup actions. Use Remove-Item -Recure could fix this bug.

How Has This Been Tested?

Copy-Item D:\Scoop\apps\ag D:\ -Recurse
❯ scoop uninstall ag
Uninstalling 'ag' (2.2.5).
Removing shim 'ag.exe'.
Removing shim 'ag.shim'.
Unlinking D:\Scoop\apps\ag\current
'ag' was uninstalled.
❯ Move-Item D:\ag D:\Scoop\apps
❯ scoop prefix ag
D:\Scoop\apps\ag\current
❯ (Get-Item D:\Scoop\apps\ag\current).GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     False    DirectoryInfo                            System.IO.FileSystemInfo

❯ scoop reset ag

Before:

❯ (Get-Item D:\Scoop\apps\ag\current).LinkType -eq 'Junction'
False

After:

❯ (Get-Item D:\Scoop\apps\ag\current).LinkType -eq 'Junction'
True

Checklist:

  • I have read the Contributing Guide.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@rashil2000
Copy link
Copy Markdown
Member

rashil2000 commented Jan 25, 2022

BTW, use New-Item -ItemType Junction instead of mklink /j to make current junction.

Just to confirm, this works in PS 5.1 and Win 7/8 also, right?

There are 3 more places that use mklink - 2 in lib/install.ps1 and 1 in lib/psmodule.ps1 (see https://cs.github.com/ScoopInstaller/Scoop?q=mklink). We can replace those too.

@niheaven
Copy link
Copy Markdown
Member Author

niheaven commented Jan 25, 2022

It existed in PS 5.1.

Okay, I'll revert the change here (mklink) and change them all in another PR to make clear change.

@rashil2000
Copy link
Copy Markdown
Member

Some more places where we can use Remove-Item instead of rmdir. Would be cool to refactor them too.

& "$env:COMSPEC" /c "rmdir /s /q `"$filepath`""

& "$env:COMSPEC" /c "del `"$filepath`""

& "$env:COMSPEC" /c "rmdir `"$linkfrom`""

& "$env:COMSPEC" /c "rmdir `"$linkfrom`""

@niheaven
Copy link
Copy Markdown
Member Author

See https://github.com/ScoopInstaller/Scoop/pull/3502/files#diff-cfd4446b6fd477007943865889b4cdbf84e1c089d960b82cd3b57c6bf019fce3 for my previous rework of removing directory. I'm separating #3502 into single purpose PRs, and this should be next job soon.

@rashil2000
Copy link
Copy Markdown
Member

Hmm. I just wanted to get rid of COMSPEC entirely, because I faced a ton of issues The system cannot find the path specified. today after migrating Scoop to a different drive. This was addressed earlier in #3993 but only in lib/git.ps1 file.

I'm under the impression that anything COMSPEC does can be handled by PS 5.1 or later.

@niheaven
Copy link
Copy Markdown
Member Author

Yes, of course, everything COMSPEC (a.k.a. cmd.exe) does could be handled by PowerShell Cmdlets.

I'll do a PR and it should be tested to see if there are some disadvantages.

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.

2 participants