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

The kiennq shim should be the default #4333

Closed
kriswilk opened this issue Apr 20, 2021 · 14 comments
Closed

The kiennq shim should be the default #4333

kriswilk opened this issue Apr 20, 2021 · 14 comments

Comments

@kriswilk
Copy link

I commented in #3634 but since it's closed, I thought it deserved some fresh exposure.

I've had trouble with shims leaving blank console windows open for some apps and some particular use cases. PR #3998 adds alternative shims which solve this and many other problems that users have reported. In particular, I have been using the "kiennq" shim with great results. It's working perfectly for all my apps.

On Dec 23, 2020 @rasa said: "Eventually, one of those shims (probably kiennq) will be made the default." There seems to be virtually unanimous agreement that it is the preferred shim among all the options.

Considering the number of issues that it resolves, I think it's more than time for "kiennq" to become the default shim. I'd really prefer not having to change the config on every system I setup just to enable it.

@parkovski
Copy link
Contributor

parkovski commented May 14, 2021

Btw, this exe can be made much smaller - I used the MS compiler with these flags and got an 18K exe instead of the 124K one available now.

# /MD = dynamic link single-threaded libc
# /O1 = max space optimization
# /GF = read-only string pooling
# /GR = RTTI
# /GL = Link time code-gen
cl.exe /nologo /std:c++17 /DNDEBUG /MD /O1 /GF /GR- /GL /Febin\shim.exe /Foobj\shim.obj shim.cpp

And if you actually want the most reduction in size, symlinking an exe is fine if the exe only dynamically links to system DLLs, but I doubt that's a drop-in replacement solution.

@rashil2000
Copy link
Member

@rasa This proposal has been sitting for a really long time. Perhaps we should make this change?

@kiennq Is the binary size reduction possible, as @parkovski suggests?

@kiennq
Copy link
Contributor

kiennq commented Nov 10, 2021

Is the binary size reduction possible, as @parkovski suggests?

I think the size reduction is coming from /MD flag, which actually makes the produced binary depends on system DLLs (or even msvc cpp runtime DLLs). Since the required DLLs might not be available on some Windows environments, to be on the safe side, I would just go with the full static link binary like now.

@kriswilk
Copy link
Author

Is the binary size reduction possible, as @parkovski suggests?

I think the size reduction is coming from /MD flag, which actually makes the produced binary depends on system DLLs ...

Yes, that's got to be the primary reason for the dramatic size difference. I agree with @kiennq that adding such a dependency is a bad idea, and that the well-tested, statically-linked version should be adopted as the default.

@rasa
Copy link
Member

rasa commented Nov 10, 2021

@rasa This proposal has been sitting for a really long time. Perhaps we should make this change?

Agreed. We change the default at 7db0fe9#diff-cfd4446b6fd477007943865889b4cdbf84e1c089d960b82cd3b57c6bf019fce3R611

Making this change will overwrite the shim on a scoop reset *, right (assuming the user hasn't run scoop config shim)?

Is that all we need to do?

@rashil2000
Copy link
Member

Assuming that the get_shim_path function gets called at every new app install, yes. Previous shims will remain unchanged, and for users who have changed shim through scoop config shim, nothing will be affected.

/cc @niheaven

@rasa
Copy link
Member

rasa commented Nov 10, 2021

Note that https://github.com/zoritle/rshim#why-this-exist says "calling [kiennq's shim] with any executable result in an infinite recursion of creating subprocess until eating all memory for unknown subtle reason."

I experienced this same issue many times and reported it in #3877 . This was using the original shim, so this may be a problem endemic to all shims.

Also note in https://github.com/baulk/baulk#package-management , it says

Tips: In Windows, after starting the process, we can use GetModuleFileNameW to get the binary file path of the process,
but when the process starts from the symbolic link, the path of the symbolic link will be used. If we only use links in baulk to create symbolic links to the links directory, there may be a problem that a specific dll cannot be loaded, so here we use the launcher mechanism to solve this problem.

This might be worth exploring. It sure seems like symlinks would work better than shims, as they do in Linux.

In addition to the Rust-based https://github.com/zoritle/rshim , we could also add support for https://github.com/bradms/zshim which is written in Zig.

@rasa
Copy link
Member

rasa commented Nov 10, 2021

Also note that Baulk's roadmap lists Limited compatibility with scoop manifest (?) as one of its goals.

@niheaven
Copy link
Member

Is UPX useful for file size?

And let's back to origin that why we need shims to see if symbol links or junctions are better than shims.

@rashil2000
Copy link
Member

rashil2000 commented Nov 10, 2021

This might be worth exploring. It sure seems like symlinks would work better than shims, as they do in Linux.

I don't think symlinks would work out. On Linux there's an important difference - the object libraries are stored separately and are already in LIBRARY_PATH (or some other standard variable), so the problem of missing linked libraries won't arise.

I have experimented with symlinks instead of shims in the past, and I always have to use a shim because most Windows programs today ship with their DLLs in the bin folder of the application itself, and when a symlink is used, the OS takes up the location of the symlink itself instead of the original location, leading to runtime errors.

I'm not sure how a hardlink would behave here, but hardlinks have their own quirks and would require significant change in Scoop core.

Note that https://github.com/zoritle/rshim#why-this-exist says "calling [kiennq's shim] with any executable result in an infinite recursion of creating subprocess until eating all memory for unknown subtle reason."

I was actually about to suggest merging #4229 because of that. Since this is non-breaking change anyway, and more options are always better.

@eugenesvk
Copy link

with their DLLs in the bin folder of the application itself, and when a symlink is used, the OS takes up the location of the symlink itself instead of the original location, leading to runtime errors.

Is there some nice hack around this limitation? I know this can be "fixed" with symlinking the DLLs :) to a PATH'ed folder, although this becomes a brittle dll hell when dll names from different apps are duplicate :(
The hardlinks face the same issue, it's basically the same as copying an exe file to another folder (you can't differentiate the new hardlink from the original hardlink)

@rashil2000
Copy link
Member

Is there some nice hack around this limitation?

Not that I'm aware of. I have a feeling this was the reason shims were employed by Scoop.

@eugenesvk
Copy link

this was the reason shims were employed by Scoop

It was then, but we're smarter now 🤓 and are even using Rust!, would be great if someone found a way to fix the symlinks

@rashil2000
Copy link
Member

Merged in develop branch - #4543

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

No branches or pull requests

7 participants