Squirrel / Squirrel.Windows

An installation and update framework for Windows desktop apps
MIT License
7.35k stars 1.03k forks source link

Fix `CreateShortcutForThisExe` for .NET Core apps #1752

Closed AArnott closed 2 years ago

AArnott commented 2 years ago

The entrypoint assembly is reported as being a .dll in the .NET Core case. A shortcut to the dll is not what the developer expects and it doesn't work for the user. Instead, look for a nearby .exe with the same file name and use that.

anaisbetts commented 2 years ago

So, someone else proposed this solution as well and I'm not super excited about it, because lots of other projects also have DLLs with the same name as EXEs (remember that Squirrel is used for many non-C# projects as well). If you can come up with a better way of detecting the .NET Core EXE stub (perhaps by peeking in the PE header bytes) that'd be Better than just seeing the DLL with the same name

AArnott commented 2 years ago

Thanks for looking, @anaisbetts. But I think you may be confusing this fix with #1692, which fixes a different problem and jumps from .exe to .dll. I think your concern there has some validity because you're jumping from an .exe which may itself be legit to a dll. But here in this PR, we're going from an invalid .dll selection to a valid .exe selection. I explain in the PR description and code comments just why this is a safe move: selecting a .dll for a shortcut is broken, so this change can't make anything worse. And unless you know of another situation in which GetEntryAssembly returns a .dll instead of an .exe then I think we can call this safe. There won't be anything in the .dll to point to the .exe because the .exe is built long after the .dll is during the build, so our solution has to be pattern based. I also check that the .exe exists for added caution.

anaisbetts commented 2 years ago

@AArnott That works for me, fix the style to match and I'm sold (string.Equals => String.Equals, put { on same line as the if)

AArnott commented 2 years ago

Done