[Proposal] Replace current C#-based shim with C version

[rasa]

Per:
#3294: Killing the shim process does not kill the child process,
#2921: Wait for console applications only,
#2339: python in cmd Ctrl+C problem,
#2006: Windows cmd gvim.exe shim waits for subprocess,
#1896: Handle Control-C in shim wrapper,
#1606: Support shims to both console and GUI apps,
and extras/#2801: Putty shims open a blank window,
I would like us to consider replacing our current C#-based shim code, (or is it at ScoopInstaller/Shim ?) with this C-based code, at https://github.com/71/scoop-better-shimexe .

Its readme explains the benefits. I can't think of any drawbacks.

[Ash258]

I am not against making this change, after some code review of new shim, but it needs to be optional in first few weeks for proper testing.

[trajano]

Can you provide instructions on how to do that?

[trajano]

Hold off on this one found something that may affect people 71/scoop-better-shimexe#6

[trajano]

Based on the discussions on the bug, seems like it may be something on my PC. No one else had reported any issues. Maybe it is good to go.

[gerardog]

I was unable to reproduce @trajano issue, and I have interests in adopting this proposal since gsudo is affected by #3294 and #1896.

How can I help move this forward?

FWIW, I forked ScoopInstaller/Shim at gerardog/Shim, replaced the C# version with this one, and updated the AppVeyor build scripts. last build

P.S. Unsure if the build script should target x32, so it covers all platforms?

[gerardog]

How can I help move this forward?

I'm sorry I am not aware of how Scoop governance works. Only thing I can think of is to mention @lukesampson, and kindly request his advice.

[lukesampson]

@rasa is the man for this. I assume we need a pull request that lets Scoop use the C shim as an option set with scoop config (according to @Ash258's advice), but ask @rasa.

[twirlse]

Any update on this? It would be greatly appreciated.

[pcgeek86]

Would be really nice if we could get this permanently fixed. I had to fix the Windows Terminal configuration by pointing to %USERPROFILE%\\scoop\\apps\\pwsh\\current\\pwsh.exe directly. If this fix were implemented, my understanding is that fixing the Windows Terminal configuration would no longer be required.

Would appreciate your insight, @rasa

[rasa]

To @Ash258’s point of it needing to be optional to start, let’s initially implement via a

scoop config shim v2
scoop reset *

A user could then downgrade by:

scoop config shim v1
scoop reset *

Then, after a few weeks, we can decide if we want to migrate all users to the new shim, and anyone who wants to stay on the old shim can do so by pinning v1.

Sound good @Ash258, @r15ch13, @lukesampson, ...?
If so, I will work on a PR that implements this.

I know that scoop reset * can cause issues for apps that share the same exe names, as the order it iterates through the apps may be different than how the user has their apps currently configured. If that’s a big concern, we could create a new command:

scoop shim reset

and ask users to run that instead of scoop reset *.

[trajano]

I say just go for it, unless someone else reports issues. I am guessing the problems are localized to something on my machine 71/scoop-better-shimexe#6

[qcts33]

There is another shim in C++. https://github.com/kiennq/scoop-better-shimexe
Maybe it is possible to have multiple option? Or just include different shims as applications in buckets.

[rasa]

So, how about

scoop config shim default

for the current c#-based shim,

scoop config shim 71

for 71’s c-based shim,and

scoop config shim kiennq

for kiennq’s c++ based shim?

[rasa]

We could do it in an app, so

scoop install shim-71

or

scoop install shim-kiennq

But that seems extremely kludgy, and I would vote against it.

[gerardog]

@kiennq version starts as fork of @71 shim. Is there a list of improvements, or a reason for rewriting from scratch? Maybe there is a clear winner.
Also, can @trajano reproduce it's issue with any/both shims?
@kiennq could you allow issues in your repo please?

[kiennq]

@kiennq version starts as fork of @71 shim. Is there a list of improvements, or a reason for rewriting from scratch?

The reason for rewriting is because of this issue 71/scoop-better-shimexe#9, which I cannot figure out the root cause immediately. The C version has to manually release memory at sometimes maybe too early and cause use after release bug. That's why I've created a C++ version with proper memory management via smart pointers and ownership model.
It's under discussion with @71 to see if it's appropriated to have this rewrite merged back to main stream.

@kiennq could you allow issues in your repo please?

Yes, if you found issue, please file a bugs.

[rasa]

To test the changes in #3998, follow @gerardog’s comment below, and then to test:

scoop config shim 71
scoop reset *
:: test
scoop config shim kiennq
scoop reset *
:: test

To switch back to the old shim, type:

scoop config shim default
scoop reset *

To switch back to the released codebase, type:

scoop config SCOOP_BRANCH master
scoop update

[gerardog]

Great work @rasa!

What worked for me was:

git fetch origin pull/3998/head:pr3998
scoop config SCOOP_BRANCH pr3998

git remote add rasa https://github.com/rasa/scoop/
git fetch rasa

git checkout pr3998
git branch --set-upstream-to=remotes/rasa/feature/config-shim

scoop update
(and so on)

[rasa]

Replying to @gerardog:

@kiennq version starts as fork of @71 shim. Is there a list of improvements, or a reason for rewriting from scratch? Maybe there is a clear winner.

FYI, @71 said to @kiennq in regards @kiennq's c++-based shim: "Your C++ code is cleaner so I'd be inclined to use it instead of my C, but it does add a dependency to C++."

[71]

To add to this:

• The shim being in C++ isn't much of a problem if the shim is distributed directly as a binary. It might be (insignificantly) slower.
• @kiennq's fork allegedly fixes an issue that I wasn't able to reproduce.

[jcwillox]

So I did some more testing, I found that my antivirus (Bitdefender) was causing the significant increase in opening times. When disabling the antivirus I saw access times similar to what kiennq just posted. Interestingly adding an exception for the shims folder, helped but did not solve the issue.

nircmdc.exe --help	Direct	Kiennq	71	Scoop
AV Active	500	500	500	540
AV Active with Exception	500	500	500	520
AV Disabled	20	50	45	62

To conclude, 0.45s is the price you pay for security I guess. I actually couldn't get a consistent result between the Kiennq's and 71's shims, I would get 1500 for Kiennq, switch to 71's and get 500, then switch back to Kiennq's and also get 500. It's honestly quite weird and overall inconsistent. I did notice that when adding the directory to the exceptions list I got similar scores across the board, and in some testing that didn't document, with the exception I more often than not got very similar scores to when I had the AV disabled.
TDLR; adding an exception to the shims folder fixed the issue Kiennq's shim.

[71]

In any case, I don't think it makes that much sense to benchmark my shim against Kiennq's. C++ will probably be easier to maintain if more changes are needed and can always be optimized, so IMO we should go with their shim regardless. The only reason why my shim should be chosen over theirs is if we need fewer build dependencies, which AFAIK is not the case since we'll likely distribute the shim as a binary directly.

[gerardog]

I've been using the new shim since August and I haven't find any issues.
Now setting up a new box today, and faced the Ctrl-C issue within in one hour.

IMHO it's time to push this fix.

[gerardog]

Windows Defender just quarantined the new shim as if it was a Virus...
I've just submitted the shim to Microsoft as a false positive.

PS C:\> scoop install grep

Installing 'grep' (2.5.4) [64bit]
grep-2.5.4-bin.zip (441.2 KB) [===============================================================================] 100%
Checking hash of grep-2.5.4-bin.zip ... ok.
grep-2.5.4-dep.zip (877.2 KB) [===============================================================================] 100%
Checking hash of grep-2.5.4-dep.zip ... ok.
Extracting grep-2.5.4-bin.zip ... done.
Extracting grep-2.5.4-dep.zip ... done.
Linking ~\scoop\apps\grep\current => ~\scoop\apps\grep\2.5.4
Creating shim for 'grep'.
Copy-Item: C:\Users\***\scoop\apps\scoop\current\lib\core.ps1:581
Line |
 581 |          Copy-Item (get_shim_path) "$shim.exe" -force
     |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Operation did not complete successfully because the file contains a virus or potentially unwanted
     | software. : 'C:\Users\***\scoop\apps\scoop\current\supporting\shims\kiennq\shim.exe'

Creating shim for 'egrep'.
Copy-Item: C:\Users\***\scoop\apps\scoop\current\lib\core.ps1:581
Line |
 581 |          Copy-Item (get_shim_path) "$shim.exe" -force
     |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Operation did not complete successfully because the file contains a virus or potentially unwanted
     | software. : 'C:\Users\***\scoop\apps\scoop\current\supporting\shims\kiennq\shim.exe'

Creating shim for 'fgrep'.
Copy-Item: C:\Users\***\scoop\apps\scoop\current\lib\core.ps1:581
Line |
 581 |          Copy-Item (get_shim_path) "$shim.exe" -force
     |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Operation did not complete successfully because the file contains a virus or potentially unwanted
     | software. : 'C:\Users\***\scoop\apps\scoop\current\supporting\shims\kiennq\shim.exe'

'grep' (2.5.4) was installed successfully!

[kiennq]

Windows Defender just quarantined the new shim as if it was a Virus...

You can try with this https://github.com/kiennq/scoop-better-shimexe/releases/tag/2.2.1, I've recompiled it with -Ofast flag, the antivirus is letting it goes now.

[ylor]

Not sure if this is the best place to post this but both 71 and kiennq have been stable for me. Solving the Ctrl+C issue is a really nice QOL improvement. The extra performance is just gravy.

[gerardog]

I see this PR is closed but the issues are still open. Dumb question. Has the new shim been released to the general audience?
Thanks

[equinox]

Hi @gerardog, no question's dumb! And yeah, the changes and new shim support were added to master (so no longer were they just in develop, the testing branch) in this commit here 7db0fe9

[gerardog]

Hurray! Please consider closing those issues as well now, since they should all be solved.

[rasa]

Ok, I closed all the above issues with this message:

This issue should be fixed in #3998, which was merged into master. To use, type:

scoop config shim kiennq
scoop reset *

or

scoop config shim 71
scoop reset *

Eventually, one of those shims (probably kiennq) will be made the default.

[gerardog]

Oh, I missunderstood equinox then. ☹

71 was a great step forward but kiennq has superseded it. Remember kiennq rewritten it in C++ to avoid some random crashes in 71's. Plus, me (and maybe other people in this thread) had been more inclined to test the later. I've been using it since May, Almost 7 months.

IMHO, at this point kiennq should be the default already for every scoop user.

[rasa]

71 was a great step forward but kiennq has superseded it. Remember kiennq rewritten it in C++ to avoid some random crashes in 71's. Plus, me (and maybe other people in this thread) had been more inclined to test the later. I've been using it since May, Almost 7 months.

Agreed. I have been using it for months without issue too.

IMHO, at this point kiennq should be the default already for every scoop user.

I agree. I would be ok making it the default on new installs, and even if an existing user runs scoop reset, but I'm not sure running reset for all users makes sense, as it would change a users setup in ways that may cause issues.

I will make this change to our develop branch soon.

[kriswilk]

Another vote in favour of the kiennq shim. It has solved a number of problems for me and seems completely stable.

Any ETA on making it the default? It would be nice not to have to always remember to change shims on every new system I work on.