-
Notifications
You must be signed in to change notification settings - Fork 28
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
Enable caching of downloading MelonLoader archive #38
base: master
Are you sure you want to change the base?
Conversation
Currently MelonLoader supports Linux x64, Windows x64 and Windows x86. Enum Architecture simplifies logic on arch detection.
Cache successfully downloaded archive to speedup next installations.
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 implementation for architectures is great, but still needs some changes. This change should've been mentioned in the PR however, as it deviates from the original goal.
The checksum is unnecessary, as mentioned in the review details. I do not agree with this change.
The cache system is not great, as it assumes the user will redownload the same version multiple times. For an average user, a version is only downloaded once, which makes caching only a waste of space, so I do not agree with this change either.
Overall, I think this PR should be renamed and only include the architecture change.
public static async Task<string?> DownloadFileAsync(string url, Stream destination, bool useCache, InstallProgressEventHandler? onProgress) | ||
{ | ||
// Get archive | ||
var result = await FetchFile(url, destination, useCache, onProgress); | ||
if (result != null) | ||
return $"Failed to fetch file from {url}: {result}"; | ||
|
||
destination.Seek(0, SeekOrigin.Begin); | ||
|
||
// Get checksum | ||
var checksumUrl = url.Replace(".zip", ".sha512"); | ||
using var checksumStr = new MemoryStream(); | ||
result = await FetchFile(checksumUrl, checksumStr, useCache, onProgress); | ||
|
||
// Checksum fetch failed, skip verification | ||
if (result != null) return null; | ||
|
||
var checksumDownload = System.Text.Encoding.UTF8.GetString(checksumStr.ToArray()); | ||
var checksumCompute = Convert.ToHexString(await Hasher.ComputeHashAsync(destination)); | ||
|
||
// Verification successful | ||
if (checksumCompute == checksumDownload) return null; | ||
// Verification failed, remove corrupted files | ||
var parentDirectory = Path.GetFileName(Path.GetDirectoryName(url)) ?? ""; | ||
File.Delete(Path.Combine(Config.CacheDir, "Cache", parentDirectory, Path.GetFileName(url))); | ||
File.Delete(Path.Combine(Config.CacheDir, "Cache", parentDirectory, Path.GetFileName(checksumUrl))); | ||
return "Fetched corrupted file (checksum mismatch)"; | ||
} |
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 assumes that the downloaded file is a zip and that a hash is present. The purpose of this function is to download any file the caller may request. The checksum seems unnecessary, as the only possibility of a "corruption" would be a returned error message (which should be indicated by the response status code).
{ | ||
// Cache preparation | ||
var parentDirectory = Path.GetFileName(Path.GetDirectoryName(url)) ?? ""; | ||
var fileCache = Path.Combine(Config.CacheDir, "Cache", parentDirectory, Path.GetFileName(url)); |
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 assumes the file name is unique for every download, which already breaks everything. ML versions do not have unique file names per version, so only one version will be cached.
public bool Is32Bit => is32Bit; | ||
public bool IsLinux => isLinux; | ||
public Architecture Arch => architecture; | ||
public bool IsLinux => architecture == Architecture.LinuxX64; |
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 IsLinux
property seems unnecessary now, so it can be removed
Also, this PR should be retargeted towards the development branch. Please read the contribution section in the readme. |
I've splitted architecture implementation into PR #40 . As this feature depends on it, I'll make it draft for a while. I'm using MelonLoader frequently and have multiple mods in different games. On some day I decided to update ML on all games to latest version. Without this features I forced to re-download archive each time and eventually got blocked by GitHub firewall for frequent downloading. I had to wait 10 minutes or so for the update to proceed. So why this PR is born. |
I see. I think a better solution would be to make a temporary cache, which gets wiped on exit. However, the cache should be managed by MLManager, not the downloader. For example, when selecting a manual zipped build, the zip is instantly extracted and cached until it has to be moved to a game. It is stored in a temp folder and removed on exit. |
Currently MelonLoader.Installer fetches installation archive every time on installation event. This can be slow and user can restricted by download throttling from GitHub. This feature enables caching of successfully downloaded archives for possible re-use with optional SHA512 checksum verification.
Cache files are stored in
CacheDir / Cache
directory.