Migrate update framework to Velopack #28743
Conversation
Earlier I changed the restarting logic to not wait until the program exits and instead try to facilitate restarting alone. This did not work, and it became clear we'd need Velopack to do the restarting. This reverts back and supposedly brings restarting logic in line with how Velopack does it
Velopack won't install the updates while the program is open, it'll do it in between restarts or before starting.
No longer needed. Velopack supports the platforms that this covers for
Also better address UpdateManager conflict
|
What are the todos in the OP supposed to mean? Is this ready for review or not? |
It's ready for review. The todo's remaining are mostly for future reference (things I'll want to add in future PR's, PR's that'll be borked because of this PR, etc). The last todo in specific will need a bit of discussion, but it shouldn't hinder what we already have, so I decided to leave it to be addressed in review. I asked a question about it earlier in the osu!dev discord:
Apart from that concern everything is ready and good to go. |
While removing the desktop specific logic from it
|
Addressed the last concern (see for conversation). Should be good for review now. |
There was a problem hiding this comment.
Lgtm! Can be tested using releases at https://github.com/smoogipoo/osu/releases.
|
@smoogipoo The velopack version on this has not been updated to reflect ppy/osu-deploy@6829172 I'll update it myself later today or tomorrow, but if you want to quickly do it now feel free. |
|
It doesn't really matter, but feel free. |
|
Bumped the version to match osu-deploy. You might want to do another quick test & reapprove if everything looks fine. Though, it should be fine given it builds. |
|
Have updated the releases in https://github.com/smoogipoo/osu/releases, all looks good. |
| @@ -31,19 +30,9 @@ public static class Program | |||
| [STAThread] | |||
| public static void Main(string[] args) | |||
| { | |||
| /* | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I've made the inline comment a bit louder for now. Probably fine to continue.
osu.Desktop/OsuGameDesktop.cs
Outdated
| // to cover this. | ||
| Squirrel.UpdateManager.RestartAppWhenExited().FireAndForget(); | ||
| return true; | ||
| Velopack.UpdateExe.Start(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think it's fine to match what we were already doing.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
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.
@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.
Just a warning that sometimes things do break if you are using different versions of the Velopack NuGet and VPK. I try to minimize this of course, but as an example I changed the default behavior of Update.exe recently from "don't restart by default, require --restart to turn on" to "restart by default and turn off with --norestart". If you were using a mismatched package version at that time, your app may not/restart as you expect. The VPK tool will emit a warning if the version is not the same but you might not catch that in CI. Just keep that in mind! |
|
|
||
| private bool restartToApplyUpdate() | ||
| { | ||
| updateManager.WaitExitThenApplyUpdates(null); |
There was a problem hiding this comment.
Really need to provide an UpdateInfo here instead of null, the same one you passed to DownloadUpdatesAsync.
There was a problem hiding this comment.
@caesay how should we handle the case where UpdateInfo came back null but there's a pending update?
There was a problem hiding this comment.
Based on the xmldoc it seems like passing null is fine in this case "The target release to apply. Can be left null to auto-apply the newest downloaded release."
Thanks for good documentation!
There was a problem hiding this comment.
Maybe I should refactor IsUpdatePendingRestart to return a VelopackAsset so it can be passed in. Null will probably work most of the time but it's not best practice.
If you pass null, Velopack doesn't know for sure what update you actually want to apply, so it will just go searching in the packages directory for the latest version. Sometimes this is not what you want. For example, Velopack supports updating to older versions now - both explicitly and implicitly eg. it can automatically downgrade if you've retracted a release from your feed because it has breaking bugs as long as the UpdateOptions.AllowVersionDowngrade is set to true.
So if you are downgrading a version but have not provided it to the apply/restart then you end up not installing it.
Also, DownloadUpdates should be a no-op if the release is already downloaded.
My suggestion is you should not need to check IsUpdatePendingRestart except for on startup (but VelopackApp already does this for you by default.. see SetAutoApply there).
You can just CheckForUpdates, if returns not null, call DownloadUpdates and then cache the UpdateInfo in a local field. If you have the local field non-null when a restart is requested then you know what to pass to Velopack.. If your local UpdateInfo is null, then you've not downloaded a new version yet.
There was a problem hiding this comment.
It does restart and apply on startup yes. On windows there is a progress dialog, nothing for mac/linux but I'm working on that. I recently just finished a cross-platform progress/dialog library https://github.com/velopack/xdialog-rs but it has not yet been integrated to Velopack.
There was a problem hiding this comment.
On a faster PC this should only take a second or two, since it's just unzipping the nupkg. On a slower PC with a big package it's a bit unfortunate without the progress bar there.
There was a problem hiding this comment.
You can just CheckForUpdates, if returns not null, call DownloadUpdates and then cache the UpdateInfo in a local field. If you have the local field non-null when a restart is requested then you know what to pass to Velopack.. If your local UpdateInfo is null, then you've not downloaded a new version yet.
Could there be a case where a second check-for-updates finds a newer update, where we'd want to use the newer UpdateInfo? Or is that not something that can happen in the update flow?
There was a problem hiding this comment.
It depends on how you implement things. CheckForUpdates will always return the latest update available. If you have already downloaded an update and another release has happened you can do another DownloadUpdates to grab the fresh version again.
There was a problem hiding this comment.
Okay good to know, thanks.
With my latest changes on this branch I bypassed that, but we'll probably add back support for it for fun.
|
The package version has since been updated in line with |
|
@smallketchup82 please don't resolve review threads, we leave these unresolved if they are still applicable (and should be read by other reviewers). |
Changes
SimpleUpdateManagertoMobileUpdateNotifierSquirrelUpdateManagerVelopackUpdateManagerit in between restarts or before starting. Therefore the only notification text we need is
StartDownload,FailDownload, etcTODO
osu/osu.iOS/OsuGameIOS.cs
Line 18 in 9e01cf7
osu/osu.Android/OsuGameAndroid.cs
Line 83 in f8a2b07
Next up
NOTE: I haven't tested the update logic because of the difficulty of doing that, and the fact that bugs here would really only show themselves in a production release environment. I have tested the restarting logic and it works.
Read ppy/osu-deploy#171 for information on migration to Velopack