git-for-windows / git

A fork of Git containing Windows-specific patches.
http://gitforwindows.org/
Other
8.34k stars 2.53k forks source link

Installer - Allow /CloseApplications to do what it needs to ensure that silent install/upgrades complete successfully #312

Closed ferventcoder closed 7 years ago

ferventcoder commented 9 years ago

As a user installing/upgrading silently, if something like ssh-agent is running, it can cause the process to hang or fail. It would be nice to be able to pass something like /CLOSEAPPLICATIONS to signify that "Yes, I am sure I would like everything closed so the install/upgrade can continue".

For a GUI based installer, I agree with @sschuberth on "it simply does not feel right to kill ssh-agent without asking the user first." However, for a silent install, a user should be able to pass something that signifies that they are okay with moving forward with whatever is necessary to make the install/upgrade happen successfully.

The relevant code blocks look to have moved over to https://github.com/git-for-windows/build-extra/blob/9f0c810c3872e335218833157272cde7d9f6b7a8/installer/install.iss.in#L349-L418

Follow up from https://github.com/msysgit/msysgit/issues/249.

dscho commented 9 years ago

Thanks for following up. I agree with the reasoning. @ferventcoder have you managed to build an installer using the new Git for Windows SDK yet? If so, could you just patch in the relevant usr\bin\ssh-agent.exe lines and see whether it Does The Right Thing?

ferventcoder commented 9 years ago

I'm good to help out, I haven't attempted to build with the new SDK yet. :)

dscho commented 9 years ago

@ferventcoder could you do that? I'll need you to test things with the new SDK anyway...

dscho commented 9 years ago

@ferventcoder once you installed the Git for Windows SDK, you can make an installer using the silent-install branch.

If that manages to find the ssh-agent and close it, I will merge said branch. Could you please test?

ferventcoder commented 9 years ago

Will do. Hopefully today.

ferventcoder commented 9 years ago

Attempted to extract produced an issue: image

I don't need a proxy.

ferventcoder commented 9 years ago

I think a reboot may be necessary.

ping dl.bintray.com times out.

dscho commented 9 years ago

ping dl.bintray.com times out.

That is correct, dl.bintray.com does not answer.

But does it resolve, that is the question... If in doubt, you could either switch to Google's DNS (8.8.8.8) or put the entry "5.153.24.114 dl.bintray.com" into your C:\Windows\system32\drivers\etc\hosts.

Or maybe just restarting the SDK installer will work around that problem?

ferventcoder commented 9 years ago

Or maybe just restarting the SDK installer will work around that problem?

Gave it a couple of shots. I decided on a reboot to see if that clears things out. The next step is to flush the dns cache, as that was not the IP address that the machine in question attempting to ping. :/

Usually use Google's DNS, but I'll check for sure. I believe it is a local machine issue.

ferventcoder commented 9 years ago

Do you think the issue is possibly a permission-related issue with regards to bintray?

These items don't show up, but that doesn't mean they won't work. Based on mirrorlist.git-for-windows:

These don't have any public direct downloads:

ferventcoder commented 9 years ago

No, that's not the issue. If I keep trying enough times, it works. :/ Definitely something with my machine.

ferventcoder commented 9 years ago

Okay, got something built, will test in a little while and give you a heads up. :)

dscho commented 9 years ago

Thanks!

ferventcoder commented 9 years ago

Doesn't quite look like that worked

image

dscho commented 9 years ago

Darn. Will have a look tomorrow.

ferventcoder commented 9 years ago

@dscho I wonder if the uninstall was coming from the OLDer git that was installed? Perhaps I should generate two installers and try this. One to install and run, and the next for upgrade?

dscho commented 9 years ago

I wonder if the uninstall was coming from the OLDer git that was installed?

Oh my, you are absolutely correct!

Perhaps I should generate two installers and try this. One to install and run, and the next for upgrade?

Actually, one and the same installer should be enough. You can reinstall one and the same version, e.g. to reinitialize messed-up installations or to change the settings in the wizard pages.

So maybe you can install it once (without a running ssh-agent), then start the ssh-agent, and then try the silent installation again?

Thank you so much!

ferventcoder commented 9 years ago

Will do.

dscho commented 9 years ago

Thank you!

TheBigBear commented 9 years ago

@ferventcoder @dscho I am also a users that is installing/upgrading silently, does this mean that there is a /silent and a /silent-uninstall option in the works and coming out real soon now? ;-)

The github support person, Scott Sanicki, that I asked this question via email pointed me to the Squirrel.Windows project and said that the Github Desktop software was going to switch to that installer to finally get a working /silent install option?

What cpomes first? This /silent and /uninstall under discussion here, or the switch to the Squirrel.Windows (ClickOnce act-a-like but better) installer?

quote from email from "Scott Sanicki (Github staff)"

Unfortunately we do not have a standalone installer at this time. GitHub Desktop makes use of Microsoft's ClickOnce technology for installation and updates.

We are currently working on an open source replacement for ClickOnce here: https://github.com/squirrel/squirrel.windows (We're always looking for contributors to help!)

Once that technology is complete and ready for use, we hope to switch GitHub Desktop to use that. It would allow for a standalone installer.

PS: I also just noticed that Squirrel.Windows reached 1.0 status within the last few days.

dscho commented 9 years ago

@TheBigBear please note that GitHub for Windows and Git for Windows are different projects. It is true that GitHub for Windows bundles a portable version of Git for Windows, but that does not include the installer that this ticket is about.

TheBigBear commented 9 years ago

@dscho so this one relates to 'git-for-windows' (dooh read the URL!) ;-) aka the friendly fork of msysgit which has just come out as the first windows version of a windows cmd line git in that line of succession without a 'preview' in the naming and came out right away with version number 2.5.0?

Ah, I see, sorry for the confusion, my bad. I hadn't noticed the issues of the silent install and uninstalls hanging under certain conditions. (Note to self - more testing!)

Thanks @ferventcoder for coming to the resuce (once again), your efforts are really much appreciated.

ferventcoder commented 9 years ago

@TheBigBear To answer the question about Squirrel for Windows, it already has a silent switch - -s.

TheBigBear commented 9 years ago

@ferventcoder yes, thanks, I know and like that part of it. [But as an IT admin I don't really like the concept of 'user installs' at all - but that is an entirely differnet topic alltogether. ;-)]

But Squirel.Windows installer is not yet used as the installer of Github Desktop for windows.

ATM it only has the "ugly" "ugly" MS ClickOnce installer which can't be kept quiet or free of user interaction. (ugly not from aestehics or visual point of view, but from angle of IT admin seeking backgound silent mass deployment)

Rob sure you remember choco had to resort to using autohotkey macros - to get it installed quietly, and uninstall in a quiet way is near impssoible as there can be soo many reasons why it doesn't want to do it without interaction.

Sorry for using the 'bandwidth' in this (almost) totally un-related topic, for this long rant.

ferventcoder commented 9 years ago

@dscho It's never as easy as we'd hope. :/

Git-2.5.1-test-64-bit.exe /VERYSILENT /NORESTART /NOCANCEL /SP- /CLOSEAPPLICATIONS /RESTARTAPPLICATIONS /NOICONS /COMPONENTS="icons,icons\quicklaunch,ext,ext\reg,ext\reg\shellhere,ext\reg\guihere,assoc,assoc_sh" /LOG

image

image

I wonder if InnoSetup has the concept of running arbitrary commands? Here's what I would see it do, not even sure it is legal yet though:

%systemroot%\system32\taskkill.exe /F /IM ssh-agent.exe /T

image

dscho commented 9 years ago

It's never as easy as we'd hope. :/

Bummer!

Here's what I would see it do, not even sure it is legal yet though:

taskkill /F /IM ssh-agent.exe /T

We have to be a little careful here because people might have another ssh-agent running, too, and we must not kill any ssh-agent that did not originate from Git for Windows...

dscho commented 9 years ago

Oh, wait a minute... Why is your ssh-agent running as vagrant? Are you sure it is the one from Git for Windows? You should be able to find out by having another (portable) Git for Windows somewhere and calling its ps -W | grep ssh-agent (which outputs the full path of ssh-agent).

ferventcoder commented 9 years ago

I wonder if InnoSetup has the concept of running arbitrary commands?

Looks like it is possible - http://www.jrsoftware.org/ishelp/index.php?topic=isxfunc_exec You are probably already familiar with that though.

We have to be a little careful here because people might have another ssh-agent running, too, and we must not kill any ssh-agent that did not originate from Git for Windows...

How do we know the difference? Or is that where the problem lies?

So two questions -

  1. Is killing all ssh-agent processes harmful (even the ones Git doesn't own) or just a minor annoyance?
  2. If we kill off all ssh-agent processes, what side effects could occur?
dscho commented 9 years ago

If we kill off all ssh-agent processes, what side effects could occur?

That would interfere with other applications that bring their own ssh-agent... I would really like to avoid that (I remember for example when I had to administer an SSH server, long time ago, and carefully started a second SSH daemon so I could shut down the main SSH daemon, and of course the script to shut down the first daemon killed all SSH daemons. That was no fun, I can tell you!).

dscho commented 9 years ago

So now I am really puzzled... /CLOSEAPPLICATIONS kills e.g. usr/bin/sh.exe, but not usr/bin/ssh-agent.exe?

ferventcoder commented 9 years ago

Why is your ssh-agent running as vagrant?

That's the name of my user on this vagrant box. It looks like the right process:

image

ps -W | grep ssh-agent

image

dscho commented 9 years ago

Yeah, sorry. Thanks for clarifying!

ferventcoder commented 9 years ago

So now I am really puzzled... /CLOSEAPPLICATIONS kills e.g. usr/bin/sh.exe, but not usr/bin/ssh-agent.exe?

I'm not sure sh was running. I didn't have a bash console running, just regular command line. It's worth giving that a shot though.

ferventcoder commented 9 years ago

Trying to stick to tools that are available in stock Windows here to get the ssh-agent processes that we care about for the upgrade.

wmic process where "name='ssh-agent.exe'" get processid, executablepath

image

sschuberth commented 9 years ago

I'm only briefly following the conversation, so apologies if this is irrelevant to you, but I wanted to point out that AFAIK the installer is still using the code that I wrote in modules.inc.iss. It is not using Inno Setup's built-in support for the Restart manager API yet. This is because we needed to be able to detect running processes that lock your files before Inno Setup had the feature. That said, the /CLOSEAPPLICATIONS switch refers to Inno Setup's build-in support, which might interfere with the logic in modules.inc.iss.

ferventcoder commented 9 years ago

Removing the silent option, it appears that bash interferes as well.

That is probably expected though. image

ferventcoder commented 9 years ago

@sschuberth I do remember a conversation about that. :)

I believe that is why the choco package currently doesn't have /CLOSEAPPLICATIONS /RESTARTAPPLICATIONS in it.

dscho commented 9 years ago

@sschuberth thanks for noticing! From this comment I assumed that we use the Restart Manager, though I see that we're using InnoSetup 5.5.6 (and its major version is below 6)...

dscho commented 9 years ago

Whoops. The version in question is actually the Windows version, so nevermind!

sschuberth commented 9 years ago

I believe that is why the choco package currently doesn't have /CLOSEAPPLICATIONS /RESTARTAPPLICATIONS in it.

Exactly.

To further clarify, my modules.inc.iss also uses the Restart Manager API if the Windows version supports it, and uses fallbacks otherwise. It's just that is uses the Restart Manager API directly, not via Inno Setup's now built-in support. Inno Setup has no fallback if the Windows version does not support the Restart Manager API.

dscho commented 9 years ago

So my guess is that for some reason, ssh-agent.exe might be marked as not restartable here.

dscho commented 9 years ago

Point in favor of my theory: https://github.com/git-for-windows/build-extra/blob/4e13a83d1a386217c021eb1360b063103d052b0c/installer/install.iss.in#L1067-L1076 (this is responsible for this message box.

sschuberth commented 9 years ago

ssh-agent.exe for sure is not restartable (in the installer's sense), as restartable applications need to link against the Restart Manager API, and I'm very certain that there is no such Windows-specific code in ssh-agent.exe.

dscho commented 9 years ago

Okay, so we would need to kill it manually without restarting it (it is not restartable for sure because the SSH_AUTH_SOCK and SSH_AGENT_PID settings cannot be the same after restarting).

ferventcoder commented 9 years ago

@dscho @sschuberth What if it were just /CLOSEAPPLICATIONS and no restart?

sschuberth commented 9 years ago

According to this comment by the maintainer of Inno Setup (and also this comment by a collaborator) that should work.

dscho commented 9 years ago

@ferventcoder I fear that /CLOSEAPPLICATIONS still would only try to use the Restart Manager (hence you see that ssh-agent.exe is not closed).

So what I have in mind is to test whether CLOSEAPPLICATIONS is in effect and call TerminateProcess() on the ssh-agent.exe process if it is in the list and marked not restartable. (Probably should also do that if RESTARTAPPLICATIONS is in effect, we cannot restart it, but we can force it to exit).

sschuberth commented 9 years ago

I fear that /CLOSEAPPLICATIONS still would only try to use the Restart Manager

No, that should not be the case, see my comment above. Inno Setup is passing 0 (and not RmShutdownOnlyRegistered) to RmShutdown.

dscho commented 9 years ago

Where do we use Restart Manager explicitly?

sschuberth commented 9 years ago

Here, here and here (basically, just look for Windows API calls that start with Rm).