From d2fdbec41dd664fb6cfc66ec6c79ee9f78eb8c7b Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 5 Aug 2022 21:26:02 -0300 Subject: [PATCH] fix: move file deletion to the end of the instance update This makes it harder for problems in the updating process to affect the current instance. Network issues, for instance, will no longer put the instance in an invalid state. Still, a possible improvement to this would be passing that logic to InstanceStaging instead, to be handled with the instance commiting directly. However, as it is now, the code would become very spaguetti-y, and given that the override operation in the commiting could also put the instance into an invalid state, it seems to me that, in order to fully error-proof this, we would need to do a copy operation on the whole instance, in order to modify the copy, and only in the end override everything an once with a rename. That also has the possibility of corrupting the instance if done without super care, however, so I think we may need to instead create an automatic backup system, with an undo command of sorts, or something like that. This doesn't seem very trivial though, so it'll probably need to wait until another PR. In the meantime, the user is advised to always backup their instances before doing this kind of action, as always. What a long commit message o.O Signed-off-by: flow --- launcher/InstanceCreationTask.cpp | 41 ++++++++++++++----- launcher/InstanceCreationTask.h | 2 + .../flame/FlameInstanceCreationTask.cpp | 25 ++++++----- .../modrinth/ModrinthInstanceCreationTask.cpp | 22 ++++++---- 4 files changed, 61 insertions(+), 29 deletions(-) diff --git a/launcher/InstanceCreationTask.cpp b/launcher/InstanceCreationTask.cpp index a82bdfe8..1b1a8c15 100644 --- a/launcher/InstanceCreationTask.cpp +++ b/launcher/InstanceCreationTask.cpp @@ -1,6 +1,7 @@ #include "InstanceCreationTask.h" #include +#include InstanceCreationTask::InstanceCreationTask() = default; @@ -19,19 +20,37 @@ void InstanceCreationTask::executeTask() return; } - // If this is set, it means we're updating an instance. Since the previous step likely - // removed some old files, we'd better not let the user abort the next task, since it'd - // put the instance in an invalid state. - // TODO: Figure out an unexpensive way of making such file removal a recoverable transaction. - setAbortStatus(!shouldOverride()); + if (!createInstance()) { + if (m_abort) + return; - if (createInstance()) { - emitSucceeded(); + qWarning() << "Instance creation failed!"; + if (!m_error_message.isEmpty()) + qWarning() << "Reason: " << m_error_message; + emitFailed(tr("Error while creating new instance.")); return; } - qWarning() << "Instance creation failed!"; - if (!m_error_message.isEmpty()) - qWarning() << "Reason: " << m_error_message; - emitFailed(tr("Error while creating new instance.")); + // If this is set, it means we're updating an instance. So, we now need to remove the + // files scheduled to, and we'd better not let the user abort in the middle of it, since it'd + // put the instance in an invalid state. + if (shouldOverride()) { + setAbortStatus(false); + setStatus(tr("Removing old conflicting files...")); + qDebug() << "Removing old files"; + + for (auto path : m_files_to_remove) { + if (!QFile::exists(path)) + continue; + qDebug() << "Removing" << path; + if (!QFile::remove(path)) { + qCritical() << "Couldn't remove the old conflicting files."; + emitFailed(tr("Failed to remove old conflicting files.")); + return; + } + } + } + + emitSucceeded(); + return; } diff --git a/launcher/InstanceCreationTask.h b/launcher/InstanceCreationTask.h index 9b51c448..03ee1a7a 100644 --- a/launcher/InstanceCreationTask.h +++ b/launcher/InstanceCreationTask.h @@ -39,6 +39,8 @@ class InstanceCreationTask : public InstanceTask { protected: bool m_abort = false; + QStringList m_files_to_remove; + private: QString m_error_message; }; diff --git a/launcher/modplatform/flame/FlameInstanceCreationTask.cpp b/launcher/modplatform/flame/FlameInstanceCreationTask.cpp index 5acb6300..e3521a38 100644 --- a/launcher/modplatform/flame/FlameInstanceCreationTask.cpp +++ b/launcher/modplatform/flame/FlameInstanceCreationTask.cpp @@ -120,15 +120,17 @@ bool FlameCreationTask::updateInstance() files_iterator++; } - QString old_minecraft_dir(inst->gameRoot()); + QDir old_minecraft_dir(inst->gameRoot()); // We will remove all the previous overrides, to prevent duplicate files! // TODO: Currently 'overrides' will always override the stuff on update. How do we preserve unchanged overrides? // FIXME: We may want to do something about disabled mods. auto old_overrides = Override::readOverrides("overrides", old_index_folder); for (auto entry : old_overrides) { - qDebug() << "Removing" << entry; - old_minecraft_dir.remove(entry); + if (entry.isEmpty()) + continue; + qDebug() << "Scheduling" << entry << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(entry)); } // Remove remaining old files (we need to do an API request to know which ids are which files...) @@ -143,7 +145,7 @@ bool FlameCreationTask::updateInstance() QEventLoop loop; - connect(job, &NetJob::succeeded, this, [raw_response, fileIds, old_inst_dir, &old_files, old_minecraft_dir] { + connect(job, &NetJob::succeeded, this, [this, raw_response, fileIds, old_inst_dir, &old_files, old_minecraft_dir] { // Parse the API response QJsonParseError parse_error{}; auto doc = QJsonDocument::fromJson(*raw_response, &parse_error); @@ -180,10 +182,9 @@ bool FlameCreationTask::updateInstance() if (file.fileName.isEmpty() || file.targetFolder.isEmpty()) continue; - qDebug() << "Removing" << file.fileName << "at" << file.targetFolder; - QString path(FS::PathCombine(old_minecraft_dir, file.targetFolder, file.fileName)); - if (!QFile::remove(path)) - qDebug() << "Failed to remove file at" << path; + QString relative_path(FS::PathCombine(file.targetFolder, file.fileName)); + qDebug() << "Scheduling" << relative_path << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(relative_path)); } }); connect(job, &NetJob::finished, &loop, &QEventLoop::quit); @@ -334,14 +335,17 @@ bool FlameCreationTask::createInstance() loop.exec(); - if (m_instance) { + bool did_succeed = getError().isEmpty(); + + if (m_instance && did_succeed) { + setAbortStatus(false); auto inst = m_instance.value(); inst->copyManagedPack(instance); inst->setName(instance.name()); } - return getError().isEmpty(); + return did_succeed; } void FlameCreationTask::idResolverSucceeded(QEventLoop& loop) @@ -421,7 +425,6 @@ void FlameCreationTask::setupDownloadJob(QEventLoop& loop) m_mod_id_resolver.reset(); connect(m_files_job.get(), &NetJob::succeeded, this, [&]() { m_files_job.reset(); - emitSucceeded(); }); connect(m_files_job.get(), &NetJob::failed, [&](QString reason) { m_files_job.reset(); diff --git a/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp b/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp index a0c67876..7d19639c 100644 --- a/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp +++ b/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp @@ -105,12 +105,15 @@ bool ModrinthCreationTask::updateInstance() } QDir old_minecraft_dir(inst->gameRoot()); + // Some files were removed from the old version, and some will be downloaded in an updated version, // so we're fine removing them! if (!old_files.empty()) { for (auto const& file : old_files) { - qDebug() << "Removing" << file.path; - old_minecraft_dir.remove(file.path); + if (file.path.isEmpty()) + continue; + qDebug() << "Scheduling" << file.path << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(file.path)); } } @@ -119,14 +122,18 @@ bool ModrinthCreationTask::updateInstance() // FIXME: We may want to do something about disabled mods. auto old_overrides = Override::readOverrides("overrides", old_index_folder); for (auto entry : old_overrides) { - qDebug() << "Removing" << entry; - old_minecraft_dir.remove(entry); + if (entry.isEmpty()) + continue; + qDebug() << "Scheduling" << entry << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(entry)); } auto old_client_overrides = Override::readOverrides("client-overrides", old_index_folder); for (auto entry : old_overrides) { - qDebug() << "Removing" << entry; - old_minecraft_dir.remove(entry); + if (entry.isEmpty()) + continue; + qDebug() << "Scheduling" << entry << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(entry)); } } @@ -244,7 +251,8 @@ bool ModrinthCreationTask::createInstance() loop.exec(); - if (m_instance) { + if (m_instance && ended_well) { + setAbortStatus(false); auto inst = m_instance.value(); inst->copyManagedPack(instance);