Skip to content
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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

winterheart
Copy link
Contributor

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.

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.
Copy link
Contributor

@slxdy slxdy left a 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.

MelonLoader.Installer/MLVersion.cs Show resolved Hide resolved
MelonLoader.Installer/MLVersion.cs Show resolved Hide resolved
MelonLoader.Installer/MLVersion.cs Show resolved Hide resolved
Comment on lines +99 to +126
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)";
}
Copy link
Contributor

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));
Copy link
Contributor

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;
Copy link
Contributor

@slxdy slxdy Dec 15, 2024

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

@slxdy
Copy link
Contributor

slxdy commented Dec 15, 2024

Also, this PR should be retargeted towards the development branch. Please read the contribution section in the readme.

@winterheart
Copy link
Contributor Author

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.

@slxdy
Copy link
Contributor

slxdy commented Dec 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants