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

Fixups for Squirrel.Windows 2.0 #1623

Merged
merged 4 commits into from
Aug 20, 2020
Merged

Fixups for Squirrel.Windows 2.0 #1623

merged 4 commits into from
Aug 20, 2020

Conversation

anaisbetts
Copy link
Contributor

@anaisbetts anaisbetts commented May 15, 2020

Creating a PR to prepare for Squirrel.Windows 2.0.0 which mostly merges #1578 but also fixes #1616. Since we're moving namespaces around, this is a breaking change but ideally this won't result in any actual work for people.

TODO:

  • Get the heckin' thing to build
  • Expose logging types in a better way since it's a public'ish API
  • Make sure the C# API changes don't break clients too horribly
  • Make sure that electron-winstaller can still build installers
  • Make sure that an update from a Squirrel.Windows 1.x installation doesn't explode
  • Verify that none of the SDK changes bork Windows 7 installers
  • Debug the CMD window showing up

/cc @shiftkey @robmen @JayBeavers @dennisameling

@anaisbetts
Copy link
Contributor Author

If anyone has spare cycles, I'd very much appreciate any and all testing that folx can do - any of the "Make sure that X" TODOs would be super great!

@robmen
Copy link
Contributor

robmen commented May 16, 2020

Why remove the versioning? Is that breaking something at runtime?

I was also able to update to v142 in https://github.com/Squirrel/Squirrel.Windows/tree/robmen/latest-sdks. I'm curious what issues you encountered keeping versioning and using the latest SDKs.

@anaisbetts
Copy link
Contributor Author

anaisbetts commented May 16, 2020

Why remove the versioning?

@robmen It just seems like an unrelated change that we don't really need - our versions are already explicitly tagged

I was also able to update to v142 in https://github.com/Squirrel/Squirrel.Windows/tree/robmen/latest-sdks.

This is probably fine, I'm just blindly paranoid that this will break someone running Win7 because Of Course It Does. This shouldn't be A Thing because it's all statically linked but again, refer to "Of Course It Does"

@robmen
Copy link
Contributor

robmen commented May 16, 2020

GitVersioning adds the Git hash to the file resource which is very helpful tracking a specific build back to exactly what commit was used to build the output. That has proven valuable in the past. We still control the full parts of the version.

v142 removes Windows XP support but includes latest security improvements.

@dennisameling
Copy link
Contributor

Will test the electron-winstaller case on Thursday, will test those installers on Windows 7 as well then (assuming that would cover the Verify that none of the SDK changes bork Windows 7 installers scenario)

dennisameling added a commit to dennisameling/windows-installer that referenced this pull request May 21, 2020
@dennisameling
Copy link
Contributor

Can confirm that electon-winstaller can still build installers with the updated Squirrel version (see dennisameling/windows-installer@ee510f2)

Installing on Windows 7 also still works, but a cmd prompt keeps running in the background until the application is closed(not 100% sure whether this is due to this new Squirrel version, at least can't reproduce on the production version of GH desktop). This is true both on Windows 7 and Windows 10, so it's not a W7-specific thing. After the application is closed and started for the 2nd time, the cmd prompt doesn't show up anymore.

image

@anaisbetts
Copy link
Contributor Author

@dennisameling This is good work, we'll definitely have to look into that

@anaisbetts
Copy link
Contributor Author

@dennisameling If you can, are you able to get the command line of that command prompt window popping up? You can do this via going into Task Manager, figuring out the process corresponding to the window, right-click on the header in the details tab "Select Columns", then choose "Command Line"

image

@dennisameling
Copy link
Contributor

@anaisbetts
When installation starts it's "C:\Users\denni\AppData\Local\SquirrelTemp\Update.exe" --install ."
image

After installation when the cmd prompt is still open in the background, I see the following processes:

image

I uploaded the installer here so you can see it for yourself if you want: https://github.com/dennisameling/desktop/releases/tag/2.5.1-beta1

@anaisbetts
Copy link
Contributor Author

Hey all, I'm still working on this, but between Coronavirus aka watching three children 24x7 without help and an international move that's coming up in 4 weeks, it's hard to find time for this work (I don't work for any company paying me to do this, it's just something I'm doing because I Want This To Work 😅). I'm doing my best!

@wsaeed
Copy link

wsaeed commented Jun 9, 2020

Why remove the versioning?

@robmen It just seems like an unrelated change that we don't really need - our versions are already explicitly tagged

I was also able to update to v142 in https://github.com/Squirrel/Squirrel.Windows/tree/robmen/latest-sdks.

This is probably fine, I'm just blindly paranoid that this will break someone running Win7 because Of Course It Does. This shouldn't be A Thing because it's all statically linked but again, refer to "Of Course It Does"

Windows 7 is no longer supported by Microsoft. So we shouldn't worry about that, and upgrade to latest 142 toolset to incorporate security enhancements.

@dennisameling
Copy link
Contributor

dennisameling commented Jun 9, 2020

So just to be sure I'm doing the right thing, this is the sequence of steps I'm taking:

  1. Use this PR to build Squirrel
  2. Copy the following files to my electron-winstaller fork (manually, there doesn't seem to be an automated means of doing this):
  • Rename net45/update.com to net45/Squirrel.com, then add it
  • Rename net45/Update.exe to net45/Squirrel.exe, then add it
  • Rename net45/Update.pdb to net45/Squirrel.pdb, then add it
  • Win32/Setup.exe
  • Win32/Setup.pdb
  • rename net45/Update-Mono.exe to net45/Squirrel-Mono.exe, then add it
  • rename net45/Update-Mono.pdb to net45/Squirrel-Mono.pdb, then add it
  • Win32/StubExecutable.exe
  • net45/SyncReleases.exe
  • net45/SyncReleases.pdb
  • Win32/WriteZipToSetup.exe
  • Win32/WriteZipToSetup.pdb
  1. Run yarn link in the electron-winstaller folder
  2. Run yarn link electron-winstaller in the desktop repository (GitHub Desktop). Can confirm the symlink is created successfully, so it uses the local Squirrel.exe, Setup.exe, etc.
  3. Run yarn build:prod in the desktop repository
  4. Run yarn package in the desktop repository, this creates the Squirrel installer

I just tested with the v142 PlatformToolset like @robmen mentioned (2cca358) and can confirm it also still works on Windows 7 (tested in VM). So upgrading to v142 should be absolutely fine 🎉 @wsaeed @robmen

Did a bit more research on the CMD window showing up. Just to make sure nothing's wrong with my local build environment, I checked out the 1.9.1 tag (Feb 9 2019) and built that version. No CMD window shows up. Then built from commit 1b7220b (Nov 27 2019), the CMD window now shows up.

So something happened between Feb 9 2019 and Nov 27 2019 that causes the CMD window to show up. If it helps, I can checkout some random commits in between those dates to see which commit is the culprit, unless someone has a different idea/suggestion that might be smarter/more efficient?

@anaisbetts
Copy link
Contributor Author

Hey @dennisameling, this procedure looks correct

I just tested with the v142 PlatformToolset like @robmen mentioned (2cca358) and can confirm it also still works on Windows 7 (tested in VM). So upgrading to v142 should be absolutely fine 🎉 @wsaeed @robmen

Cool, let's merge that then

So something happened between Feb 9 2019 and Nov 27 2019 that causes the CMD window to show up. If it helps, I can checkout some random commits in between those dates to see which commit is the culprit, unless someone has a different idea/suggestion that might be smarter/more efficient?

Have you ever used git bisect? You're basically describing what it'll walk you through automatically! You're gonna kick it off by writing:

git bisect start
git bisect good 1.9.1
git bisect bad 1b7220b

git bisect will then proceed to check out different commits (it's doing a binary search of the commit log). You'll git submodule update --init --recursive, follow your build steps, then test to see if it shows CMD or not. If it doesn't, run git bisect good, if it does? Run git bisect bad

@ComtelJeremy
Copy link

Is there anything I can do to help with these changes? We are currently waiting on the Splat dependency to be resolved so we can push an update to our production app which now uses ReactiveUI. Please let me know if there is anything I can do to assist with getting these changes released.

@dennisameling
Copy link
Contributor

dennisameling commented Aug 8, 2020

Finally had some time to check with the git bisect. Looks like the CMD window starts to show up due to this PR: #1487

@robmen Do you have any idea what could be causing this problem? See the screenshot above: #1623 (comment)

Here's a version you can test with (you'll see it shows Update.exe in a new CMD window): https://github.com/dennisameling/desktop/releases/tag/2.5.1-beta1

@dennisameling
Copy link
Contributor

Found it. This is where the bug was introduced: https://github.com/Squirrel/Squirrel.Windows/pull/1487/files#diff-5fabd62d2b450baad1635d76f78c2d7bR7 - the OutputType of Update.csproj should be WinExe instead of Exe. Found the hint in this thread:

A common way to make a "hidden" program is using /target:winexe since it won't display a console, regardless of what way it is run.

@anaisbetts here's a PR to fix the behavior 😄 #1642

@anaisbetts
Copy link
Contributor Author

Great catch!

@dennisameling
Copy link
Contributor

@ComtelJeremy any chance you could test Squirrel against this PR? Especially:

  • Can you build an installer successfully?
  • Does the installer work as expected?
  • Does an upgrade from a previous installation work as well?
  • Does the installer work on Windows 7 (I have a Hyper-V VM available for testing if you need one)

If you want to have a pre-built version of this PR version, let me know and I'll drop a ZIP build here 🚀

For the other open points I think we need some actions from @anaisbetts, but correct me if I'm wrong 😊

@ComtelJeremy
Copy link

@dennisameling I will do what I can! I would appreciate both the ZIP build (although I can also build this PR if it is easier) and the Win 7 image (I don't have one handy). I can test with our existing Squirrel aware project (likely will be able to test a bit this weekend).

@dennisameling
Copy link
Contributor

dennisameling commented Aug 14, 2020

@ComtelJeremy (EDIT: Win7 VM link removed)

Here's the custom build of this PR (I also included 775846c to prevent the CMD terminal from showing up while installing): https://github.com/dennisameling/Squirrel.Windows/releases/tag/1623

@ComtelJeremy
Copy link

@dennisameling I have the image and zip, thanks! You can take down the link, if you like. I'll try to test this weekend. 🚀

@ComtelJeremy
Copy link

@dennisameling Hang on... didn't get one file... 🤦

@ComtelJeremy
Copy link

@dennisameling Got it now. Thank you.

@ComtelJeremy
Copy link

@dennisameling and @anaisbetts, here are my results:

  • Installer built successfully. I did not try the .msi, however the .exe worked fine.
  • Installer ran successfully. No issues.
  • Update via a local Releases source (C:\Releases) worked fine with this version of Squirrel, as well.
  • Upgrade from previous version of Squirrel works as expected.
    • Upgrade using Setup.exe works.
    • Upgrade using Squirrel and a local Releases source (C:\Releases) works fine too.
  • All of the installation and update tests above were successful with Windows 7 as well (I did not test building the installer on Windows 7).

Everything worked as expected. The only difference I noticed was the SquirrelTemp log filename changed, but this probably changed a while ago and I never noticed (as we were still using an older version of Squirrel in our app and hadn't updated the dependency in a while until these tests). I also noticed that now, with the new version, the cmd window is hidden again.

Hopefully this testing helps a little. Let me know if you need more info or anything else tested that I couldn't think of. Happy to help, as Squirrel has been very useful for us. Have a good weekend.

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.

Squirrel installer crashes on Windows 10 ARM64 (Cecil update required)
5 participants