WorkingDir incorrect in NetSparkleUpdater.RunDownloadedInstaller

[SeanAWalsh]

I had a look at Issue #183 to see if I might volunteer to help. In doing this I realised why my upgrades never restart the application after the upgrade. It's because I never set "RelaunchAfterUpdate = true" for my SparkleUpdater object. It defaults to false, so even though the caption in the button in UpdateAvailableWindow says "Install and Relaunch" my app never relaunched. Always meant to investigate this and now know why. Result. Might be worth putting this in the documentation. Maybe default value of RelaunchAfterUpdate should be true. Maybe button caption needs a visit...

However, in passing I had a look at the "cmd" file written into my Local/AppData/Temp folder and I noticed that it doesn't get the working directory correct.

Mine said something along the lines of:

:install
"C:\Users\sean\AppData\Local\Temp\MyProduct.1.3.72.0.exe"
cd file:///C:/Program Files (x86)/MyCompany/My Product Folder/NetSparkle.DLL
"C:\Program Files (x86)\MyCompany\My Product Folder\MyProduct.exe" 
:afterinstall
endlocal

So restarts have a "silent" error message and don't get the right working directory. This probably doesn't matter too much, as for most programs nowadays the working directory isn't that important.

The "file:///..." string has two problems

• It isn't quoted so paths with spaces will be problematic.
• It's actually a DLL name rather than a folder.

Reason being that Utilities.GetFullBaseDirectory() says:
// https://stackoverflow.com/a/837501/3938401
return System.Reflection.Assembly.GetExecutingAssembly().CodeBase;

Reading the stackoverflow article and with a bit of testing, I came up with:
// https://stackoverflow.com/a/837501/3938401
return Path.GetDirectoryName(new Uri(System.Reflection.Assembly.GetExecutingAssembly().CodeBase).LocalPath);

This works as I think it is intended. The new Uri(wrapper) is needed to handle the "file:///" thing and get a LocalPath and then the Path.GetDirectoryName returns the path that is wanted. This is discussed in a roundabout way in the stackoverflow article.

I then did this to NetSparkle.RunDownloadedInstaller

	...
	if (RelaunchAfterUpdate)
	{
		const char q = '"';				// Apply Quotes to workingDir
		relaunchAfterUpdate = $@"
		cd {q}{workingDir}{q}
		{executableName}";
	}

	string output = $@"
		chcp 65001 > nul
		set /A counter=0                       
		setlocal ENABLEDELAYEDEXPANSION

		:loop
			set /A counter=!counter!+1
			if !counter! == 90 (
				goto :afterinstall
			)
			tasklist | findstr ""\<{processID}\>"" > nul
			if not errorlevel 1 (
				timeout /t 1 > nul
				goto :loop
			)

		:install
			{installerCmd}

		:afterinstall
			{relaunchAfterUpdate}			// Move this call to here

		endlocal";
	await write.WriteAsync(output);				// Make this an async call
	write.Close();
}

I did 3 things here.

• Quoted workingDir to handle paths with spaces
• Moved the {relaunchAfterUpdate} line after label :afterinstall (minor, but seems like where it belongs)
• Changed "write.Write(output);" to "await write.WriteAsync(output);" as DownLoadInstaller is an async method

So anyway, now I get:

...
:install
	"C:\Users\sean\AppData\Local\Temp\MyProduct.1.3.72.0.exe"

:afterinstall
	cd "C:\Program Files (x86)\MyCompany\My Product Folder"
	"C:\Program Files (x86)\MyCompany\My Product Folder\MyProduct.exe" 

endlocal

Works a charm.

[Deadpikle]

👋🏻 Thanks for the reports!

It's because I never set "RelaunchAfterUpdate = true"

This probably needs to be more obvious in the readme somewhere, and yes, if it's not set to true, the button should say 'Install' and not 'Install and Relaunch'. That'd be a bug.

Quoted workingDir to handle paths with spaces

Yeah this would be a bug and needs to be fixed; good catch.

Moved the {relaunchAfterUpdate} line after label :afterinstall (minor, but seems like where it belongs)

Mmm....I agree with this, but probably need to add some docs that say "if your installer doesn't launch within 90 seconds, the app will restart without updating" or something. I think restarting an un-updated app would be preferable to doing nothing.

Changed "write.Write(output);" to "await write.WriteAsync(output);" as DownLoadInstaller is an async method

Makes sense.

RestartExecutablePath / Utilities.GetFullBaseDirectory();

Hmmmm...RestartExecutablePath is modifiable, so at least users can fix this on their own...need to look at this again, as the SO comments vary widely in what "works" vs what "doesn't work". See also #146.

[SeanAWalsh]

I spent a few hours going through the various options in the SO article. I tested without the new Uri wrapper and found small vestiges of the file:/// left behind (i.e. Path.GetDirectoryName(....Codebase)).

Codebase definitely returns file:///C:/Program Files.../NetSparkle.dll

In the end, the line I came up with worked for me (Winforms / .NetFramework.

The only thing I didn't check was whether to use GetExecutingAssembly or GetEntryAssembly. In the end, I decided this really didn't matter. I couldn't think of a situation where they would be any different and in the end we are merely supplying a default value here for something which the NetSparkle user has full control of anyway.

[SeanAWalsh]

By the way, my moving of the relaunch lines after the :afterinstall label was purely aesthetic. NetSparkle has always relaunched after an unsuccessful install. So the moving of these lines does not change behaviour in any way as control flow will just ignore the label (as it always has anyway).

We'd need a NoRelaunchAfterUnsuccessfulUpgrade or NoRelaunchAfterUnsuccessfulInstall property on the SparkleUpdater and then a minor bit of string building to slot in the "IF ERRORLEVEL 1" somewhere to implement this behaviour. Not worth the bother as I don't think anyone would ever want to use this. I agree with you when you say that restarting an un-updated app is preferable to doing nothing.

[Deadpikle]

Have made the following changes:

1. Made sure workingDir was quoted properly.
2. Wrote to files async
3. Fixed the bat file not exiting after 90 seconds to match it up with Linux/macOS behavior if the main process doesn't end. The process that needs to end not leaving is now properly considered an error.
4. Fixed macOS/Linux not using the variable executable path/name to actually align with documentation...programmers may need to test each operating system for this since macOS/Linux might do things differently

Still to do:

This probably needs to be more obvious in the readme somewhere, and yes, if it's not set to true, the button should say 'Install' and not 'Install and Relaunch'. That'd be a bug.

This will take a bit more work since we'd have to update the view models (Avalonia, WPF) with more things to be able to handle this properly. I didn't look at WinForms changes, but presumably that also needs adjustments.

I think I never found this issue myself because the software I wrote that uses NetSparkle restarts the software via the installer, not via NetSparkle.

RestartExecutablePath changes

I hear you, but since it's configurable and likely different on .NET Framework vs .NET Core/5+, I need to see what the "best default" is. Perhaps for .NET Framework we can use what you've suggested (with a fallback to Environment.CommandLine if that throws, maybe?), and .NET Core / .NET 5+ can default to System.Diagnostics.Process.GetCurrentProcess().MainModule.FileName or something based on #146 (comment).

[Deadpikle]

This is now done.

Install and Relaunch

IUIFactory now takes a SparkleUpdater for all methods so that it can know how things are set up. Sorry for this breaking change, but happier to do it now than wait for a 3.0!!! Button titles have been fixed in the 4 built-in UIs.

RestartExecutablePath changes

This is now fixed and working on all 4 UIs. RestartExecutablePath is now actually a path and RestartExecutableName is now actually a file name (by default). Slightly different functionality for .NET Framework vs .NET Core. Hopefully the defaults will work for most people.

[SeanAWalsh]

Many thanks!