Skip to content

fix(shim): Add 'Get-CommandPath()' to find git#4913

Merged
niheaven merged 8 commits intodevelopfrom
sh-shim-path
May 27, 2022
Merged

fix(shim): Add 'Get-CommandPath()' to find git#4913
niheaven merged 8 commits intodevelopfrom
sh-shim-path

Conversation

@rashil2000
Copy link
Copy Markdown
Member

Description

Without scoop-which, the shimming function was picking up the ~\scoop\shims directory as source. This corrects it.

Motivation and Context

Closes ScoopInstaller/Main#3562

How Has This Been Tested?

Checklist:

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

@rashil2000 rashil2000 requested a review from niheaven May 9, 2022 16:20
@chawyehsu
Copy link
Copy Markdown
Member

This is a bad implementation. To keep the libcore to be decoupled, it should never call other modules. An abstraction of extracting which from scoop-which to a reusable function would be better. Put it into libcore so functions elsewhere may use it when they need.

@rashil2000
Copy link
Copy Markdown
Member Author

Why do we need to keep core.ps1 decoupled? IIRC we stopped importing it in the installation script.

Nevertheless this PR is mainly intended as a hotfix (which should go out with v2.0.0) otherwise bash shims will not work.

I'll try to implement a proper way later in a subsequent PR.

@niheaven
Copy link
Copy Markdown
Member

I prefer a two-step check for this function: one for scoop-installed git, then a gcm one. scoop installed git could be checked by appdir and if not found, fall back to gcm.

@rashil2000
Copy link
Copy Markdown
Member Author

Checking for scoop installed git using appdir is not feasible, plenty of packages provide the git.exe executable - git, git-with-openssh, git-without-openssh, deepgit, mingit, mingit-busybox etc. We can't put conditions for all of them.

@niheaven
Copy link
Copy Markdown
Member

Hmm, so let's extract the main function of scoop-which and add it to core.ps1 and try to avoid using scoop-which in such a situation IMO. That's the best way.

@niheaven niheaven changed the title fix(shim): Use correct git source directory fix(shim): Add 'Get-CommandPath()' to find git May 24, 2022
@niheaven
Copy link
Copy Markdown
Member

@rashil2000 Tweak some codes by using 'Get-ShimPathfromscoop-shim.ps1`, please test it, thanks.

Comment thread lib/core.ps1 Outdated
@niheaven niheaven merged commit 0f6d012 into develop May 27, 2022
@niheaven niheaven deleted the sh-shim-path branch May 27, 2022 02:18
Comment thread lib/core.ps1
return $null
}
$commandPath = if ($comm.Path -like "$userShims*" -or $comm.Path -like "$globalShims*") {
Get-ShimTarget ($comm.Path -replace '\.exe$', '.shim')
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If a bin includes both *.ps1 and *.exe shims (like aria2c):

> gcm aria2c -all
...\shims\aria2c.ps1
...\shims\aria2c.exe

The command scoop which aria2c show 'aria2c' not found / not a scoop shim..

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The new shim system should use only .exe shims, and .ps1 shims are only used for .cmd or bash script.

Try remove all shims except scoop.* and scoop reset *, and this should be fixed.

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.

4 participants