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

Migrate update framework to Velopack #28743

Merged
merged 25 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0ee8918
initial implementation
smallketchup82 Jun 26, 2024
1025e5b
rewrite the restart function
smallketchup82 Jun 27, 2024
04c8df0
Merge branch 'master' into velopack
smallketchup82 Jun 27, 2024
36a3765
Replace with attemptexit to better display how restarting is borked
smallketchup82 Jun 27, 2024
fae8f5f
Refactor VeloUpdateManager
smallketchup82 Jul 4, 2024
cae3607
Fix up restarting
smallketchup82 Jul 4, 2024
c13f24d
Remove InstallingUpdate progress notification
smallketchup82 Jul 4, 2024
461b791
Remove SimpleUpdateManager
smallketchup82 Jul 4, 2024
9e01cf7
Move setupVelo logic higher up
smallketchup82 Jul 4, 2024
6a03092
Reformat
smallketchup82 Jul 4, 2024
72cf6bb
Allow downgrading
smallketchup82 Jul 4, 2024
4898cff
Restart patch
smallketchup82 Jul 4, 2024
71816c0
Resurrect SimpleUpdateManager as MobileUpdateNotifier
smallketchup82 Jul 5, 2024
fcede9a
Bump velopack version
smallketchup82 Aug 7, 2024
d5b5215
Merge branch 'master' into velopack
smoogipoo Aug 31, 2024
636ee50
Rename to VelopackUpdateManager
smoogipoo Aug 31, 2024
a038799
Update osu.Desktop.csproj
smallketchup82 Aug 31, 2024
b990af6
Use full name
peppy Sep 2, 2024
42e1168
Remove github token variable & pass null for the github token
smallketchup82 Sep 2, 2024
f8a6a6a
Request restart asynchronously to avoid blocking update thread
peppy Sep 2, 2024
68e6fa2
Make comment about velopack's init a bit more loud
peppy Sep 2, 2024
cd9b822
Pass through correct update to apply when calling `WaitExitThenApplyU…
peppy Sep 2, 2024
08224b4
Simplify update process by caching pending update info and early-hand…
peppy Sep 3, 2024
b610233
Don't log probable network failures to sentry
peppy Sep 3, 2024
e564e8c
Add todo about fixing stutter on update application
peppy Sep 4, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion osu.Android/OsuGameAndroid.cs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public override void SetHost(GameHost host)
host.Window.CursorState |= CursorState.Hidden;
}

protected override UpdateManager CreateUpdateManager() => new SimpleUpdateManager();
protected override UpdateManager CreateUpdateManager() => new MobileUpdateNotifier();

protected override BatteryInfo CreateBatteryInfo() => new AndroidBatteryInfo();

Expand Down
34 changes: 9 additions & 25 deletions osu.Desktop/OsuGameDesktop.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// See the LICENCE file in the repository root for full licence text.

using System;
using System.Diagnostics;
using System.IO;
using System.Reflection;
using System.Runtime.Versioning;
Expand All @@ -19,7 +18,6 @@
using osu.Framework.Allocation;
using osu.Game.IO;
using osu.Game.IPC;
using osu.Game.Online.Multiplayer;
using osu.Game.Performance;
using osu.Game.Utils;

Expand Down Expand Up @@ -102,35 +100,21 @@ protected override UpdateManager CreateUpdateManager()
if (!string.IsNullOrEmpty(packageManaged))
return new NoActionUpdateManager();

switch (RuntimeInfo.OS)
{
case RuntimeInfo.Platform.Windows:
Debug.Assert(OperatingSystem.IsWindows());

return new SquirrelUpdateManager();

default:
return new SimpleUpdateManager();
}
return new VelopackUpdateManager();
}

public override bool RestartAppWhenExited()
{
switch (RuntimeInfo.OS)
try
{
case RuntimeInfo.Platform.Windows:
Debug.Assert(OperatingSystem.IsWindows());

// Of note, this is an async method in squirrel that adds an arbitrary delay before returning
// likely to ensure the external process is in a good state.
//
// We're not waiting on that here, but the outro playing before the actual exit should be enough
// to cover this.
Squirrel.UpdateManager.RestartAppWhenExited().FireAndForget();
return true;
Velopack.UpdateExe.Start();
Copy link
Member

Choose a reason for hiding this comment

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

This call is now blocking and will cause a stutter in the game.

We can avoid this with an assumption that the outro sequence will take longer than the updater process takes to start up:

diff --git a/osu.Desktop/OsuGameDesktop.cs b/osu.Desktop/OsuGameDesktop.cs
index 94f6ef0fc3..c75a3f0a1a 100644
--- a/osu.Desktop/OsuGameDesktop.cs
+++ b/osu.Desktop/OsuGameDesktop.cs
@@ -5,6 +5,7 @@
 using System.IO;
 using System.Reflection;
 using System.Runtime.Versioning;
+using System.Threading.Tasks;
 using Microsoft.Win32;
 using osu.Desktop.Performance;
 using osu.Desktop.Security;
@@ -18,6 +19,7 @@
 using osu.Framework.Allocation;
 using osu.Game.IO;
 using osu.Game.IPC;
+using osu.Game.Online.Multiplayer;
 using osu.Game.Performance;
 using osu.Game.Utils;
 
@@ -105,16 +107,8 @@ protected override UpdateManager CreateUpdateManager()
 
         public override bool RestartAppWhenExited()
         {
-            try
-            {
-                Velopack.UpdateExe.Start();
-                return true;
-            }
-            catch (Exception e)
-            {
-                Logger.Error(e, "Failed to restart application");
-                return base.RestartAppWhenExited();
-            }
+            Task.Run(() => Velopack.UpdateExe.Start()).FireAndForget();
+            return true;
         }
 
         protected override void LoadComplete()

Or if we actually need to wait for the process... things are going to get quite a bit more complicated.

@smoogipoo thoughts on this? I think I'd just propose the above to match what we were already doing.

Copy link
Contributor Author

@smallketchup82 smallketchup82 Sep 2, 2024

Choose a reason for hiding this comment

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

The above solution works fine. In my discussion with caesay, he implemented a restart implementation into velopack which should wait 30 seconds for the assembly to exit on its own, otherwise it will kill it. No need to worry about pacing/race conditions here.

Though, I'm a little iffy about doing FireAndForget. I would still like to handle exceptions in starting the restarter. Best not to leave the caller of the function in a state of confusion as to whether the game will actually restart or not. It's the reason I added the try..catch

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to match what we were already doing.

Copy link
Member

@peppy peppy Sep 2, 2024

Choose a reason for hiding this comment

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

No need to worry about pacing/race conditions here.

The fail case would the update process not being started in time, not the game not being killed in time. It's a different concern, but as I mentioned it's very unlikely to cause a problem. Worst case the user has to start osu! again because it didn't do so automatically.

Though, I'm a little iffy about doing FireAndForget. I would still like to handle exceptions in starting the restarter.

The method handles exceptions:

public static void FireAndForget(this Task task, Action? onSuccess = null, Action<Exception>? onError = null) =>

Best not to leave the caller of the function in a state of confusion as to whether the game will actually restart or not.

As I mentioned above, doing so will be complicated. velopack doesn't expose this as async anymore, so we'd have to use Task.Run and then handle the exit call in the continuation. This would require changing the API to accept a delegate to trigger the exit process.

Copy link

Choose a reason for hiding this comment

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

There is nothing blocking about UpdateExe.Start. there does appear to be a Thread.Sleep(300) in UpdateExe.Apply but I could refactor this to a Task.Delay

Copy link
Member

@peppy peppy Sep 2, 2024

Choose a reason for hiding this comment

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

While testing I noticed a pause on macOS which I suspect is this call. I'll have to profile to be sure, but I think Process.Start can block for short time.

Copy link
Member

Choose a reason for hiding this comment

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

@caesay profiled this. as you say, it's mostly the sleep:

JetBrains Rider 2024-09-04 at 06 50 17

Of note, the overhead of starting the process may still cause a frame stutter for users with higher refresh rates. But the sleep part is the bigger issue.

Making this flow async would probably be the best outcome.

return true;
}
catch (Exception e)
{
Logger.Error(e, "Failed to restart application");
smallketchup82 marked this conversation as resolved.
Show resolved Hide resolved
return base.RestartAppWhenExited();
}

return base.RestartAppWhenExited();
}

protected override void LoadComplete()
Expand Down
53 changes: 11 additions & 42 deletions osu.Desktop/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.IO;
using System.Runtime.Versioning;
using osu.Desktop.LegacyIpc;
using osu.Desktop.Windows;
using osu.Framework;
Expand All @@ -14,7 +13,7 @@
using osu.Game.IPC;
using osu.Game.Tournament;
using SDL;
using Squirrel;
using Velopack;

namespace osu.Desktop
{
Expand All @@ -31,19 +30,9 @@ public static class Program
[STAThread]
public static void Main(string[] args)
{
/*
Copy link
Member

@peppy peppy Sep 2, 2024

Choose a reason for hiding this comment

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

It seems a similar method is used for unit test detection, but in some basic testing I couldn't break it in the same way we broke it last time (ie. inserting a Logger.Log call before the setup call).

"needs to be run before anything else" sounds like a common sense thing, but is there an actual reason you found which makes this a requirement? If so, I'd probably want that explicitly mentioned.

Copy link
Contributor Author

@smallketchup82 smallketchup82 Sep 2, 2024

Choose a reason for hiding this comment

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

The Velopack docs suggest that you must run it before everything else. However, as you found, that's not necessarily a strict requirement. In another example on Velopack's docs, code is executed before the .Run()

As for me, I decided "So you should put setupVelopack() before everything else, but putting stuff before it isn't the end of the world as it would've been with Squirrel. So I'll tone it down a bit, and just keep the advisory since it's good practice to run it ASAP."

Though, @caesay would likely know best about this

Copy link
Member

Choose a reason for hiding this comment

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

I've made the inline comment a bit louder for now. Probably fine to continue.

* WARNING: DO NOT PLACE **ANY** CODE ABOVE THE FOLLOWING BLOCK!
*
* Logic handling Squirrel MUST run before EVERYTHING if you do not want to break it.
* To be more precise: Squirrel is internally using a rather... crude method to determine whether it is running under NUnit,
* namely by checking loaded assemblies:
* https://github.com/clowd/Clowd.Squirrel/blob/24427217482deeeb9f2cacac555525edfc7bd9ac/src/Squirrel/SimpleSplat/PlatformModeDetector.cs#L17-L32
*
* If it finds ANY assembly from the ones listed above - REGARDLESS of the reason why it is loaded -
* the app will then do completely broken things like:
* - not creating system shortcuts (as the logic is if'd out if "running tests")
* - not exiting after the install / first-update / uninstall hooks are ran (as the `Environment.Exit()` calls are if'd out if "running tests")
*/
// Velopack needs to run before anything else
setupVelo();

if (OperatingSystem.IsWindows())
{
var windowsVersion = Environment.OSVersion.Version;
Expand All @@ -66,8 +55,6 @@ public static void Main(string[] args)
return;
}
}

setupSquirrel();
}

// NVIDIA profiles are based on the executable name of a process.
Expand Down Expand Up @@ -177,32 +164,14 @@ private static bool trySendIPCMessage(IIpcHost host, string cwd, string[] args)
return false;
}

[SupportedOSPlatform("windows")]
private static void setupSquirrel()
private static void setupVelo()
{
SquirrelAwareApp.HandleEvents(onInitialInstall: (_, tools) =>
{
tools.CreateShortcutForThisExe();
tools.CreateUninstallerRegistryEntry();
WindowsAssociationManager.InstallAssociations();
}, onAppUpdate: (_, tools) =>
{
tools.CreateUninstallerRegistryEntry();
WindowsAssociationManager.UpdateAssociations();
}, onAppUninstall: (_, tools) =>
{
tools.RemoveShortcutForThisExe();
tools.RemoveUninstallerRegistryEntry();
WindowsAssociationManager.UninstallAssociations();
}, onEveryRun: (_, _, _) =>
{
// While setting the `ProcessAppUserModelId` fixes duplicate icons/shortcuts on the taskbar, it currently
// causes the right-click context menu to function incorrectly.
//
// This may turn out to be non-required after an alternative solution is implemented.
// see https://github.com/clowd/Clowd.Squirrel/issues/24
// tools.SetProcessAppUserModelId();
});
VelopackApp
.Build()
.WithFirstRun(v =>
{
if (OperatingSystem.IsWindows()) WindowsAssociationManager.InstallAssociations();
}).Run();
}
}
}
180 changes: 0 additions & 180 deletions osu.Desktop/Updater/SquirrelUpdateManager.cs

This file was deleted.

Loading
Loading