0neGal / viper

Launcher+Updater for TF|2 Northstar
https://0negal.github.io/viper
GNU General Public License v3.0
151 stars 21 forks source link

How does Viper handle GH release with missing `Northstar.release.vX.Y.Z.zip`? #178

Closed GeckoEidechse closed 1 year ago

GeckoEidechse commented 1 year ago

I'm planning to overhaul the release process for Northstar to be able to push out releases faster. Currently there's like 10 different steps I have to go through when I make a release taking me at least half an hour to complete.

As part of this I'd change release creation from building on tag to building on release. Currently I have to manually download CI build and then re-upload to release page. Naturally this is quite stupid when I can just setup GitHub to upload to release page ^^

As compile times are a thing however, this means there will be a few minutes where the GitHub release does not have a corresponding release zip file.

AFAIK Viper is the only major mod manager that still grabs the release builds from GitHub as opposed to Thunderstore and so I wanted to check whether Viper can cleanly handle missing ZIP file or whether a user opening Viper while the release is still being built will be greeted with a random error message.

GeckoEidechse commented 1 year ago

Note that I am NOT planning to stop doing release zip on GitHub, so using GitHub as download source is still fine ^^ (with proper CI setup I can get uploading releases to GitHub "for free" so I don't see a need to stop uploading them :P)

0neGal commented 1 year ago

As compile times are a thing however, this means there will be a few minutes where the GitHub release does not have a corresponding release zip file.

Is there a good reason to make the release as a draft initially, then publishing it once compiled?

and so I wanted to check whether Viper can cleanly handle missing ZIP file

I dont see any way it would, it'd likely error out when trying to download, and simply not finish the update, the exact outcome I can't say without testing it...

Unless the write stream emits a finish event on an error then I dont see it doing any harm beyond canceling out.

Although I do realize now we don't actually have error handling for the HTTP request, so there might still be some unexpected behavior, I'll look into that...

AFAIK Viper is the only major mod manager that still grabs the release builds from GitHub as opposed to Thunderstore

Note that I am NOT planning to stop doing release zip on GitHub, so using GitHub as download source is still fine ^^

I will point out that I do plan to still add Thunderstore support, and use it by default (unless there's a reason to not use it by default?)

GeckoEidechse commented 1 year ago

Is there a good reason to make the release as a draft initially, then publishing it once compiled?

The reason is that last time I tried with setting draft first (this was when testing with FlightCore) I couldn't get it to trigger CI on compile, only on actual release.

If anyone knows how to adjust the the CI config to already compile on draft, I'm more than happy to take that PR but personally I'm not willing to delay improving release pipeline for what is essentially not an issue in upstream Northstar ^^"

 

I dont see any way it would, it'd likely error out when trying to download, and simply not finish the update, the exact outcome I can't say without testing it...

Unless the write stream emits a finish event on an error then I dont see it doing any harm beyond canceling out.

Although I do realize now we don't actually have error handling for the HTTP request, so there might still be some unexpected behavior, I'll look into that...

Aight. My basic train of thought was to just add edge case handling where basically you just first check whether a link exists or 404s and then just throw a warning like "Update check currently not available, please try again later" or just silent fail altogether. Given that it's a timespan of like 5-ish minutes in which a GitHub release would be up without corresponding zip attached, silent failing or showing some warning is fine. I just want to avoid the case of crash looping on a missing link. :P

 

I will point out that I do plan to still add Thunderstore support, and use it by default (unless there's a reason to not use it by default?)

I'm well aware of that and looking forward to it. Though depending on the time it takes to develop, test, and release the change, adding some edge case handling for missing zip link is probably the easy solution in the short run ^^

0neGal commented 1 year ago

Given that it's a timespan of like 5-ish minutes in which a GitHub release would be up without corresponding zip attached, silent failing or showing some warning is fine. I just want to avoid the case of crash looping on a missing link. :P

Just tested it, it seems that what I hoped wouldn't happen, does happen, if the HTTP request https.get() returns an error it doesn't stop at any point, and in fact keeps going with the stream, and does emit the finished event, leading to it trying to unzip a file that, instead of containing the archive, actually contains the HTML for GitHub's 404 page.

Neato...

Overall, I'd consider this a silent error, it doesn't cause any issues as the unzipper silently fails when trying to parse the zip, for obvious reasons... However the end event that the unzipper emits still gets emitted when this happens, so the console will still print out Installation/Update finished!, very funky.

I'll have a look at fixing this together with #177 whilst refactoring the update process.

I must say however, the devtools make it seem like the NS team discovered some high grade compression... ``` main.js:129 Downloading... 0.0mb main.js:129 Downloading... 0.1mb main.js:129 Downloading... 0.2mb main.js:96 Extracting update... main.js:129 Extracting update... main.js:129 Extracting update... 31% main.js:129 Extracting update... 62% main.js:129 Extracting update... 93% main.js:129 Extracting update... 100% main.js:129 Up-to-date main.js:96 Done! Ready to play! ```

adding some edge case handling for missing zip link is probably the easy solution in the short run ^^

Agreed!

0neGal commented 1 year ago

This is now fixed in the latest release (v1.7.1), if we don't get a 200 status code we abort the update, instead of continuing on.