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

Fix CreateShortcutForThisExe for .NET Core apps #1752

Merged
merged 1 commit into from
Oct 5, 2021

Conversation

AArnott
Copy link
Contributor

@AArnott AArnott commented Oct 4, 2021

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
Copy link
Contributor

anaisbetts commented Oct 4, 2021

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
Copy link
Contributor Author

AArnott commented Oct 4, 2021

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
Copy link
Contributor

@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
Copy link
Contributor Author

AArnott commented Oct 4, 2021

Done

// This happens in .NET Core apps. A shortcut to a .dll doesn't work, so replace with the .exe.
string candidateExe = Path.Combine(Path.GetDirectoryName(entrypoint), Path.GetFileNameWithoutExtension(entrypoint)) + ".exe";
if (File.Exists(candidateExe))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

⬆️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️
An .editorconfig file to help conform to this would be great.
Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

For sure, I should add that

Copy link
Contributor

Choose a reason for hiding this comment

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

@anaisbetts see #1482 for an attempt.

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 anaisbetts merged commit 136db8c into Squirrel:develop Oct 5, 2021
@AArnott AArnott deleted the fixCreateShortcut branch October 5, 2021 12:46
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