-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Changes from 19 commits
0ee8918
1025e5b
04c8df0
36a3765
fae8f5f
cae3607
c13f24d
461b791
9e01cf7
6a03092
72cf6bb
4898cff
71816c0
fcede9a
d5b5215
636ee50
a038799
b990af6
42e1168
f8a6a6a
68e6fa2
cd9b822
08224b4
b610233
e564e8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,6 @@ | |
|
||
using System; | ||
using System.IO; | ||
using System.Runtime.Versioning; | ||
using osu.Desktop.LegacyIpc; | ||
using osu.Desktop.Windows; | ||
using osu.Framework; | ||
|
@@ -14,7 +13,7 @@ | |
using osu.Game.IPC; | ||
using osu.Game.Tournament; | ||
using SDL; | ||
using Squirrel; | ||
using Velopack; | ||
|
||
namespace osu.Desktop | ||
{ | ||
|
@@ -31,19 +30,9 @@ public static class Program | |
[STAThread] | ||
public static void Main(string[] args) | ||
{ | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 "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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 As for me, I decided "So you should put Though, @caesay would likely know best about this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
setupVelopack(); | ||
|
||
if (OperatingSystem.IsWindows()) | ||
{ | ||
var windowsVersion = Environment.OSVersion.Version; | ||
|
@@ -66,8 +55,6 @@ public static void Main(string[] args) | |
return; | ||
} | ||
} | ||
|
||
setupSquirrel(); | ||
} | ||
|
||
// NVIDIA profiles are based on the executable name of a process. | ||
|
@@ -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 setupVelopack() | ||
{ | ||
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(); | ||
} | ||
} | ||
} |
This file was deleted.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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..catchThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The method handles exceptions:
osu/osu.Game/Online/Multiplayer/MultiplayerClientExtensions.cs
Line 14 in 0d7e82a
As I mentioned above, doing so will be complicated. velopack doesn't expose this as
async
anymore, so we'd have to useTask.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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
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.