From 15ec1abb6a3acb77b36f14d3ddccc97a8df8c8e1 Mon Sep 17 00:00:00 2001 From: flow Date: Sat, 23 Jul 2022 23:13:53 -0300 Subject: feat: use QIODevice for calcuating the JAR hash on Modrinth Signed-off-by: flow --- launcher/modplatform/EnsureMetadataTask.cpp | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) (limited to 'launcher/modplatform/EnsureMetadataTask.cpp') diff --git a/launcher/modplatform/EnsureMetadataTask.cpp b/launcher/modplatform/EnsureMetadataTask.cpp index 60c54c4e..a5c9cbca 100644 --- a/launcher/modplatform/EnsureMetadataTask.cpp +++ b/launcher/modplatform/EnsureMetadataTask.cpp @@ -50,21 +50,29 @@ EnsureMetadataTask::EnsureMetadataTask(QList& mods, QDir dir, ModPlatform: QString EnsureMetadataTask::getHash(Mod* mod) { /* Here we create a mapping hash -> mod, because we need that relationship to parse the API routes */ - QByteArray jar_data; - try { - jar_data = FS::read(mod->fileinfo().absoluteFilePath()); - } catch (FS::FileSystemException& e) { - qCritical() << QString("Failed to open / read JAR file of %1").arg(mod->name()); - qCritical() << QString("Reason: ") << e.cause(); - + if (mod->type() == Mod::MOD_FOLDER) return {}; - } + QString result; switch (m_provider) { case ModPlatform::Provider::MODRINTH: { + QFile file(mod->fileinfo().absoluteFilePath()); + + try { + file.open(QFile::ReadOnly); + } catch (FS::FileSystemException& e) { + qCritical() << QString("Failed to open JAR file of %1").arg(mod->name()); + qCritical() << QString("Reason: ") << e.cause(); + + return {}; + } + auto hash_type = ProviderCaps.hashType(ModPlatform::Provider::MODRINTH).first(); + result = ProviderCaps.hash(ModPlatform::Provider::MODRINTH, &file, hash_type); - return QString(ProviderCaps.hash(ModPlatform::Provider::MODRINTH, jar_data, hash_type).toHex()); + file.close(); + + break; } case ModPlatform::Provider::FLAME: { QByteArray jar_data_treated; @@ -78,7 +86,7 @@ QString EnsureMetadataTask::getHash(Mod* mod) } } - return {}; + return result; } bool EnsureMetadataTask::abort() -- cgit From b1763353ea0fd2d1924e3560f0a674cb6260721b Mon Sep 17 00:00:00 2001 From: flow Date: Sat, 23 Jul 2022 23:16:28 -0300 Subject: feat: do incremental calculation of CF's hash Signed-off-by: flow --- launcher/modplatform/EnsureMetadataTask.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'launcher/modplatform/EnsureMetadataTask.cpp') diff --git a/launcher/modplatform/EnsureMetadataTask.cpp b/launcher/modplatform/EnsureMetadataTask.cpp index a5c9cbca..617cbe18 100644 --- a/launcher/modplatform/EnsureMetadataTask.cpp +++ b/launcher/modplatform/EnsureMetadataTask.cpp @@ -75,14 +75,13 @@ QString EnsureMetadataTask::getHash(Mod* mod) break; } case ModPlatform::Provider::FLAME: { - QByteArray jar_data_treated; - for (char c : jar_data) { + auto should_filter_out = [](char c) { // CF-specific - if (!(c == 9 || c == 10 || c == 13 || c == 32)) - jar_data_treated.push_back(c); - } + return (c == 9 || c == 10 || c == 13 || c == 32); + }; - return QString::number(MurmurHash2(jar_data_treated, jar_data_treated.length())); + std::ifstream file_stream(mod->fileinfo().absoluteFilePath().toStdString(), std::ifstream::binary); + result = QString::number(MurmurHash2(std::move(file_stream), 4096, should_filter_out)); } } -- cgit From e6f2a3893a6c9b3c0a2b94ffde41a7dbee1accb0 Mon Sep 17 00:00:00 2001 From: flow Date: Sun, 24 Jul 2022 15:13:42 -0300 Subject: refactor+feat: improve code separation in ensure metadata ... and avoid calculating the same hash multiple times Signed-off-by: flow --- launcher/modplatform/EnsureMetadataTask.cpp | 99 ++++++++++++----------------- launcher/modplatform/EnsureMetadataTask.h | 21 ++++-- 2 files changed, 58 insertions(+), 62 deletions(-) (limited to 'launcher/modplatform/EnsureMetadataTask.cpp') diff --git a/launcher/modplatform/EnsureMetadataTask.cpp b/launcher/modplatform/EnsureMetadataTask.cpp index 617cbe18..1f49d7c9 100644 --- a/launcher/modplatform/EnsureMetadataTask.cpp +++ b/launcher/modplatform/EnsureMetadataTask.cpp @@ -3,89 +3,72 @@ #include #include -#include "FileSystem.h" #include "Json.h" + #include "minecraft/mod/Mod.h" #include "minecraft/mod/tasks/LocalModUpdateTask.h" + #include "modplatform/flame/FlameAPI.h" #include "modplatform/flame/FlameModIndex.h" #include "modplatform/modrinth/ModrinthAPI.h" #include "modplatform/modrinth/ModrinthPackIndex.h" + #include "net/NetJob.h" -#include "tasks/MultipleOptionsTask.h" static ModPlatform::ProviderCapabilities ProviderCaps; static ModrinthAPI modrinth_api; static FlameAPI flame_api; -EnsureMetadataTask::EnsureMetadataTask(Mod* mod, QDir dir, ModPlatform::Provider prov) : Task(nullptr), m_index_dir(dir), m_provider(prov) +EnsureMetadataTask::EnsureMetadataTask(Mod* mod, QDir dir, ModPlatform::Provider prov) + : Task(nullptr), m_index_dir(dir), m_provider(prov), m_hashing_task(nullptr), m_current_task(nullptr) { - auto hash = getHash(mod); - if (hash.isEmpty()) - emitFail(mod); - else - m_mods.insert(hash, mod); + auto hash_task = createNewHash(mod); + connect(hash_task.get(), &Task::succeeded, [this, hash_task, mod] { m_mods.insert(hash_task->getResult(), mod); }); + connect(hash_task.get(), &Task::failed, [this, hash_task, mod] { emitFail(mod, RemoveFromList::No); }); + hash_task->start(); } EnsureMetadataTask::EnsureMetadataTask(QList& mods, QDir dir, ModPlatform::Provider prov) - : Task(nullptr), m_index_dir(dir), m_provider(prov) + : Task(nullptr), m_index_dir(dir), m_provider(prov), m_current_task(nullptr) { + m_hashing_task = new ConcurrentTask(this, "MakeHashesTask", 10); for (auto* mod : mods) { - if (!mod->valid()) { - emitFail(mod); + auto hash_task = createNewHash(mod); + if (!hash_task) continue; - } - - auto hash = getHash(mod); - if (hash.isEmpty()) { - emitFail(mod); - continue; - } - - m_mods.insert(hash, mod); + connect(hash_task.get(), &Task::succeeded, [this, hash_task, mod] { m_mods.insert(hash_task->getResult(), mod); }); + connect(hash_task.get(), &Task::failed, [this, hash_task, mod] { emitFail(mod, RemoveFromList::No); }); + m_hashing_task->addTask(hash_task); } } -QString EnsureMetadataTask::getHash(Mod* mod) +Hashing::Hasher::Ptr EnsureMetadataTask::createNewHash(Mod* mod) { - /* Here we create a mapping hash -> mod, because we need that relationship to parse the API routes */ - if (mod->type() == Mod::MOD_FOLDER) - return {}; + if (!mod->valid() || mod->type() == Mod::MOD_FOLDER) + return nullptr; - QString result; - switch (m_provider) { - case ModPlatform::Provider::MODRINTH: { - QFile file(mod->fileinfo().absoluteFilePath()); - - try { - file.open(QFile::ReadOnly); - } catch (FS::FileSystemException& e) { - qCritical() << QString("Failed to open JAR file of %1").arg(mod->name()); - qCritical() << QString("Reason: ") << e.cause(); - - return {}; - } - - auto hash_type = ProviderCaps.hashType(ModPlatform::Provider::MODRINTH).first(); - result = ProviderCaps.hash(ModPlatform::Provider::MODRINTH, &file, hash_type); - - file.close(); + return Hashing::createHasher(mod->fileinfo().absoluteFilePath(), m_provider); +} +QString EnsureMetadataTask::getExistingHash(Mod* mod) +{ + // Check for already computed hashes + // (linear on the number of mods vs. linear on the size of the mod's JAR) + auto it = m_mods.keyValueBegin(); + while (it != m_mods.keyValueEnd()) { + if ((*it).second == mod) break; - } - case ModPlatform::Provider::FLAME: { - auto should_filter_out = [](char c) { - // CF-specific - return (c == 9 || c == 10 || c == 13 || c == 32); - }; - - std::ifstream file_stream(mod->fileinfo().absoluteFilePath().toStdString(), std::ifstream::binary); - result = QString::number(MurmurHash2(std::move(file_stream), 4096, should_filter_out)); - } + it++; + } + + // We already have the hash computed + if (it != m_mods.keyValueEnd()) { + return (*it).first; } - return result; + // No existing hash + return {}; } bool EnsureMetadataTask::abort() @@ -185,20 +168,22 @@ void EnsureMetadataTask::executeTask() version_task->start(); } -void EnsureMetadataTask::emitReady(Mod* m) +void EnsureMetadataTask::emitReady(Mod* m, RemoveFromList remove) { qDebug() << QString("Generated metadata for %1").arg(m->name()); emit metadataReady(m); - m_mods.remove(getHash(m)); + if (remove == RemoveFromList::Yes) + m_mods.remove(getExistingHash(m)); } -void EnsureMetadataTask::emitFail(Mod* m) +void EnsureMetadataTask::emitFail(Mod* m, RemoveFromList remove) { qDebug() << QString("Failed to generate metadata for %1").arg(m->name()); emit metadataFailed(m); - m_mods.remove(getHash(m)); + if (remove == RemoveFromList::Yes) + m_mods.remove(getExistingHash(m)); } // Modrinth diff --git a/launcher/modplatform/EnsureMetadataTask.h b/launcher/modplatform/EnsureMetadataTask.h index 79db6976..13319266 100644 --- a/launcher/modplatform/EnsureMetadataTask.h +++ b/launcher/modplatform/EnsureMetadataTask.h @@ -1,12 +1,14 @@ #pragma once #include "ModIndex.h" -#include "tasks/SequentialTask.h" #include "net/NetJob.h" +#include "modplatform/helpers/HashUtils.h" + +#include "tasks/ConcurrentTask.h" + class Mod; class QDir; -class MultipleOptionsTask; class EnsureMetadataTask : public Task { Q_OBJECT @@ -17,6 +19,8 @@ class EnsureMetadataTask : public Task { ~EnsureMetadataTask() = default; + Task::Ptr getHashingTask() { return m_hashing_task; } + public slots: bool abort() override; protected slots: @@ -31,10 +35,16 @@ class EnsureMetadataTask : public Task { auto flameProjectsTask() -> NetJob::Ptr; // Helpers - void emitReady(Mod*); - void emitFail(Mod*); + enum class RemoveFromList { + Yes, + No + }; + void emitReady(Mod*, RemoveFromList = RemoveFromList::Yes); + void emitFail(Mod*, RemoveFromList = RemoveFromList::Yes); - auto getHash(Mod*) -> QString; + // Hashes and stuff + auto createNewHash(Mod*) -> Hashing::Hasher::Ptr; + auto getExistingHash(Mod*) -> QString; private slots: void modrinthCallback(ModPlatform::IndexedPack& pack, ModPlatform::IndexedVersion& ver, Mod*); @@ -50,5 +60,6 @@ class EnsureMetadataTask : public Task { ModPlatform::Provider m_provider; QHash m_temp_versions; + ConcurrentTask* m_hashing_task; NetJob* m_current_task; }; -- cgit From f4b207220c60b9bbc6d4ea414a419768b8913c12 Mon Sep 17 00:00:00 2001 From: flow Date: Tue, 2 Aug 2022 15:38:51 -0300 Subject: fix: add some more nullptr checks / protection die sigsegv :gun: Signed-off-by: flow --- launcher/modplatform/EnsureMetadataTask.cpp | 44 ++++++++++++++++++++++------- launcher/modplatform/EnsureMetadataTask.h | 4 +-- 2 files changed, 36 insertions(+), 12 deletions(-) (limited to 'launcher/modplatform/EnsureMetadataTask.cpp') diff --git a/launcher/modplatform/EnsureMetadataTask.cpp b/launcher/modplatform/EnsureMetadataTask.cpp index 1f49d7c9..11bdb99d 100644 --- a/launcher/modplatform/EnsureMetadataTask.cpp +++ b/launcher/modplatform/EnsureMetadataTask.cpp @@ -24,8 +24,10 @@ EnsureMetadataTask::EnsureMetadataTask(Mod* mod, QDir dir, ModPlatform::Provider : Task(nullptr), m_index_dir(dir), m_provider(prov), m_hashing_task(nullptr), m_current_task(nullptr) { auto hash_task = createNewHash(mod); + if (!hash_task) + return; connect(hash_task.get(), &Task::succeeded, [this, hash_task, mod] { m_mods.insert(hash_task->getResult(), mod); }); - connect(hash_task.get(), &Task::failed, [this, hash_task, mod] { emitFail(mod, RemoveFromList::No); }); + connect(hash_task.get(), &Task::failed, [this, hash_task, mod] { emitFail(mod, "", RemoveFromList::No); }); hash_task->start(); } @@ -38,14 +40,14 @@ EnsureMetadataTask::EnsureMetadataTask(QList& mods, QDir dir, ModPlatform: if (!hash_task) continue; connect(hash_task.get(), &Task::succeeded, [this, hash_task, mod] { m_mods.insert(hash_task->getResult(), mod); }); - connect(hash_task.get(), &Task::failed, [this, hash_task, mod] { emitFail(mod, RemoveFromList::No); }); + connect(hash_task.get(), &Task::failed, [this, hash_task, mod] { emitFail(mod, "", RemoveFromList::No); }); m_hashing_task->addTask(hash_task); } } Hashing::Hasher::Ptr EnsureMetadataTask::createNewHash(Mod* mod) { - if (!mod->valid() || mod->type() == Mod::MOD_FOLDER) + if (!mod || !mod->valid() || mod->type() == Mod::MOD_FOLDER) return nullptr; return Hashing::createHasher(mod->fileinfo().absoluteFilePath(), m_provider); @@ -120,7 +122,7 @@ void EnsureMetadataTask::executeTask() QMutableHashIterator mods_iter(m_mods); while (mods_iter.hasNext()) { auto mod = mods_iter.next(); - emitFail(mod.value()); + emitFail(mod.value(), mod.key()); } emitSucceeded(); @@ -168,22 +170,44 @@ void EnsureMetadataTask::executeTask() version_task->start(); } -void EnsureMetadataTask::emitReady(Mod* m, RemoveFromList remove) +void EnsureMetadataTask::emitReady(Mod* m, QString key, RemoveFromList remove) { + if (!m) { + qCritical() << "Tried to mark a null mod as ready."; + if (!key.isEmpty()) + m_mods.remove(key); + + return; + } + qDebug() << QString("Generated metadata for %1").arg(m->name()); emit metadataReady(m); - if (remove == RemoveFromList::Yes) - m_mods.remove(getExistingHash(m)); + if (remove == RemoveFromList::Yes) { + if (key.isEmpty()) + key = getExistingHash(m); + m_mods.remove(key); + } } -void EnsureMetadataTask::emitFail(Mod* m, RemoveFromList remove) +void EnsureMetadataTask::emitFail(Mod* m, QString key, RemoveFromList remove) { + if (!m) { + qCritical() << "Tried to mark a null mod as failed."; + if (!key.isEmpty()) + m_mods.remove(key); + + return; + } + qDebug() << QString("Failed to generate metadata for %1").arg(m->name()); emit metadataFailed(m); - if (remove == RemoveFromList::Yes) - m_mods.remove(getExistingHash(m)); + if (remove == RemoveFromList::Yes) { + if (key.isEmpty()) + key = getExistingHash(m); + m_mods.remove(key); + } } // Modrinth diff --git a/launcher/modplatform/EnsureMetadataTask.h b/launcher/modplatform/EnsureMetadataTask.h index 13319266..a8b0851e 100644 --- a/launcher/modplatform/EnsureMetadataTask.h +++ b/launcher/modplatform/EnsureMetadataTask.h @@ -39,8 +39,8 @@ class EnsureMetadataTask : public Task { Yes, No }; - void emitReady(Mod*, RemoveFromList = RemoveFromList::Yes); - void emitFail(Mod*, RemoveFromList = RemoveFromList::Yes); + void emitReady(Mod*, QString key = {}, RemoveFromList = RemoveFromList::Yes); + void emitFail(Mod*, QString key = {}, RemoveFromList = RemoveFromList::Yes); // Hashes and stuff auto createNewHash(Mod*) -> Hashing::Hasher::Ptr; -- cgit From 7b27f200b1f131f0ea3b23433974cbe68eb979bb Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 5 Aug 2022 16:23:46 -0300 Subject: fix: don't mutate QHash while iterating over it Even though it was using a QMutableHashIterator, sometimes it didn't work quite well, so this is a bit better. Signed-off-by: flow --- launcher/modplatform/EnsureMetadataTask.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'launcher/modplatform/EnsureMetadataTask.cpp') diff --git a/launcher/modplatform/EnsureMetadataTask.cpp b/launcher/modplatform/EnsureMetadataTask.cpp index 11bdb99d..42137398 100644 --- a/launcher/modplatform/EnsureMetadataTask.cpp +++ b/launcher/modplatform/EnsureMetadataTask.cpp @@ -119,11 +119,9 @@ void EnsureMetadataTask::executeTask() } auto invalidade_leftover = [this] { - QMutableHashIterator mods_iter(m_mods); - while (mods_iter.hasNext()) { - auto mod = mods_iter.next(); - emitFail(mod.value(), mod.key()); - } + for (auto mod = m_mods.constBegin(); mod != m_mods.constEnd(); mod++) + emitFail(mod.value(), mod.key(), RemoveFromList::No); + m_mods.clear(); emitSucceeded(); }; -- cgit