From c19866f887b01a74f48157d750b3f5b086579ec6 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 12 Aug 2020 15:59:42 +0200 Subject: [PATCH 1/9] [libcalamares] Add a module-weight to the module descriptor --- CMakeModules/CalamaresAddPlugin.cmake | 9 ++++++- src/libcalamares/modulesystem/Descriptor.cpp | 27 +++++++++++++++----- src/libcalamares/modulesystem/Descriptor.h | 4 +++ src/modules/README.md | 12 +++++++++ 4 files changed, 45 insertions(+), 7 deletions(-) diff --git a/CMakeModules/CalamaresAddPlugin.cmake b/CMakeModules/CalamaresAddPlugin.cmake index ee3c63acb..a90e6df63 100644 --- a/CMakeModules/CalamaresAddPlugin.cmake +++ b/CMakeModules/CalamaresAddPlugin.cmake @@ -41,6 +41,7 @@ # [NO_CONFIG] # [SHARED_LIB] # [EMERGENCY] +# [WEIGHT w] # ) # # Function parameters: @@ -63,6 +64,9 @@ # - EMERGENCY # If this is set, the module is marked as an *emergency* module in the # descriptor. See *Emergency Modules* in the module documentation. +# - WEIGHT +# If this is set, writes an explicit weight into the module.desc; +# module weights are used in progress reporting. # include( CMakeParseArguments ) @@ -73,7 +77,7 @@ function( calamares_add_plugin ) # parse arguments ( name needs to be saved before passing ARGN into the macro ) set( NAME ${ARGV0} ) set( options NO_CONFIG NO_INSTALL SHARED_LIB EMERGENCY ) - set( oneValueArgs NAME TYPE EXPORT_MACRO RESOURCES ) + set( oneValueArgs NAME TYPE EXPORT_MACRO RESOURCES WEIGHT ) set( multiValueArgs SOURCES UI LINK_LIBRARIES LINK_PRIVATE_LIBRARIES COMPILE_DEFINITIONS REQUIRES ) cmake_parse_arguments( PLUGIN "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN} ) set( PLUGIN_NAME ${NAME} ) @@ -181,6 +185,9 @@ function( calamares_add_plugin ) if ( PLUGIN_NO_CONFIG ) file( APPEND ${_file} "noconfig: true\n" ) endif() + if ( PLUGIN_WEIGHT ) + file( APPEND ${_file} "weight: ${PLUGIN_WEIGHT}\n" ) + endif() endif() if ( NOT PLUGIN_NO_INSTALL ) diff --git a/src/libcalamares/modulesystem/Descriptor.cpp b/src/libcalamares/modulesystem/Descriptor.cpp index 1ac4dc6c0..9b3e48b6e 100644 --- a/src/libcalamares/modulesystem/Descriptor.cpp +++ b/src/libcalamares/modulesystem/Descriptor.cpp @@ -87,8 +87,9 @@ Descriptor::fromDescriptorData( const QVariantMap& moduleDesc ) d.m_isEmergeny = CalamaresUtils::getBool( moduleDesc, "emergency", false ); d.m_hasConfig = !CalamaresUtils::getBool( moduleDesc, "noconfig", false ); // Inverted logic during load d.m_requiredModules = CalamaresUtils::getStringList( moduleDesc, "requiredModules" ); + d.m_weight = int( CalamaresUtils::getInteger( moduleDesc, "weight", -1 ) ); - QStringList consumedKeys { "type", "interface", "name", "emergency", "noconfig", "requiredModules" }; + QStringList consumedKeys { "type", "interface", "name", "emergency", "noconfig", "requiredModules", "weight" }; switch ( d.interface() ) { @@ -99,23 +100,37 @@ Descriptor::fromDescriptorData( const QVariantMap& moduleDesc ) case Interface::Python: case Interface::PythonQt: d.m_script = CalamaresUtils::getString( moduleDesc, "script" ); + if ( d.m_script.isEmpty() ) + { + cWarning() << "Module descriptor contains no *script*" << d.name(); + d.m_isValid = false; + } consumedKeys << "script"; break; case Interface::Process: d.m_script = CalamaresUtils::getString( moduleDesc, "command" ); - d.m_processTimeout = CalamaresUtils::getInteger( moduleDesc, "timeout", 30 ); + d.m_processTimeout = int( CalamaresUtils::getInteger( moduleDesc, "timeout", 30 ) ); d.m_processChroot = CalamaresUtils::getBool( moduleDesc, "chroot", false ); - consumedKeys << "command" - << "timeout" - << "chroot"; - if ( d.m_processTimeout < 0 ) { d.m_processTimeout = 0; } + if ( d.m_script.isEmpty() ) + { + cWarning() << "Module descriptor contains no *script*" << d.name(); + d.m_isValid = false; + } + consumedKeys << "command" + << "timeout" + << "chroot"; break; } + if ( !d.m_isValid ) + { + return d; + } + QStringList superfluousKeys; for ( auto kv = moduleDesc.keyBegin(); kv != moduleDesc.keyEnd(); ++kv ) { diff --git a/src/libcalamares/modulesystem/Descriptor.h b/src/libcalamares/modulesystem/Descriptor.h index 8166f9869..a94c0574d 100644 --- a/src/libcalamares/modulesystem/Descriptor.h +++ b/src/libcalamares/modulesystem/Descriptor.h @@ -80,6 +80,9 @@ public: bool isEmergency() const { return m_isEmergeny; } bool hasConfig() const { return m_hasConfig; } + int weight() const { return m_weight < 1 ? 1 : m_weight; } + bool explicitWeight() const { return m_weight > 0; } + /// @brief The directory where the module.desc lives QString directory() const { return m_directory; } @@ -125,6 +128,7 @@ private: QString m_name; QString m_directory; QStringList m_requiredModules; + int m_weight = -1; Type m_type; Interface m_interface; bool m_isValid = false; diff --git a/src/modules/README.md b/src/modules/README.md index ee6e378d2..1220c7398 100644 --- a/src/modules/README.md +++ b/src/modules/README.md @@ -46,9 +46,19 @@ Module descriptors **must** have the following keys: - *interface* (see below for the different interfaces; generally we refer to the kinds of modules by their interface) +Module descriptors for C++ modules **may** have the following key: +- *load* (the name of the shared library to load; if empty, uses a + standard library name derived from the module name) + Module descriptors for Python modules **must** have the following key: - *script* (the name of the Python script to load, nearly always `main.py`) +Module descriptors for process modules **must** have the following key: +- *command* (the command to run) +Module descriptors for process modules **may** have the following keys: +- *timeout* (how long, in seconds, to wait for the command to run) +- *chroos* (if true, run the command in the target system rather than the host) + Module descriptors **may** have the following keys: - *emergency* (a boolean value, set to true to mark the module as an emergency module) @@ -56,6 +66,8 @@ Module descriptors **may** have the following keys: has no configuration file; defaults to false) - *requiredModules* (a list of modules which are required for this module to operate properly) +- *weight* (a relative module weight, used to scale progress reporting) + ### Required Modules From 73b8ecd622008bf0b2b36b50da82b60b39b91171 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 12 Aug 2020 16:03:25 +0200 Subject: [PATCH 2/9] [unpackfs] Drop special-case for unpackfs - make the module weight 12, rather than special-casing internals --- src/libcalamares/PythonJob.cpp | 5 ++--- src/libcalamares/PythonJob.h | 3 +-- src/modules/unpackfs/module.desc | 1 + 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libcalamares/PythonJob.cpp b/src/libcalamares/PythonJob.cpp index 50828e896..87091101e 100644 --- a/src/libcalamares/PythonJob.cpp +++ b/src/libcalamares/PythonJob.cpp @@ -1,5 +1,5 @@ /* === This file is part of Calamares - === - * + * * SPDX-FileCopyrightText: 2014-2016 Teo Mrnjavac * SPDX-FileCopyrightText: 2018-2020 Adriaan de Groot * @@ -184,7 +184,6 @@ PythonJob::PythonJob( const ModuleSystem::InstanceKey& instance, , m_workingPath( workingPath ) , m_description() , m_configurationMap( moduleConfiguration ) - , m_weight( ( instance.module() == QStringLiteral( "unpackfs" ) ) ? 12.0 : 1.0 ) { } @@ -194,7 +193,7 @@ PythonJob::~PythonJob() {} qreal PythonJob::getJobWeight() const { - return m_weight; + return 1.0; } QString diff --git a/src/libcalamares/PythonJob.h b/src/libcalamares/PythonJob.h index 498eac44f..6efc45e7f 100644 --- a/src/libcalamares/PythonJob.h +++ b/src/libcalamares/PythonJob.h @@ -1,5 +1,5 @@ /* === This file is part of Calamares - === - * + * * SPDX-FileCopyrightText: 2014 Teo Mrnjavac * SPDX-FileCopyrightText: 2020 Adriaan de Groot * @@ -68,7 +68,6 @@ private: QString m_workingPath; QString m_description; QVariantMap m_configurationMap; - qreal m_weight; }; } // namespace Calamares diff --git a/src/modules/unpackfs/module.desc b/src/modules/unpackfs/module.desc index 54d95df1d..c87613d74 100644 --- a/src/modules/unpackfs/module.desc +++ b/src/modules/unpackfs/module.desc @@ -5,3 +5,4 @@ name: "unpackfs" interface: "python" script: "main.py" requiredModules: [ mount ] +weight: 12 From a91ef65a37f366324444acabdcd84cc6e8939e23 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 12 Aug 2020 16:08:37 +0200 Subject: [PATCH 3/9] [libcalamares] Make job weights integers --- src/libcalamares/Job.cpp | 6 +++--- src/libcalamares/Job.h | 10 ++++++++-- src/libcalamares/PythonJob.cpp | 6 ------ src/libcalamares/PythonJob.h | 2 -- 4 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/libcalamares/Job.cpp b/src/libcalamares/Job.cpp index 95339f349..a7e67b5f6 100644 --- a/src/libcalamares/Job.cpp +++ b/src/libcalamares/Job.cpp @@ -1,5 +1,5 @@ /* === This file is part of Calamares - === - * + * * SPDX-FileCopyrightText: 2014-2015 Teo Mrnjavac * * Calamares is free software: you can redistribute it and/or modify @@ -101,10 +101,10 @@ Job::Job( QObject* parent ) Job::~Job() {} -qreal +int Job::getJobWeight() const { - return qreal( 1.0 ); + return 1; } diff --git a/src/libcalamares/Job.h b/src/libcalamares/Job.h index 41674cfff..dd13f8608 100644 --- a/src/libcalamares/Job.h +++ b/src/libcalamares/Job.h @@ -100,13 +100,19 @@ public: /** @brief The job's (relative) weight. * - * The default implementation returns 1.0, which gives all jobs + * The default implementation returns 1, which gives all jobs * the same weight, so they advance the overall progress the same * amount. This is nonsense, since some jobs take much longer than * others; it's up to the individual jobs to say something about * how much work is (relatively) done. + * + * Since jobs are caused by **modules** from the sequence, the + * overall weight of the module is taken into account: its weight + * is divided among the jobs based on each jobs relative weight. + * This can be used in a module that runs a bunch of jobs to indicate + * which of the jobs is "heavy" and which is not. */ - virtual qreal getJobWeight() const; + virtual int getJobWeight() const; /** @brief The human-readable name of this job * * This should be a very short statement of what the job does. diff --git a/src/libcalamares/PythonJob.cpp b/src/libcalamares/PythonJob.cpp index 87091101e..c1cfbbc1c 100644 --- a/src/libcalamares/PythonJob.cpp +++ b/src/libcalamares/PythonJob.cpp @@ -190,12 +190,6 @@ PythonJob::PythonJob( const ModuleSystem::InstanceKey& instance, PythonJob::~PythonJob() {} -qreal -PythonJob::getJobWeight() const -{ - return 1.0; -} - QString PythonJob::prettyName() const { diff --git a/src/libcalamares/PythonJob.h b/src/libcalamares/PythonJob.h index 6efc45e7f..b34b40b0b 100644 --- a/src/libcalamares/PythonJob.h +++ b/src/libcalamares/PythonJob.h @@ -55,8 +55,6 @@ public: QString prettyStatusMessage() const override; JobResult exec() override; - virtual qreal getJobWeight() const override; - private: struct Private; From c296bcffa35cef8664b4d9e2897ab7d6655f863f Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 12 Aug 2020 16:45:27 +0200 Subject: [PATCH 4/9] [libcalamares] When enqueueing jobs, pass a weight - The weight is the module (instance) weight, which can be - the default weight of 1 - the weight specified for the module (in module.desc / the module descriptor; this defaults to 1, above) - the weight specified for the instance (in settings.conf) The last of these "wins"; weights are constrained to 1..100 The weight isn't actually used in progress computation yet. --- src/libcalamares/JobQueue.cpp | 11 +---------- src/libcalamares/JobQueue.h | 11 +++++++---- .../modulesystem/ModuleManager.h | 10 ++++++++++ .../viewpages/ExecutionViewStep.cpp | 18 ++++++++++++++++-- 4 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/libcalamares/JobQueue.cpp b/src/libcalamares/JobQueue.cpp index bc3028ae7..b78ba32dc 100644 --- a/src/libcalamares/JobQueue.cpp +++ b/src/libcalamares/JobQueue.cpp @@ -205,16 +205,7 @@ JobQueue::start() void -JobQueue::enqueue( const job_ptr& job ) -{ - Q_ASSERT( !m_thread->isRunning() ); - m_jobs.append( job ); - emit queueChanged( m_jobs ); -} - - -void -JobQueue::enqueue( const JobList& jobs ) +JobQueue::enqueue( int moduleWeight, const JobList& jobs ) { Q_ASSERT( !m_thread->isRunning() ); m_jobs.append( jobs ); diff --git a/src/libcalamares/JobQueue.h b/src/libcalamares/JobQueue.h index ff2694d8f..3847b4667 100644 --- a/src/libcalamares/JobQueue.h +++ b/src/libcalamares/JobQueue.h @@ -1,5 +1,5 @@ /* === This file is part of Calamares - === - * + * * SPDX-FileCopyrightText: 2014-2015 Teo Mrnjavac * * Calamares is free software: you can redistribute it and/or modify @@ -30,7 +30,6 @@ namespace Calamares { - class GlobalStorage; class JobThread; @@ -45,8 +44,12 @@ public: GlobalStorage* globalStorage() const; - void enqueue( const job_ptr& job ); - void enqueue( const JobList& jobs ); + /** @brief Queues up jobs from a single module source + * + * The total weight of the jobs is spread out to fill the weight + * of the module. + */ + void enqueue( int moduleWeight, const JobList& jobs ); void start(); bool isRunning() const { return !m_finished; } diff --git a/src/libcalamaresui/modulesystem/ModuleManager.h b/src/libcalamaresui/modulesystem/ModuleManager.h index ed4cef8ba..eb7d086c1 100644 --- a/src/libcalamaresui/modulesystem/ModuleManager.h +++ b/src/libcalamaresui/modulesystem/ModuleManager.h @@ -71,6 +71,16 @@ public: */ ModuleSystem::Descriptor moduleDescriptor( const QString& name ); + /** @brief returns the module descriptor structure for the module @p instance + * + * Descriptors are for the module, which may have multiple instances; + * this is the same as moduleDescriptor( instance.module() ). + */ + ModuleSystem::Descriptor moduleDescriptor( const ModuleSystem::InstanceKey& instanceKey ) + { + return moduleDescriptor( instanceKey.module() ); + } + /** * @brief moduleInstance returns a Module object for a given instance key. * @param instanceKey the instance key for a module instance. diff --git a/src/libcalamaresui/viewpages/ExecutionViewStep.cpp b/src/libcalamaresui/viewpages/ExecutionViewStep.cpp index b2205d70e..6f3983dc4 100644 --- a/src/libcalamaresui/viewpages/ExecutionViewStep.cpp +++ b/src/libcalamaresui/viewpages/ExecutionViewStep.cpp @@ -146,10 +146,24 @@ ExecutionViewStep::onActivate() { m_slideshow->changeSlideShowState( Slideshow::Start ); + const auto instanceDescriptors = Calamares::Settings::instance()->moduleInstances(); + JobQueue* queue = JobQueue::instance(); - for( const auto& instanceKey : m_jobInstanceKeys ) + for ( const auto& instanceKey : m_jobInstanceKeys ) { + const auto& moduleDescriptor = Calamares::ModuleManager::instance()->moduleDescriptor( instanceKey ); Calamares::Module* module = Calamares::ModuleManager::instance()->moduleInstance( instanceKey ); + + const auto instanceDescriptor + = std::find_if( instanceDescriptors.constBegin(), + instanceDescriptors.constEnd(), + [=]( const Calamares::InstanceDescription& d ) { return d.key() == instanceKey; } ); + int weight = moduleDescriptor.weight(); + if ( instanceDescriptor != instanceDescriptors.constEnd() && instanceDescriptor->explicitWeight() ) + { + weight = instanceDescriptor->weight(); + } + weight = qBound( 1, weight, 100 ); if ( module ) { auto jl = module->jobs(); @@ -160,7 +174,7 @@ ExecutionViewStep::onActivate() j->setEmergency( true ); } } - queue->enqueue( jl ); + queue->enqueue( weight, jl ); } } From 08ea51a344ce25cdebfd3e8619dbe2941f4dca36 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 19 Aug 2020 11:28:53 +0200 Subject: [PATCH 5/9] [partition] Fix tests after removal of single-job-enqueue --- .../partition/tests/PartitionJobTests.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/modules/partition/tests/PartitionJobTests.cpp b/src/modules/partition/tests/PartitionJobTests.cpp index bd2c6f879..ae40215db 100644 --- a/src/modules/partition/tests/PartitionJobTests.cpp +++ b/src/modules/partition/tests/PartitionJobTests.cpp @@ -223,7 +223,7 @@ PartitionJobTests::queuePartitionTableCreation( PartitionTable::TableType type ) { auto job = new CreatePartitionTableJob( m_device.data(), type ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); } CreatePartitionJob* @@ -275,7 +275,7 @@ PartitionJobTests::testCreatePartition() Partition* partition1 = job->partition(); QVERIFY( partition1 ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); freePartition = firstFreePartition( m_device->partitionTable() ); QVERIFY( freePartition ); @@ -283,7 +283,7 @@ PartitionJobTests::testCreatePartition() Partition* partition2 = job->partition(); QVERIFY( partition2 ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); freePartition = firstFreePartition( m_device->partitionTable() ); QVERIFY( freePartition ); @@ -291,7 +291,7 @@ PartitionJobTests::testCreatePartition() Partition* partition3 = job->partition(); QVERIFY( partition3 ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); QVERIFY( m_runner.run() ); @@ -316,14 +316,14 @@ PartitionJobTests::testCreatePartitionExtended() Partition* partition1 = job->partition(); QVERIFY( partition1 ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); freePartition = firstFreePartition( m_device->partitionTable() ); QVERIFY( freePartition ); job = newCreatePartitionJob( freePartition, PartitionRole( PartitionRole::Extended ), FileSystem::Extended, 10_MiB ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); Partition* extendedPartition = job->partition(); freePartition = firstFreePartition( extendedPartition ); @@ -332,7 +332,7 @@ PartitionJobTests::testCreatePartitionExtended() Partition* partition2 = job->partition(); QVERIFY( partition2 ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); QVERIFY( m_runner.run() ); @@ -394,7 +394,7 @@ PartitionJobTests::testResizePartition() KPM_PARTITION_FLAG( None ) ); CreatePartitionJob* job = new CreatePartitionJob( m_device.data(), partition ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); QVERIFY( m_runner.run() ); } @@ -418,7 +418,7 @@ PartitionJobTests::testResizePartition() // Resize ResizePartitionJob* job = new ResizePartitionJob( m_device.data(), partition, newFirst, newLast ); job->updatePreview(); - m_queue.enqueue( job_ptr( job ) ); + m_queue.enqueue( 1, JobList() << job_ptr( job ) ); QVERIFY( m_runner.run() ); QCOMPARE( partition->firstSector(), newFirst ); From 941b5af3a224256fa7a7b5bb4f5b0dd64676d498 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 19 Aug 2020 12:54:40 +0200 Subject: [PATCH 6/9] [libcalamares] Rip out the guts of job-queue-running - compute weights and accumulations beforehand - mutex-lock structures so you can enqueue while running jobs - simplify progress reporting calculations - doesn't actually run any jobs --- src/libcalamares/JobQueue.cpp | 185 ++++++++++++++++++---------------- src/libcalamares/JobQueue.h | 1 - 2 files changed, 99 insertions(+), 87 deletions(-) diff --git a/src/libcalamares/JobQueue.cpp b/src/libcalamares/JobQueue.cpp index b78ba32dc..491e08f7c 100644 --- a/src/libcalamares/JobQueue.cpp +++ b/src/libcalamares/JobQueue.cpp @@ -28,11 +28,34 @@ #include "Job.h" #include "utils/Logger.h" +#include +#include #include namespace Calamares { +struct WeightedJob +{ + /** @brief Cumulative weight **before** this job starts + * + * This is calculated as jobs come in. + */ + double cumulative = 0.0; + /** @brief Weight of the job within the module's jobs + * + * When a list of jobs is added from a particular module, + * the jobs are weighted relative to that module's overall weight + * **and** the other jobs in the list, so that each job + * gets its share: + * ( job-weight / total-job-weight ) * module-weight + */ + double weight = 0.0; + + job_ptr job; +}; +using WeightedJobList = QList< WeightedJob >; + class JobThread : public QThread { public: @@ -45,106 +68,99 @@ public: virtual ~JobThread() override; - void setJobs( JobList&& jobs ) + void finalize() { - m_jobs = jobs; - - qreal totalJobsWeight = 0.0; - for ( auto job : m_jobs ) + Q_ASSERT( m_runningJobs->isEmpty() ); + QMutexLocker qlock( &m_enqueMutex ); + QMutexLocker rlock( &m_runMutex ); + std::swap( m_runningJobs, m_queuedJobs ); + m_overallQueueWeight + = m_runningJobs->isEmpty() ? 0.0 : ( m_runningJobs->last().cumulative + m_runningJobs->last().weight ); + if ( m_overallQueueWeight < 1 ) { - totalJobsWeight += job->getJobWeight(); + m_overallQueueWeight = 1.0; } - for ( auto job : m_jobs ) + } + + void enqueue( int moduleWeight, const JobList& jobs ) + { + QMutexLocker qlock( &m_enqueMutex ); + + double cumulative + = m_queuedJobs->isEmpty() ? 0.0 : ( m_queuedJobs->last().cumulative + m_queuedJobs->last().weight ); + + double totalJobWeight = std::accumulate( jobs.cbegin(), jobs.cend(), 0.0, []( double total, const job_ptr& j ) { + return total + j->getJobWeight(); + } ); + if ( totalJobWeight < 1 ) { - qreal jobWeight = qreal( job->getJobWeight() / totalJobsWeight ); - m_jobWeights.append( jobWeight ); + totalJobWeight = 1.0; + } + + for ( const auto& j : jobs ) + { + double jobContribution = ( j->getJobWeight() / totalJobWeight ) * moduleWeight; + m_queuedJobs->append( WeightedJob { cumulative, jobContribution, j } ); + cumulative += jobContribution; } } void run() override { - bool anyFailed = false; - QString message; - QString details; + QMutexLocker rlock( &m_runMutex ); + bool failureEncountered = false; m_jobIndex = 0; - for ( auto job : m_jobs ) + for ( const auto& jobitem : *m_runningJobs ) { - if ( anyFailed && !job->isEmergency() ) + if ( failureEncountered && !jobitem.job->isEmergency() ) { - cDebug() << "Skipping non-emergency job" << job->prettyName(); - ++m_jobIndex; - continue; + cDebug() << "Skipping non-emergency job" << jobitem.job->prettyName(); } - - emitProgress(); - cDebug() << "Starting" << ( anyFailed ? "EMERGENCY JOB" : "job" ) << job->prettyName() << "(there are" - << ( m_jobs.count() - m_jobIndex ) << "left)"; - connect( job.data(), &Job::progress, this, &JobThread::emitProgress ); - JobResult result = job->exec(); - if ( !anyFailed && !result ) + else { - anyFailed = true; - message = result.message(); - details = result.details(); + jobProgress( 0.0 ); // 0% for *this job* + cDebug() << "Starting" << ( failureEncountered ? "EMERGENCY JOB" : "job" ) << jobitem.job->prettyName() + << '(' << ( m_jobIndex + 1 ) << '/' << m_runningJobs->count() << ')'; + jobProgress( 1.0 ); // 100% for *this job* } - emitProgress( 1.0 ); - ++m_jobIndex; + m_jobIndex++; } - if ( anyFailed ) + } + + void jobProgress( double percentage ) const + { + percentage = qBound( 0.0, percentage, 1.0 ); + + QString message; + double progress = 0.0; + if ( m_jobIndex < m_runningJobs->count() ) { - emitFailed( message, details ); + + const auto& jobitem = m_runningJobs->at( m_jobIndex ); + progress = ( jobitem.cumulative + jobitem.weight * percentage ) / m_overallQueueWeight; + message = jobitem.job->prettyStatusMessage(); } else { - emitProgress(); + progress = 1.0; + message = tr( "Done" ); } - emitFinished(); + QMetaObject::invokeMethod( + m_queue, "progress", Qt::QueuedConnection, Q_ARG( double, progress ), Q_ARG( QString, message ) ); } + private: - JobList m_jobs; - QList< qreal > m_jobWeights; + QMutex m_runMutex; + QMutex m_enqueMutex; + + std::unique_ptr< WeightedJobList > m_runningJobs = std::make_unique< WeightedJobList >(); + std::unique_ptr< WeightedJobList > m_queuedJobs = std::make_unique< WeightedJobList >(); + JobQueue* m_queue; - int m_jobIndex; - - void emitProgress( qreal jobPercent = 0 ) - { - // Make sure jobPercent is reasonable, in case a job messed up its - // percentage computations. - jobPercent = qBound( qreal( 0 ), jobPercent, qreal( 1 ) ); - - int jobCount = m_jobs.size(); - QString message = m_jobIndex < jobCount ? m_jobs.at( m_jobIndex )->prettyStatusMessage() : tr( "Done" ); - - qreal percent = 1.0; // Pretend we're done, since the if will reset it - if ( m_jobIndex < jobCount ) - { - qreal cumulativeProgress = 0.0; - for ( auto jobWeight : m_jobWeights.mid( 0, m_jobIndex ) ) - { - cumulativeProgress += jobWeight; - } - percent = cumulativeProgress + ( ( m_jobWeights.at( m_jobIndex ) ) * jobPercent ); - - Logger::CDebug( Logger::LOGVERBOSE ) - << "[JOBQUEUE]: Progress for Job[" << m_jobIndex << "]: " << ( jobPercent * 100 ) << "% completed"; - Logger::CDebug( Logger::LOGVERBOSE ) - << "[JOBQUEUE]: Progress Overall: " << ( cumulativeProgress * 100 ) << "% (accumulated) + " - << ( ( ( m_jobWeights.at( m_jobIndex ) ) * jobPercent ) * 100 ) - << "% (this job) = " << ( percent * 100 ) << "% (total)"; - } - QMetaObject::invokeMethod( - m_queue, "progress", Qt::QueuedConnection, Q_ARG( qreal, percent ), Q_ARG( QString, message ) ); - } - - void emitFailed( const QString& message, const QString& details ) - { - QMetaObject::invokeMethod( - m_queue, "failed", Qt::QueuedConnection, Q_ARG( QString, message ), Q_ARG( QString, details ) ); - } - - void emitFinished() { QMetaObject::invokeMethod( m_queue, "finish", Qt::QueuedConnection ); } + int m_jobIndex = 0; ///< Index into m_runningJobs + double m_overallQueueWeight = 0.0; ///< cumulation when **all** the jobs are done }; JobThread::~JobThread() {} @@ -152,7 +168,6 @@ JobThread::~JobThread() {} JobQueue* JobQueue::s_instance = nullptr; - JobQueue* JobQueue::instance() { @@ -160,13 +175,6 @@ JobQueue::instance() } -GlobalStorage* -JobQueue::globalStorage() const -{ - return m_storage; -} - - JobQueue::JobQueue( QObject* parent ) : QObject( parent ) , m_thread( new JobThread( this ) ) @@ -197,8 +205,7 @@ void JobQueue::start() { Q_ASSERT( !m_thread->isRunning() ); - m_thread->setJobs( std::move( m_jobs ) ); - m_jobs.clear(); + m_thread->finalize(); m_finished = false; m_thread->start(); } @@ -208,8 +215,8 @@ void JobQueue::enqueue( int moduleWeight, const JobList& jobs ) { Q_ASSERT( !m_thread->isRunning() ); - m_jobs.append( jobs ); - emit queueChanged( m_jobs ); + m_thread->enqueue( moduleWeight, jobs ); + emit queueChanged( jobs ); // FIXME: bogus } void @@ -219,4 +226,10 @@ JobQueue::finish() emit finished(); } +GlobalStorage* +JobQueue::globalStorage() const +{ + return m_storage; +} + } // namespace Calamares diff --git a/src/libcalamares/JobQueue.h b/src/libcalamares/JobQueue.h index 3847b4667..31b5407ef 100644 --- a/src/libcalamares/JobQueue.h +++ b/src/libcalamares/JobQueue.h @@ -66,7 +66,6 @@ signals: private: static JobQueue* s_instance; - JobList m_jobs; JobThread* m_thread; GlobalStorage* m_storage; bool m_finished = true; ///< Initially, not running From 521015b1b45344ebf767559a985b7335b21056e7 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 19 Aug 2020 13:06:50 +0200 Subject: [PATCH 7/9] [libcalamares] Match types to existing qreal usage, signal progress --- src/libcalamares/JobQueue.cpp | 48 +++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/src/libcalamares/JobQueue.cpp b/src/libcalamares/JobQueue.cpp index 491e08f7c..17cf11829 100644 --- a/src/libcalamares/JobQueue.cpp +++ b/src/libcalamares/JobQueue.cpp @@ -41,7 +41,7 @@ struct WeightedJob * * This is calculated as jobs come in. */ - double cumulative = 0.0; + qreal cumulative = 0.0; /** @brief Weight of the job within the module's jobs * * When a list of jobs is added from a particular module, @@ -50,7 +50,7 @@ struct WeightedJob * gets its share: * ( job-weight / total-job-weight ) * module-weight */ - double weight = 0.0; + qreal weight = 0.0; job_ptr job; }; @@ -86,12 +86,13 @@ public: { QMutexLocker qlock( &m_enqueMutex ); - double cumulative + qreal cumulative = m_queuedJobs->isEmpty() ? 0.0 : ( m_queuedJobs->last().cumulative + m_queuedJobs->last().weight ); - double totalJobWeight = std::accumulate( jobs.cbegin(), jobs.cend(), 0.0, []( double total, const job_ptr& j ) { - return total + j->getJobWeight(); - } ); + qreal totalJobWeight + = std::accumulate( jobs.cbegin(), jobs.cend(), qreal( 0.0 ), []( qreal total, const job_ptr& j ) { + return total + j->getJobWeight(); + } ); if ( totalJobWeight < 1 ) { totalJobWeight = 1.0; @@ -99,7 +100,7 @@ public: for ( const auto& j : jobs ) { - double jobContribution = ( j->getJobWeight() / totalJobWeight ) * moduleWeight; + qreal jobContribution = ( j->getJobWeight() / totalJobWeight ) * moduleWeight; m_queuedJobs->append( WeightedJob { cumulative, jobContribution, j } ); cumulative += jobContribution; } @@ -109,6 +110,8 @@ public: { QMutexLocker rlock( &m_runMutex ); bool failureEncountered = false; + QString message; ///< Filled in with errors + QString details; m_jobIndex = 0; for ( const auto& jobitem : *m_runningJobs ) @@ -119,21 +122,40 @@ public: } else { - jobProgress( 0.0 ); // 0% for *this job* + emitProgress( 0.0 ); // 0% for *this job* cDebug() << "Starting" << ( failureEncountered ? "EMERGENCY JOB" : "job" ) << jobitem.job->prettyName() << '(' << ( m_jobIndex + 1 ) << '/' << m_runningJobs->count() << ')'; - jobProgress( 1.0 ); // 100% for *this job* + connect( jobitem.job.data(), &Job::progress, this, &JobThread::emitProgress ); + auto result = jobitem.job->exec(); + if ( !failureEncountered && !result ) + { + // so this is the first failure + failureEncountered = true; + message = result.message(); + details = result.details(); + } + emitProgress( 1.0 ); // 100% for *this job* } m_jobIndex++; } + if ( failureEncountered ) + { + QMetaObject::invokeMethod( + m_queue, "failed", Qt::QueuedConnection, Q_ARG( QString, message ), Q_ARG( QString, details ) ); + } + else + { + emitProgress( 1.0 ); + } + QMetaObject::invokeMethod( m_queue, "finish", Qt::QueuedConnection ); } - void jobProgress( double percentage ) const + void emitProgress( qreal percentage ) const { percentage = qBound( 0.0, percentage, 1.0 ); QString message; - double progress = 0.0; + qreal progress = 0.0; if ( m_jobIndex < m_runningJobs->count() ) { @@ -147,7 +169,7 @@ public: message = tr( "Done" ); } QMetaObject::invokeMethod( - m_queue, "progress", Qt::QueuedConnection, Q_ARG( double, progress ), Q_ARG( QString, message ) ); + m_queue, "progress", Qt::QueuedConnection, Q_ARG( qreal, progress ), Q_ARG( QString, message ) ); } @@ -160,7 +182,7 @@ private: JobQueue* m_queue; int m_jobIndex = 0; ///< Index into m_runningJobs - double m_overallQueueWeight = 0.0; ///< cumulation when **all** the jobs are done + qreal m_overallQueueWeight = 0.0; ///< cumulation when **all** the jobs are done }; JobThread::~JobThread() {} From 053321d4d20196b86cbca6d24539c1a20ad5f938 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 19 Aug 2020 15:07:47 +0200 Subject: [PATCH 8/9] [libcalamares] Test for progress reporting - add a dummy job class for tests - run a queue with 3 jobs from 2 modules to check progress reporting --- src/libcalamares/Tests.cpp | 163 ++++++++++++++++++++++++++++++++++++- 1 file changed, 161 insertions(+), 2 deletions(-) diff --git a/src/libcalamares/Tests.cpp b/src/libcalamares/Tests.cpp index 5903dfde6..68e1112c6 100644 --- a/src/libcalamares/Tests.cpp +++ b/src/libcalamares/Tests.cpp @@ -20,9 +20,9 @@ */ #include "GlobalStorage.h" +#include "JobQueue.h" #include "Settings.h" #include "modulesystem/InstanceKey.h" - #include "utils/Logger.h" #include @@ -46,6 +46,8 @@ private Q_SLOTS: void testInstanceDescription(); void testSettings(); + + void testJobQueue(); }; void @@ -341,7 +343,7 @@ TestLibCalamares::testInstanceDescription() { QVariantMap m; m.insert( "module", "welcome" ); - m.insert( "weight", 1); + m.insert( "weight", 1 ); InstanceDescription d = InstanceDescription::fromSettings( m ); QVERIFY( d.isValid() ); @@ -480,6 +482,163 @@ sequence: } } +constexpr const std::chrono::milliseconds MAX_TEST_DURATION( 3000 ); +constexpr const int MAX_TEST_SLEEP( 2 ); // seconds, < MAX_TEST_DURATION + +Q_STATIC_ASSERT( std::chrono::seconds( MAX_TEST_SLEEP ) < MAX_TEST_DURATION ); + +class DummyJob : public Calamares::Job +{ +public: + DummyJob( QObject* parent ) + : Calamares::Job( parent ) + { + } + virtual ~DummyJob() override; + + QString prettyName() const override; + Calamares::JobResult exec() override; +}; + +DummyJob::~DummyJob() {} + +QString +DummyJob::prettyName() const +{ + return QString( "DummyJob" ); +} + +Calamares::JobResult +DummyJob::exec() +{ + cDebug() << "Starting DummyJob"; + progress( 0.5 ); + QThread::sleep( MAX_TEST_SLEEP ); + cDebug() << ".. continuing DummyJob"; + progress( 0.75 ); + return Calamares::JobResult::ok(); +} + + +void +TestLibCalamares::testJobQueue() +{ + // Run an empty queue + { + Calamares::JobQueue q; + QVERIFY( !q.isRunning() ); + + QSignalSpy spy_progress( &q, &Calamares::JobQueue::progress ); + QSignalSpy spy_finished( &q, &Calamares::JobQueue::finished ); + QSignalSpy spy_failed( &q, &Calamares::JobQueue::failed ); + + QEventLoop loop; + connect( &q, &Calamares::JobQueue::finished, &loop, &QEventLoop::quit ); + QTimer::singleShot( MAX_TEST_DURATION, &loop, &QEventLoop::quit ); + q.start(); + QVERIFY( q.isRunning() ); + loop.exec(); + QVERIFY( !q.isRunning() ); + QCOMPARE( spy_finished.count(), 1 ); + QCOMPARE( spy_failed.count(), 0 ); + QCOMPARE( spy_progress.count(), 1 ); // just one, 100% at queue end + } + + // Run a dummy queue + { + Calamares::JobQueue q; + QVERIFY( !q.isRunning() ); + + q.enqueue( 8, Calamares::JobList() << Calamares::job_ptr( new DummyJob( this ) ) ); + QSignalSpy spy_progress( &q, &Calamares::JobQueue::progress ); + QSignalSpy spy_finished( &q, &Calamares::JobQueue::finished ); + QSignalSpy spy_failed( &q, &Calamares::JobQueue::failed ); + + QEventLoop loop; + connect( &q, &Calamares::JobQueue::finished, &loop, &QEventLoop::quit ); + QTimer::singleShot( MAX_TEST_DURATION, &loop, &QEventLoop::quit ); + q.start(); + QVERIFY( q.isRunning() ); + loop.exec(); + QVERIFY( !q.isRunning() ); + QCOMPARE( spy_finished.count(), 1 ); + QCOMPARE( spy_failed.count(), 0 ); + // 0% by the queue at job start + // 50% by the job itself + // 90% by the job itself + // 100% by the queue at job end + // 100% by the queue at queue end + QCOMPARE( spy_progress.count(), 5 ); + } + + { + Calamares::JobQueue q; + QVERIFY( !q.isRunning() ); + + q.enqueue( 8, Calamares::JobList() << Calamares::job_ptr( new DummyJob( this ) ) ); + q.enqueue( 12, + Calamares::JobList() << Calamares::job_ptr( new DummyJob( this ) ) + << Calamares::job_ptr( new DummyJob( this ) ) ); + QSignalSpy spy_progress( &q, &Calamares::JobQueue::progress ); + QSignalSpy spy_finished( &q, &Calamares::JobQueue::finished ); + QSignalSpy spy_failed( &q, &Calamares::JobQueue::failed ); + + QEventLoop loop; + connect( &q, &Calamares::JobQueue::finished, &loop, &QEventLoop::quit ); + // Run the loop longer because the jobs take longer (there are 3 of them) + QTimer::singleShot( 3 * MAX_TEST_DURATION, &loop, &QEventLoop::quit ); + q.start(); + QVERIFY( q.isRunning() ); + loop.exec(); + QVERIFY( !q.isRunning() ); + QCOMPARE( spy_finished.count(), 1 ); + QCOMPARE( spy_failed.count(), 0 ); + // 0% by the queue at job start + // 50% by the job itself + // 90% by the job itself + // 100% by the queue at job end + // 4 more for the next job + // 4 more for the next job + // 100% by the queue at queue end + QCOMPARE( spy_progress.count(), 13 ); + + /* Consider how progress will be reported: + * + * - the first module has weight 8, so the 1 job it has has weight 8 + * - the second module has weight 12, so each of its two jobs has weight 6 + * + * Total weight of the modules is 20. So the events are + * + * Job Progress Overall Weight Consumed Overall Progress + * 1 0 0 0.00 + * 1 50 4 0.20 + * 1 75 6 0.30 + * 1 100 8 0.40 + * 2 0 8 0.40 + * 2 50 11 (8 + 50% of 6) 0.55 + * 2 75 12.5 0.625 + * 2 100 14 0.70 + * 3 0 14 0.70 + * 3 50 17 0.85 + * 3 75 18.5 0.925 + * 3 100 20 1.00 + * - 100 20 1.00 + */ + cDebug() << "Progress signals:"; + qreal overallProgress = 0.0; + for ( const auto& e : spy_progress ) + { + QCOMPARE( e.count(), 2 ); + const auto v = e.first(); + QVERIFY( v.canConvert< qreal >() ); + qreal progress = v.toReal(); + cDebug() << Logger::SubEntry << progress; + QVERIFY( progress >= overallProgress ); // Doesn't go backwards + overallProgress = progress; + } + } +} + QTEST_GUILESS_MAIN( TestLibCalamares ) From 9d395e82f0e1f25af53154f90f14f29961013e37 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Wed, 19 Aug 2020 15:32:19 +0200 Subject: [PATCH 9/9] [libcalamares] Try to avoid progress going backwards This is more a test-inspired hack than anything else: since signals are delivered asynchronously, we can end up delivering progress signals out-of-order, and then the signal spy lists them wrong: progress goes backwards. Insert a tiny delay between jobs to allow signals to be delivered in-order. --- src/libcalamares/JobQueue.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcalamares/JobQueue.cpp b/src/libcalamares/JobQueue.cpp index 17cf11829..efe0392e5 100644 --- a/src/libcalamares/JobQueue.cpp +++ b/src/libcalamares/JobQueue.cpp @@ -134,6 +134,7 @@ public: message = result.message(); details = result.details(); } + QThread::msleep( 16 ); // Very brief rest before reporting the job as complete emitProgress( 1.0 ); // 100% for *this job* } m_jobIndex++; @@ -158,7 +159,6 @@ public: qreal progress = 0.0; if ( m_jobIndex < m_runningJobs->count() ) { - const auto& jobitem = m_runningJobs->at( m_jobIndex ); progress = ( jobitem.cumulative + jobitem.weight * percentage ) / m_overallQueueWeight; message = jobitem.job->prettyStatusMessage();