-
Notifications
You must be signed in to change notification settings - Fork 32
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 to Velopack #171
Migrate to Velopack #171
Conversation
I've updated the PR description with necessary information. I'm open for any discussions on this. To keep things organized, here are some quick links to relevant branches: To get things started, I'd like for some feedback on my osu-deploy branch. |
Velopack automatically manages and optimizes the number and size of deltas
Let me know if there's anything I can do to help move this along |
I'm waiting on review. If you'd like, you can give this a review, though I'm not sure how much of a help that would be since the core team would likely want a review of their own. It'd be nice to know where this feature stands priority-wise. If we're looking at a couple months until it's given attention, I'll probably go ahead and work on something else in the meantime. |
To me this is an up-next feature. I've discussed with smoogi getting it attention sooner rather than later. |
So when I converted this to use local tools instead of global tools, it turns out that I forgot to account for the *working directory* of the commands. It was being set to the osu!.Desktop solution folder, rather than osu-deploy, resulting in an error that the vpk tool couldnt be found. This fixes that, and since the commands process absolute paths anyways, it doesnt really change anything.
This version is a stable version, unlike the pre-release we were on beforehand. It also includes all of the changes I asked for, making it a pretty seamless upgrade. I've tested a deploy using it, and it works.
I'm in the process of refactoring this entire PR because it's unreadable as it is, please hold off on pushing additional fixes for now. (it's also broken on macOS, but again, this is something I'm fixing/have fixed) |
@smoogipoo I'm going to be starting my last year of High School on September 3rd, so I'll likely be inactive from the 3rd to December (winter break). I will not be checking Discord, and I probably won't have the time to make large changes (I'll probably end up having to spread work across weeks & months rather than days). Felt like letting you know cause I do have a few PR's open (velopack & normalization), and you seem to be the one working through most of them. Since I won't be as available starting a week from now, It'd be nice if we could try to get some work done on my PR's (or just this one) before then. Let me know what you'd like to do here. I'd be okay with handing my branches over if it comes to it. |
"mac" isn't supported for `dotnet build`, and looks like vpk also uses/can use "osx".
In particular, the `--icon` argument requires a specific icon type. We already have our own prebuilt bundle with the icon _and_ other things like the plist.
Sorry for taking so long, I've been jumping around several bigger tasks and this one got delayed as a result. I'm going to take a different approach, which is to just PR changes to your fork. See smallketchup82#1 + smallketchup82#2. |
No problem, I would've been fine with waiting longer but I'm getting a little tight on schedule. I'm going to try and get this finished ASAP while I still can. PRing to my fork works, I'll give the PR's a review and test them myself. But just to make sure, are the currently open PR's on my fork all there is, or do you still have more work to do on refactoring? |
I suppose we can rename the portable package/installer post-upload since it's not used elsewhere. Will try going down that route. |
I think I'm done now, with all the latest changes PR'd to the fork. Will need to wait for a velopack release containing velopack/velopack#211, and update the signing on our deployment repo, and then this should be good to go. For testing, my fork has three releases all hooked up: https://github.com/smoogipoo/osu/releases
Will likely update the signing and do a final test deploy tomorrow. |
@smoogipoo If I'm not mistaken, can't we just use this for the time being? https://github.com/velopack/velopack/releases/tag/0.0.598-g933b2ab |
0.0.598-g933b2ab has the fix from smoogipoo in it already, I just released it. |
Oh awesome, thanks @caesay! This is ready to go. We likely won't do further testing/release until Monday at the earliest due to the complexities involved, but everything should now be in place. |
@peppy You might want to use fresh eyes to review this one. I struggled with the original code so I took the liberty of separating everything out into I've tried to keep the core logic familiar for now, so it's not entirely clean - there's still references to statics existing in I'd suggest refactoring further if required as a separate PR. |
@smoogipoo is it save to assume the OP checklist is out of date? it seems like signing is handled in https://github.com/ppy/osu-auth-client/pull/63 and |
Checklist is up to date - I haven't tested signing at all, and I also haven't tested updating on The releases on https://github.com/smoogipoo/osu/releases have x64 binaries ready to be tested, I just don't have an x64 machine around me atm. |
Okay awesome, I'll test those. |
bool wineRcEditCommand = | ||
Program.RunCommand("wine", $"\"{Path.GetFullPath("tools/rcedit-x64.exe")}\"" | ||
+ $" \"{Path.Combine(Program.StagingPath, "osu!.exe")}\"" | ||
+ $" --set-icon \"{IconPath}\"", | ||
exitOnFail: false); |
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.
Is this something specific to your operating environment @smoogipoo, or something github actions related? I know for a fact that I have wine installed nowhere.
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 is how it is in master
, dating back to 2018 or so. I don't remember the reason why this exists in the first place, but It looks like .NET is handling the app icon fine via ApplicationIcon
at least when compiled on Windows (which is what our runners do anyway). So... we can maybe remove this, but as I mentioned above I intended to keep the code familiar and not introduce too many changes.
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.
Oh okay. I checked and it looked like it didn't exist but I guess I missed it.
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.
Maybe I'm going crazy. I definitely thought I saw it there but I can't find it, and it looks like it was added in this PR (the Wine part)...
I guess it's not a big issue either way? Though I still want to explore removing all of this altogether as a followup?
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.
maybe @smallketchup82 can answer this then
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.
Documenting here for now because I was about to remove this... It appears that ApplicationIcon
sets the icon correctly for the shortcut, taskbar, and alt-tab, but the window titlebar loses its icon.
Which is weird because we have SDL code to handle that... https://github.com/ppy/osu-framework/blob/e0e945672d41c395c1977d273220cb17f9d52e2a/osu.Framework/Platform/Windows/SDL2WindowsWindow.cs#L293
Needs more investigation.
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.
maybe @smallketchup82 can answer this then
Yes I added this in. Meant to be a sort of "fallback" method when compiling for windows on a linux system. There's no equivalent unix binary for that tool (at least to my knowledge), so the best way to run it is via wine (if the user has it installed). If the user doesn't have wine installed, it'll fail gracefully and move on.
Seems to work perfectly, and removing it more or less breaks cross compilation (icon's don't get set). Best to keep it in so that the option of using it is available
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.
Velopack now has a cross-platform resource editor written in C#, as it needs to set icons on it's own Update.exe even when on Linux. I guess you could steal this and make your own small utility instead of using wine+rcedit. https://github.com/velopack/velopack/blob/develop/src/vpk/Velopack.Packaging.Windows/ResourceEdit.cs
Merging this based on local testing and code review to reduce moving pieces. Will test signing as the final step. |
Thanks for all the help! |
Codependency:
Testing / task lists
xattr -c osu\!.app
xattr -c osu\!.app
Velopack
I highly recommend reading me and caesay’s discussion in the osu!dev discord to get an idea of what’s going on here.
This is one PR (and the main PR since it contains the largest diff) in a series of PR's to migrate osu!lazer's update framework from Clowd.Squirrel to Velopack, the "next generation" of Clowd.Squirrel.
Why the migration?
Clowd.Squirrel has numerous issues that have long plagued osu!lazer and lazer's development as a whole. Velopack has the following differences compared to Clowd.Squirrel (not sure if they're differences, but it is what it is):
current
which is a symlink to the most recent release. This allows features like Windows Associations to not require updating each time the game launches.All of these would allow for a much simpler osu-deploy, a much cleaner osu!lazer, more opportunities in the future (release channels) and a better user experience for the end user.
So how's it going to work?
I'm going to be basing my osu-deploy branch off of caesay’s branch, while keeping it in line with recent osu-deploy updates and updates to velopack. The same can be said for my osu!lazer branch, though I unintentionally reimplemented caesay's branch instead of basing it on his branch.
Velopack is more or less a drop in replacement for Clowd.Squirrel, so all that really needs to be done is to remove lines and change existing lines to use Velopack instead. Velopack will automatically manage reading from Squirrel's RELEASE files and migrating clients onto Velopack, so there is no work needed to be done on that front.
Both me and caesay want to discuss things before doing things, so as to not waste effort. Which is why this PR, while it is in it's draft phase, will be the main place for discussion on Velopack migration, design choices, considerations, etc.
Differences from caesay's branch
GithubReleaseHelper
Final notes
Not much to say here. Maintainers can edit the branch as needed. @caesay if you want to make changes to my branch, I'm okay with either a PR on my fork or a git patch. This should close #125 as a side effect (as vpk pack will output a nupkg, removing the need for nuget.exe). And this will make all open PR's obsolete.