Merge branch 'fixup-module-weights'

src/calamares/CalamaresApplication.cpp

@@ -39,6 +39,7 @@
 #include <QDesktopWidget>
 #include <QDir>
 #include <QFileInfo>
+#include <QTimer>
 
 
 CalamaresApplication::CalamaresApplication( int& argc, char* argv[] )
@@ -354,7 +355,7 @@ CalamaresApplication::initView()
     connect( m_moduleManager, &Calamares::ModuleManager::modulesLoaded, this, &CalamaresApplication::initViewSteps );
     connect( m_moduleManager, &Calamares::ModuleManager::modulesFailed, this, &CalamaresApplication::initFailed );
 
-    m_moduleManager->loadModules();
+    QTimer::singleShot( 0, m_moduleManager, &Calamares::ModuleManager::loadModules );
 
     m_mainwindow->move( this->desktop()->availableGeometry().center() - m_mainwindow->rect().center() );
 

src/libcalamares/CMakeLists.txt

@@ -207,6 +207,16 @@ if ( ECM_FOUND AND BUILD_TESTING )
             Qt5::Test
     )
     calamares_automoc( libcalamaresnetworktest )
+
+    ecm_add_test(
+            modulesystem/Tests.cpp
+        TEST_NAME
+            libcalamaresmodulesystemtest
+        LINK_LIBRARIES
+            calamares
+            Qt5::Test
+    )
+    calamares_automoc( libcalamaresmodulesystemtest )
 endif()
 
 if( BUILD_TESTING )

src/libcalamares/Settings.cpp

@@ -169,14 +169,14 @@ interpretSequence( const YAML::Node& node, Settings::ModuleSequence& moduleSeque
                 continue;
             }
             QString thisActionS = sequenceVListItem.toMap().firstKey();
-            ModuleAction thisAction;
+            ModuleSystem::Action thisAction;
             if ( thisActionS == "show" )
             {
-                thisAction = ModuleAction::Show;
+                thisAction = ModuleSystem::Action::Show;
             }
             else if ( thisActionS == "exec" )
             {
-                thisAction = ModuleAction::Exec;
+                thisAction = ModuleSystem::Action::Exec;
             }
             else
             {

src/libcalamares/Settings.h

@@ -46,7 +46,7 @@ public:
     using InstanceDescriptionList = QList< InstanceDescription >;
     InstanceDescriptionList customModuleInstances() const;
 
-    using ModuleSequence = QList< QPair< ModuleAction, QStringList > >;
+    using ModuleSequence = QList< QPair< ModuleSystem::Action, QStringList > >;
     ModuleSequence modulesSequence() const;
 
     QString brandingComponentName() const;

src/libcalamares/modulesystem/Actions.h

@@ -22,13 +22,16 @@
 
 namespace Calamares
 {
+namespace ModuleSystem
+{
 
-enum class ModuleAction : char
+enum class Action : char
 {
     Show,
     Exec
 };
 
+}  // namespace ModuleSystem
 }  // namespace Calamares
 
 #endif

src/libcalamares/modulesystem/InstanceKey.h

@@ -0,0 +1,111 @@
+/* === This file is part of Calamares - <https://github.com/calamares> ===
+ *
+ *   Copyright 2014-2015, Teo Mrnjavac <teo@kde.org>
+ *   Copyright 2018-2019, Adriaan de Groot <groot@kde.org>
+ *
+ *   Calamares is free software: you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation, either version 3 of the License, or
+ *   (at your option) any later version.
+ *
+ *   Calamares is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with Calamares. If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef MODULESYSTEM_INSTANCEKEY_H
+#define MODULESYSTEM_INSTANCEKEY_H
+
+#include <QList>
+#include <QPair>
+#include <QString>
+#include <QStringList>
+
+namespace Logger
+{
+class CLog;
+}
+
+namespace Calamares
+{
+namespace ModuleSystem
+{
+
+/** @brief A module instance's key (`module@id`)
+ *
+ * A module instance is identified by both the module's name
+ * (a Calamares module, e.g. `users`) and an instance id.
+ * Usually, the instance id is the same as the module name
+ * and the whole module instance key is `users@users`, but
+ * it is possible to use the same module more than once
+ * and then you distinguish those module instances by their
+ * secondary id (e.g. `users@one`).
+ *
+ * This is supported by the *instances* configuration entry
+ * in `settings.conf`.
+ */
+class InstanceKey : public QPair< QString, QString >
+{
+public:
+    /// @brief Create an instance key from explicit module and id.
+    InstanceKey( const QString& module, const QString& id )
+        : QPair( module, id )
+    {
+        if ( second.isEmpty() )
+        {
+            second = first;
+        }
+        validate();
+    }
+
+    /// @brief Create unusual, invalid instance key
+    InstanceKey()
+        : QPair( QString(), QString() )
+    {
+    }
+
+    /// @brief A valid module has both name and id
+    bool isValid() const { return !first.isEmpty() && !second.isEmpty(); }
+
+    /// @brief A custom module has a non-default id
+    bool isCustom() const { return first != second; }
+
+    QString module() const { return first; }
+    QString id() const { return second; }
+
+    /// @brief Create instance key from stringified version
+    static InstanceKey fromString( const QString& s )
+    {
+        QStringList moduleEntrySplit = s.split( '@' );
+        if ( moduleEntrySplit.length() < 1 || moduleEntrySplit.length() > 2 )
+        {
+            return InstanceKey();
+        }
+        // For length 1, first == last
+        return InstanceKey( moduleEntrySplit.first(), moduleEntrySplit.last() );
+    }
+
+    QString toString() const
+    {
+        return first + '@' + second;
+    }
+
+private:
+    /** @brief Check validity and reset module and id if needed. */
+    void validate()
+    {
+        if ( first.contains( '@' ) || second.contains( '@' ) )
+        {
+            first = QString();
+            second = QString();
+        }
+    }
+};
+
+}  // namespace ModuleSystem
+}  // namespace Calamares
+
+#endif

src/libcalamares/modulesystem/Tests.cpp

@@ -0,0 +1,133 @@
+/* === This file is part of Calamares - <https://github.com/calamares> ===
+ *
+ *   Copyright 2019, Adriaan de Groot <groot@kde.org>
+ *
+ *   Calamares is free software: you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation, either version 3 of the License, or
+ *   (at your option) any later version.
+ *
+ *   Calamares is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with Calamares. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "modulesystem/InstanceKey.h"
+
+#include <QtTest/QtTest>
+
+using Calamares::ModuleSystem::InstanceKey;
+
+class ModuleSystemTests : public QObject
+{
+    Q_OBJECT
+public:
+    ModuleSystemTests() {}
+    virtual ~ModuleSystemTests() {}
+
+private Q_SLOTS:
+    void initTestCase();
+
+    void testEmptyInstanceKey();
+    void testCustomInstanceKey();
+    void testFromStringInstanceKey();
+
+    void testBadSimpleCases();
+    void testBadFromStringCases();
+};
+
+void
+ModuleSystemTests::initTestCase()
+{
+}
+
+void
+assert_is_invalid( const InstanceKey& k )
+{
+    QVERIFY( !k.isValid() );
+    QVERIFY( !k.isCustom() );
+    QVERIFY( k.module().isEmpty() );
+    QVERIFY( k.id().isEmpty() );
+    QVERIFY( k.toString().isEmpty() );
+}
+
+void
+ModuleSystemTests::testEmptyInstanceKey()
+{
+    InstanceKey k0;
+    assert_is_invalid( k0 );
+}
+
+void
+ModuleSystemTests::testCustomInstanceKey()
+{
+    InstanceKey k0( "derp", "derp" );
+    QVERIFY( k0.isValid() );
+    QVERIFY( !k0.isCustom() );
+    QCOMPARE( k0.module(), QStringLiteral( "derp" ) );
+    QCOMPARE( k0.id(), QStringLiteral( "derp" ) );
+    QCOMPARE( k0.toString(), QStringLiteral( "derp@derp" ) );
+
+    InstanceKey k1( "derp", "horse" );
+    QVERIFY( k1.isValid() );
+    QVERIFY( k1.isCustom() );
+    QCOMPARE( k1.module(), QStringLiteral( "derp" ) );
+    QCOMPARE( k1.id(), QStringLiteral( "horse" ) );
+    QCOMPARE( k1.toString(), QStringLiteral( "derp@horse" ) );
+
+    InstanceKey k4( "derp", QString() );
+    QVERIFY( k4.isValid() );
+    QVERIFY( !k4.isCustom() );
+    QCOMPARE( k4.module(), QStringLiteral( "derp" ) );
+    QCOMPARE( k4.id(), QStringLiteral( "derp" ) );
+    QCOMPARE( k4.toString(), QStringLiteral( "derp@derp" ) );
+}
+
+void
+ModuleSystemTests::testFromStringInstanceKey()
+{
+    InstanceKey k0 = InstanceKey::fromString( "derp@derp" );
+    QVERIFY( k0.isValid() );
+    QVERIFY( !k0.isCustom() );
+    QCOMPARE( k0.module(), QStringLiteral( "derp" ) );
+    QCOMPARE( k0.id(), QStringLiteral( "derp" ) );
+
+    InstanceKey k1 = InstanceKey::fromString( "derp@horse" );
+    QVERIFY( k1.isValid() );
+    QVERIFY( k1.isCustom() );
+    QCOMPARE( k1.module(), QStringLiteral( "derp" ) );
+    QCOMPARE( k1.id(), QStringLiteral( "horse" ) );
+
+    InstanceKey k2 = InstanceKey::fromString( "derp" );
+    QVERIFY( k2.isValid() );
+    QVERIFY( !k2.isCustom() );
+    QCOMPARE( k2.module(), QStringLiteral( "derp" ) );
+    QCOMPARE( k2.id(), QStringLiteral( "derp" ) );
+}
+
+/// @brief These are expected to fail since they show bugs in the code
+void
+ModuleSystemTests::testBadSimpleCases()
+{
+    InstanceKey k4( "derp", "derp@derp" );
+    assert_is_invalid( k4 );
+}
+
+void
+ModuleSystemTests::testBadFromStringCases()
+{
+    InstanceKey k0 = InstanceKey::fromString( QString() );
+    assert_is_invalid( k0 );
+
+    k0 = InstanceKey::fromString( "derp@derp@derp" );
+    assert_is_invalid( k0 );
+}
+
+
+QTEST_GUILESS_MAIN( ModuleSystemTests )
+
+#include "Tests.moc"

src/libcalamaresui/modulesystem/ModuleManager.cpp

@@ -137,7 +137,12 @@ ModuleManager::doInit()
 QStringList
 ModuleManager::loadedInstanceKeys()
 {
-    return m_loadedModulesByInstanceKey.keys();
+    QStringList l;
+    for ( const auto& m : m_loadedModulesByInstanceKey.keys() )
+    {
+        l << m.toString();
+    }
+    return l;
 }
 
 
@@ -150,7 +155,7 @@ ModuleManager::moduleDescriptor( const QString& name )
 Module*
 ModuleManager::moduleInstance( const QString& instanceKey )
 {
-    return m_loadedModulesByInstanceKey.value( instanceKey );
+    return m_loadedModulesByInstanceKey.value( ModuleSystem::InstanceKey::fromString( instanceKey ) );
 }
 
 
@@ -160,12 +165,12 @@ ModuleManager::moduleInstance( const QString& instanceKey )
  * @return -1 on failure, otherwise index of the instance that matches.
  */
 static int
-findCustomInstance( const Settings::InstanceDescriptionList& customInstances, const QString& module, const QString& id )
+findCustomInstance( const Settings::InstanceDescriptionList& customInstances, const ModuleSystem::InstanceKey& m )
 {
     for ( int i = 0; i < customInstances.count(); ++i )
     {
         const auto& thisInstance = customInstances[ i ];
-        if ( thisInstance.value( "module" ) == module && thisInstance.value( "id" ) == id )
+        if ( thisInstance.value( "module" ) == m.module() && thisInstance.value( "id" ) == m.id() )
         {
             return i;
         }
@@ -177,136 +182,131 @@ findCustomInstance( const Settings::InstanceDescriptionList& customInstances, co
 void
 ModuleManager::loadModules()
 {
-    QTimer::singleShot( 0, this, [this]() {
+    QStringList failedModules = checkDependencies();
-        QStringList failedModules = checkDependencies();
+    Settings::InstanceDescriptionList customInstances = Settings::instance()->customModuleInstances();
-        Settings::InstanceDescriptionList customInstances = Settings::instance()->customModuleInstances();
 
-        const auto modulesSequence
+    const auto modulesSequence
-            = failedModules.isEmpty() ? Settings::instance()->modulesSequence() : Settings::ModuleSequence();
+        = failedModules.isEmpty() ? Settings::instance()->modulesSequence() : Settings::ModuleSequence();
-        for ( const auto& modulePhase : modulesSequence )
+    for ( const auto& modulePhase : modulesSequence )
+    {
+        ModuleSystem::Action currentAction = modulePhase.first;
+
+        foreach ( const QString& moduleEntry, modulePhase.second )
         {
-            ModuleAction currentAction = modulePhase.first;
+            auto instanceKey = ModuleSystem::InstanceKey::fromString( moduleEntry );
-
+            if ( !instanceKey.isValid() )
-            foreach ( const QString& moduleEntry, modulePhase.second )
             {
-                QStringList moduleEntrySplit = moduleEntry.split( '@' );
+                cError() << "Wrong module entry format for module" << moduleEntry;
-                QString moduleName;
+                failedModules.append( moduleEntry );
-                QString instanceId;
+                continue;
-                QString configFileName;
+            }
-                if ( moduleEntrySplit.length() < 1 || moduleEntrySplit.length() > 2 )
+
+
+            if ( !m_availableDescriptorsByModuleName.contains( instanceKey.module() )
+                    || m_availableDescriptorsByModuleName.value( instanceKey.module() ).isEmpty() )
+            {
+                cError() << "Module" << instanceKey.toString() << "not found in module search paths."
+                            << Logger::DebugList( m_paths );
+                failedModules.append( instanceKey.toString() );
+                continue;
+            }
+
+            QString configFileName;
+            if ( instanceKey.isCustom() )
+            {
+                int found = findCustomInstance( customInstances, instanceKey );
+
+                if ( found > -1 )
                 {
-                    cError() << "Wrong module entry format for module" << moduleEntry;
+                    configFileName = customInstances[ found ].value( "config" );
+                }
+                else  //ought to be a custom instance, but cannot find instance entry
+                {
+                    cError() << "Custom instance" << moduleEntry << "not found in custom instances section.";
                     failedModules.append( moduleEntry );
                     continue;
                 }
-                moduleName = moduleEntrySplit.first();
+            }
-                instanceId = moduleEntrySplit.last();
+            else
-                configFileName = QString( "%1.conf" ).arg( moduleName );
+            {
+                configFileName = QString( "%1.conf" ).arg( instanceKey.module() );
+            }
 
-                if ( !m_availableDescriptorsByModuleName.contains( moduleName )
+            // So now we can assume that the module entry is at least valid,
-                     || m_availableDescriptorsByModuleName.value( moduleName ).isEmpty() )
+            // that we have a descriptor on hand (and therefore that the
+            // module exists), and that the instance is either default or
+            // defined in the custom instances section.
+            // We still don't know whether the config file for the entry
+            // exists and is valid, but that's the only thing that could fail
+            // from this point on. -- Teo 8/2015
+            Module* thisModule = m_loadedModulesByInstanceKey.value( instanceKey, nullptr );
+            if ( thisModule && !thisModule->isLoaded() )
+            {
+                cError() << "Module" << instanceKey.toString() << "exists but not loaded.";
+                failedModules.append( instanceKey.toString() );
+                continue;
+            }
+
+            if ( thisModule && thisModule->isLoaded() )
+            {
+                cDebug() << "Module" << instanceKey.toString() << "already loaded.";
+            }
+            else
+            {
+                thisModule = Module::fromDescriptor( m_availableDescriptorsByModuleName.value( instanceKey.module() ),
+                                                        instanceKey.id(),
+                                                        configFileName,
+                                                        m_moduleDirectoriesByModuleName.value( instanceKey.module() ) );
+                if ( !thisModule )
                 {
-                    cError() << "Module" << moduleName << "not found in module search paths."
+                    cError() << "Module" << instanceKey.toString() << "cannot be created from descriptor" << configFileName;
-                             << Logger::DebugList( m_paths );
+                    failedModules.append( instanceKey.toString() );
-                    failedModules.append( moduleName );
                     continue;
                 }
 
-                if ( moduleName != instanceId )  //means this is a custom instance
+                if ( !checkDependencies( *thisModule ) )
                 {
-                    int found = findCustomInstance( customInstances, moduleName, instanceId );
+                    // Error message is already printed
-
+                    failedModules.append( instanceKey.toString() );
-                    if ( found > -1 )
-                    {
-                        configFileName = customInstances[ found ].value( "config" );
-                    }
-                    else  //ought to be a custom instance, but cannot find instance entry
-                    {
-                        cError() << "Custom instance" << moduleEntry << "not found in custom instances section.";
-                        failedModules.append( moduleEntry );
-                        continue;
-                    }
-                }
-
-                // So now we can assume that the module entry is at least valid,
-                // that we have a descriptor on hand (and therefore that the
-                // module exists), and that the instance is either default or
-                // defined in the custom instances section.
-                // We still don't know whether the config file for the entry
-                // exists and is valid, but that's the only thing that could fail
-                // from this point on. -- Teo 8/2015
-
-                QString instanceKey = QString( "%1@%2" ).arg( moduleName ).arg( instanceId );
-
-                Module* thisModule = m_loadedModulesByInstanceKey.value( instanceKey, nullptr );
-                if ( thisModule && !thisModule->isLoaded() )
-                {
-                    cError() << "Module" << instanceKey << "exists but not loaded.";
-                    failedModules.append( instanceKey );
                     continue;
                 }
 
-                if ( thisModule && thisModule->isLoaded() )
+                // If it's a ViewModule, it also appends the ViewStep to the ViewManager.
+                thisModule->loadSelf();
+                m_loadedModulesByInstanceKey.insert( instanceKey, thisModule );
+                if ( !thisModule->isLoaded() )
                 {
-                    cDebug() << "Module" << instanceKey << "already loaded.";
+                    cError() << "Module" << instanceKey.toString() << "loading FAILED.";
-                }
+                    failedModules.append( instanceKey.toString() );
-                else
+                    continue;
-                {
-                    thisModule = Module::fromDescriptor( m_availableDescriptorsByModuleName.value( moduleName ),
-                                                         instanceId,
-                                                         configFileName,
-                                                         m_moduleDirectoriesByModuleName.value( moduleName ) );
-                    if ( !thisModule )
-                    {
-                        cError() << "Module" << instanceKey << "cannot be created from descriptor" << configFileName;
-                        failedModules.append( instanceKey );
-                        continue;
-                    }
-
-                    if ( !checkDependencies( *thisModule ) )
-                    {
-                        // Error message is already printed
-                        failedModules.append( instanceKey );
-                        continue;
-                    }
-
-                    // If it's a ViewModule, it also appends the ViewStep to the ViewManager.
-                    thisModule->loadSelf();
-                    m_loadedModulesByInstanceKey.insert( instanceKey, thisModule );
-                    if ( !thisModule->isLoaded() )
-                    {
-                        cError() << "Module" << instanceKey << "loading FAILED.";
-                        failedModules.append( instanceKey );
-                        continue;
-                    }
-                }
-
-                // At this point we most certainly have a pointer to a loaded module in
-                // thisModule. We now need to enqueue jobs info into an EVS.
-                if ( currentAction == ModuleAction::Exec )
-                {
-                    ExecutionViewStep* evs
-                        = qobject_cast< ExecutionViewStep* >( Calamares::ViewManager::instance()->viewSteps().last() );
-                    if ( !evs )  // If the last step is not an EVS, we must create it.
-                    {
-                        evs = new ExecutionViewStep( ViewManager::instance() );
-                        ViewManager::instance()->addViewStep( evs );
-                    }
-
-                    evs->appendJobModuleInstanceKey( instanceKey );
                 }
             }
+
+            // At this point we most certainly have a pointer to a loaded module in
+            // thisModule. We now need to enqueue jobs info into an EVS.
+            if ( currentAction == ModuleSystem::Action::Exec )
+            {
+                ExecutionViewStep* evs
+                    = qobject_cast< ExecutionViewStep* >( Calamares::ViewManager::instance()->viewSteps().last() );
+                if ( !evs )  // If the last step is not an EVS, we must create it.
+                {
+                    evs = new ExecutionViewStep( ViewManager::instance() );
+                    ViewManager::instance()->addViewStep( evs );
+                }
+
+                evs->appendJobModuleInstanceKey( instanceKey.toString() );
+            }
         }
-        if ( !failedModules.isEmpty() )
+    }
-        {
+    if ( !failedModules.isEmpty() )
-            ViewManager::instance()->onInitFailed( failedModules );
+    {
-            emit modulesFailed( failedModules );
+        ViewManager::instance()->onInitFailed( failedModules );
-        }
+        emit modulesFailed( failedModules );
-        else
+    }
-        {
+    else
-            emit modulesLoaded();
+    {
-        }
+        emit modulesLoaded();
-    } );
+    }
 }
 
 void

src/libcalamaresui/modulesystem/ModuleManager.h

@@ -20,6 +20,8 @@
 #ifndef MODULELOADER_H
 #define MODULELOADER_H
 
+#include "modulesystem/InstanceKey.h"
+
 #include "Requirement.h"
 
 #include <QObject>
@@ -77,8 +79,9 @@ public:
     Module* moduleInstance( const QString& instanceKey );
 
     /**
-     * @brief loadModules initiates the asynchronous module loading operation.
+     * @brief loadModules does all of the module loading operation.
      * When this is done, the signal modulesLoaded is emitted.
+     * It is recommended to call this from a single-shot QTimer.
      */
     void loadModules();
 
@@ -123,7 +126,7 @@ private:
 
     QMap< QString, QVariantMap > m_availableDescriptorsByModuleName;
     QMap< QString, QString > m_moduleDirectoriesByModuleName;
-    QMap< QString, Module* > m_loadedModulesByInstanceKey;
+    QMap< ModuleSystem::InstanceKey, Module* > m_loadedModulesByInstanceKey;
     const QStringList m_paths;
 
     static ModuleManager* s_instance;