Repository navigation
Updater fixes, and drop Newtonsoft.Json - #486
Merged
Merged
Conversation
GetLatestVersionInfo added the Accept and User-Agent headers to the shared HttpClient on every call, so each time the About box checked for updates the headers grew another copy. Set them when the client is created.
DownloadAsync didn't check the response status, so a 404 or 500 page was written to the update zip and only failed later as a hash mismatch or a broken archive. Throw on a non-success status instead.
RunCommandAsync redirected stderr but only read stdout. If winget wrote more to stderr than the pipe buffer holds, it would block and WaitForExit would never return. Stderr isn't used, so stop redirecting it.
Newtonsoft.Json was only used to read three fields from the GitHub release response, but it is embedded in Rapr.exe and adds about 700 KB. Read the response with DataContractJsonSerializer, which is part of .NET Framework. The parsing moves into ParseLatestRelease so it can be tested without network access. Behavior is unchanged.
The update check used assets[0]. That only works while the release has a single asset; adding anything else (a checksum file, symbols) could make the updater download the wrong file. Look for the DriverStoreExplorer-*.zip that release.yml uploads.
The downloaded zip was only checked when GitHub's release response included a digest for it; without one, it was installed unchecked. Since the app runs elevated, stop before downloading when there is no digest. Releases published since GitHub added asset digests always have one.
Since lostindark#483 was merged, the GitHub Actions test step crashed twice with the test host dying inside a WinForms window callback (NativeWindow.Callback -> WndProcException -> OnThreadException), once with an OutOfMemoryException and once with an AccessViolationException. No test failed; the same commit passes all tests on AppVeyor. DSEFormTests is the only class that creates controls and pumps window messages, and it ran in parallel with the other test classes. Mark it DoNotParallelize so it runs after them on its own.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A few fixes to the self-updater, one commit each:
DownloadAsyncdidn't check the HTTP status, so a 404 page could be saved as the zip. It now throws.DataContractJsonSerializer. Newtonsoft was only used to read three fields of the GitHub response and made Rapr.exe about 700 KB bigger. The parsing moved toParseLatestReleaseso it can be tested without network access.DriverStoreExplorer-*.zip) instead ofassets[0], so adding another file to a release can't make the updater download the wrong one.There are tests for the parsing, the asset selection, the missing digest and the HTTP error.
I didn't add Authenticode verification of the new exe. I'd want to test it against a real signed release first, but I'm happy to do that as a follow-up if you want it.