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

feat(scoop-shim): Add scoop shim to manipulate shims #4736

Merged
merged 20 commits into from
Feb 26, 2022
Merged

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Feb 13, 2022

Description

Add a new scoop shim command, with following subcommand:

  • scoop shim add <shim_name> <command_path>
    • Create shim in shim dir, command_path should be full path or shim name or external program
    • If command_path has whitespaces or has arguments, use quotes
  • scoop shim remove <shim_names>
    • Remove shims, custom or predefined, use whitespaces to separate
  • scoop shim list [<shim_name>]
    • List all shims or specific ones (regex search)
  • scoop shim info <shim_name>
    • Shows shim information
  • scoop shim alter <shim_name>
    • Change shim's source

I borrowed some ideas from #4718 (thanks to @rashil2000) to show shims list table view and make some tweaks to scoop list command.

Motivation and Context

Closes #3303

Relates to #4727

How Has This Been Tested?

image
image
image

Checklist:

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

@rashil2000
Copy link
Member

rashil2000 commented Feb 13, 2022

Some notes:

  • For the list command, there are 7 columns. When there are that many columns in an object, the List view is more comprehensible (instead of Table view).
  • For the info command, we can reuse the ScoopShimType and return the object instead of putting Write-Host.
  • So can we merge the code for scoop-alter here? (instead of calling the script explicitly)

@niheaven
Copy link
Member Author

For the list command, there are 7 columns. When there are that many columns in an object, the List view is more comprehensible (instead of Table view).

For so many shim items, Table View is more compact.

For the info command, we can reuse the ScoopShimType and return the object instead of putting Write-Host.

I can do this, and it is my first thought when writing the function. But I wonder who needs such an object even if we return it? For clear result display, a List View of ScoopShim should be added.

So can we merge the code for scoop-alter here? (instead of calling the script explicitly)

Sure, will do it.

@rashil2000
Copy link
Member

For so many shim items, Table View is more compact.

I wonder how many results it would typically return. Won't it be 2 or 3 at max?

But I wonder who needs such an object even if we return it?

Right now we don't need, but in future someone might xD. Also, it looks a lot nicer.

image

For clear result display, a List View of ScoopShim should be added.

Yes, that's what I had in mind.

@niheaven
Copy link
Member Author

niheaven commented Feb 14, 2022

I wonder how many results it would typically return. Won't it be 2 or 3 at max?

If you just want a full 'list' (a.k.a. scoop shim list), maybe hundreds? Of cause, if you filter it by app name, 2 or 3. Just likes scoop list.

@rashil2000
Copy link
Member

Hmm, understood.

@niheaven
Copy link
Member Author

scoop shim list and scoop shim info:

image

scoop shim alter and scoop shim rm (alias to scoop shim remove):

image

Since scoop shim list may produce non array output, I cannot distinct the output of scoop shim list and scoop shim info, so using different type in ScoopTypes.Format.ps1xml to create different views.

@niheaven
Copy link
Member Author

niheaven commented Feb 14, 2022

After this is merged, Scoop Core could be bumped to version 0.9.0.0. This is a wonderful version IMO, isn't it?

@rashil2000
Copy link
Member

Sure thing. Will review later today. Pretty big PR xD

libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
@rashil2000
Copy link
Member

I have made a commit for minor styling stuff, please pull.

Doubt: when will a shim be hidden? Do you have an example?

@rashil2000
Copy link
Member

Another suggestion: it would be useful to show if a shim is custom-made while doing scoop shim info <shim>. But I guess this is not possible due to current implementation of shims Scoop core.

@niheaven
Copy link
Member Author

Doubt: when will a shim be hidden? Do you have an example?

For some internal alias or some command from other apps that has high priority

~ took 22m12sscoop shim list | ? { $_.IsHidden }

Name     Source           Alternatives IsGlobal IsHidden
----     ------           ------------ -------- --------
ar       busybox                       False    True
bash     busybox                       False    True
cat      coreutils-uutils              False    True
clear    busybox                       False    True
cp       coreutils-uutils              False    True
diff     diffutils                     False    True
dos2unix busybox                       False    True
dotnet   dotnet-sdk                    False    True
echo     coreutils-uutils              False    True
env      coreutils-uutils              False    True
expand   coreutils-uutils              False    True
find     busybox                       False    True
getopt   busybox                       False    True
hostname coreutils-uutils              False    True
iconv    busybox                       False    True

@rasa
Copy link
Member

rasa commented Feb 16, 2022

For some internal alias or some command from other apps that has high priority

So a hidden shim is one app's shim, that's been overwritten by another app's shim?

@niheaven
Copy link
Member Author

niheaven commented Feb 16, 2022

So a hidden shim is one app's shim, that's been overwritten by another app's shim?

A hidden shim is an inaccessible one, for example I've curl installed from Scoop, but since Windows put a curl.exe under System32, I couldn't use scooped curl unless by its full path.

~scoop which curl
C:\WINDOWS\system32\curl.exe

~Get-Command curl -All

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Application     curl.exe                                           7.79.1.0   C:\WINDOWS\system32\curl.exe
Application     curl.exe                                           0.0.0.0    D:\Scoop\shims\curl.exe

~scoop shim info curl

Name     : curl
Path     : D:\Scoop\shims\curl.exe
Source   : curl
Type     : Application
IsGlobal : False
IsHidden : True

The same to some Windows alias, e.g., mv, cat, cp, etc..

@rashil2000
Copy link
Member

Will review later today - I have my exams going on 😅

@niheaven
Copy link
Member Author

Will review later today - I have my exams going on 😅

Aha, good luck!

@niheaven
Copy link
Member Author

I'll merge this one and #4734 and plan to next Core release. If there's some bug or improvement, comment here and I'll make hotfix.

@rashil2000
Copy link
Member

Reviewing now

@rashil2000
Copy link
Member

and handle --global in the script.

This doesn't work. For example, both the following commands will create a global shim, without any shim args:

.\libexec\scoop-shim add bash E:\MSys2\usr\bin\bash.exe --global
.\libexec\scoop-shim add bash E:\MSys2\usr\bin\bash.exe '--global'

@rashil2000
Copy link
Member

I think we can just avoid all the manual handling hassle by doing [Switch]$global instead of [Switch]$g. Since this is a special case, we will just mention that "Use single hyphen instead of double hyphens". Otherwise trying to mix and match unix arguments in PowerShell will create a lot of pain and edge cases.

@niheaven
Copy link
Member Author

PowerShell handle --arg with or without quotes, and remove quotes in the args... So,

  1. As your comment, using $global
  2. Add a virtual param, scoop shim add bash bash --global ''

@rashil2000
Copy link
Member

  1. Add a virtual param, scoop shim add bash bash --global ''

How will we tell users about the reasoning/semantics behind this? This would be confusing IMO.

@niheaven
Copy link
Member Author

How will we tell users about the reasoning/semantics behind this? This would be confusing IMO.

Yes, it is. I'll change the syntax and hope there are no bugs.

@niheaven
Copy link
Member Author

Check the last commit.

Copy link
Member

@rashil2000 rashil2000 left a comment

Choose a reason for hiding this comment

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

It works now!

(Tested in PS 7.1.2).

Just some minor comments

libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
libexec/scoop-shim.ps1 Outdated Show resolved Hide resolved
Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
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.

3 participants