Add workspace to MiniInstaller#1037
Conversation
Wartori54
left a comment
There was a problem hiding this comment.
Just some polish needed here and there.
About your fast mode question, I'd be fine with merging without any changes to fast mode, it is meant for advanced users only, so if the installation breaks the user will notice that and be able to fix it.
|
Thanks @Wartori54 for the feedback, all comments are addressed in the latest commit. |
Wartori54
left a comment
There was a problem hiding this comment.
Other than this LGTM. Would appreciate some testing on other platforms before merging though.
|
The latest commit uses |
microlith57
left a comment
There was a problem hiding this comment.
code looks good, just a few missing param names (non-blocking). still would be good to test on windows/macos :p
| public static void MoveExecutable(string srcPath, string dstPath) { | ||
| File.Delete(dstPath); | ||
| File.Move(srcPath, dstPath); | ||
| File.Move(srcPath, dstPath, true); |
There was a problem hiding this comment.
nitpick: (for readability)
| File.Move(srcPath, dstPath, true); | |
| File.Move(srcPath, dstPath, overwrite: true); |
(& similarly for the other File.Move calls below)
| string steamworksLibDst = Path.Combine(Globals.PathGame, "Steamworks.NET.dll"); | ||
| File.Delete(steamworksLibDst); | ||
| File.Copy(steamworksLibSrc, steamworksLibDst); | ||
| File.Copy(steamworksLibSrc, steamworksLibDst, true); |
There was a problem hiding this comment.
nitpick: (for readability)
| File.Copy(steamworksLibSrc, steamworksLibDst, true); | |
| File.Copy(steamworksLibSrc, steamworksLibDst, overwrite: true); |
|
|
||
| if (Directory.Exists(Globals.PathMiniInstallerWorkspace)) { | ||
| Logger.LogLine("MiniInstaller workspace already exists, cleaning before continuing."); | ||
| Directory.Delete(Globals.PathMiniInstallerWorkspace, true); |
There was a problem hiding this comment.
nitpick: (readability)
| Directory.Delete(Globals.PathMiniInstallerWorkspace, true); | |
| Directory.Delete(Globals.PathMiniInstallerWorkspace, recursive: true); |
(& similarly for the other recursive Directory.Delete)
|
The pull request was approved and entered the 3-day last-call window. |
SnipUndercover
left a comment
There was a problem hiding this comment.
Adding a "request changes" review just so that this doesn't get accidentally merged without testing, will turn into an approval when it's done.
|
Parameter names have been added in the latest commit. |
33be988 to
186809b
Compare
There was a problem hiding this comment.
Code-wise looks good, but upon testing MiniInstaller crashes when patching FNA. It attempts to resolve the Celeste assembly, but it finds Celeste.exe which is a Piton apphost on already modded installs. MonoMod then tries to read that as a managed .NET assembly and promptly crash.
MonoMod does not consider the coreified vanilla Celeste copy in the workspace, as it is named Celeste.Core.dll instead of Celeste.dll.
I cannot commit to your branch, so I have sent a patch file in #modding_dev on the Celeste Discord server.
I tested it by running MiniInstaller twice from a fresh copy of Celeste and confirmed it works.
|
Also this doesn't really solve the problem of |
|
From my testing, @SnipUndercover patches work on Linux as well. |
This PR adds a workspace folder to the MiniInstaller installation process, so it doesn't overwrite
Celeste.exeearly (for example, during the step applying Everest patches, after coreification).The workspace is used to store the
Celeste.exe/Celeste.dllfiles, until after the step applying Everest patches, reducing the time window during which the process can be interrupted and left in a state which would alloworig/Celeste.exeto be coreified.This is a work in progress, the following still needs to be done:
Fixes #983.
Fixes #984.
As far as I understand, #984 occurs only because of #983.