-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
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! |
Why remove the versioning? Is that breaking something at runtime? I was also able to update to |
@robmen It just seems like an unrelated change that we don't really need - our versions are already explicitly tagged
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" |
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.
|
Will test the |
Can confirm that 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. |
@dennisameling This is good work, we'll definitely have to look into that |
@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" |
@anaisbetts After installation when the cmd prompt is still open in the background, I see the following processes: 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 |
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! |
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. |
So just to be sure I'm doing the right thing, this is the sequence of steps I'm taking:
I just tested with the 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 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? |
Hey @dennisameling, this procedure looks correct
Cool, let's merge that then
Have you ever used git bisect start
git bisect good 1.9.1
git bisect bad 1b7220b
|
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. |
Finally had some time to check with the @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 |
Found it. This is where the bug was introduced: https://github.com/Squirrel/Squirrel.Windows/pull/1487/files#diff-5fabd62d2b450baad1635d76f78c2d7bR7 - the OutputType of
@anaisbetts here's a PR to fix the behavior 😄 #1642 |
Great catch! |
@ComtelJeremy any chance you could test Squirrel against this PR? Especially:
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 😊 |
@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). |
@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 |
@dennisameling I have the image and zip, thanks! You can take down the link, if you like. I'll try to test this weekend. 🚀 |
@dennisameling Hang on... didn't get one file... 🤦 |
@dennisameling Got it now. Thank you. |
@dennisameling and @anaisbetts, here are my results:
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. |
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:
/cc @shiftkey @robmen @JayBeavers @dennisameling