From 9e4b2d14cb88ab9f7a44a97449f439512490b27b Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 28 Mar 2022 12:29:47 +0200 Subject: [PATCH] [libcalamares] Add a path parameter when creating descriptors This allows us to print the path of a descriptor file in error messages, which in turn makes it easier to find problems with the descriptor files. --- src/calamares/testmain.cpp | 14 +++++--- src/libcalamares/modulesystem/Descriptor.cpp | 33 +++++++++++++++---- src/libcalamares/modulesystem/Descriptor.h | 5 ++- src/libcalamares/modulesystem/Tests.cpp | 9 ++--- .../modulesystem/ModuleManager.cpp | 13 ++++++-- 5 files changed, 56 insertions(+), 18 deletions(-) diff --git a/src/calamares/testmain.cpp b/src/calamares/testmain.cpp index ecbf77888..87d90c882 100644 --- a/src/calamares/testmain.cpp +++ b/src/calamares/testmain.cpp @@ -210,10 +210,11 @@ ExecViewModule::ExecViewModule() { // Normally the module-loader gives the module an instance key // (out of the settings file, or the descriptor of the module). - // We don't have one, so build one -- this gives us "x@x". + // We don't have one, so build one -- this gives us "execView@execView". QVariantMap m; - m.insert( "name", "x" ); - Calamares::Module::initFrom( Calamares::ModuleSystem::Descriptor::fromDescriptorData( m ), "x" ); + const QString execView = QStringLiteral( "execView" ); + m.insert( "name", execView ); + Calamares::Module::initFrom( Calamares::ModuleSystem::Descriptor::fromDescriptorData( m, execView ), execView ); } ExecViewModule::~ExecViewModule() {} @@ -291,7 +292,7 @@ load_module( const ModuleConfig& moduleConfig ) return new ExecViewModule; } - QFileInfo fi; + QFileInfo fi; // This is kept around to hold the path of the module descriptor bool ok = false; QVariantMap descriptor; @@ -357,7 +358,10 @@ load_module( const ModuleConfig& moduleConfig ) cDebug() << Logger::SubEntry << "Module" << moduleName << "job-configuration:" << configFile; Calamares::Module* module = Calamares::moduleFromDescriptor( - Calamares::ModuleSystem::Descriptor::fromDescriptorData( descriptor ), name, configFile, moduleDirectory ); + Calamares::ModuleSystem::Descriptor::fromDescriptorData( descriptor, fi.absoluteFilePath() ), + name, + configFile, + moduleDirectory ); return module; } diff --git a/src/libcalamares/modulesystem/Descriptor.cpp b/src/libcalamares/modulesystem/Descriptor.cpp index 71b50d867..52275b2f0 100644 --- a/src/libcalamares/modulesystem/Descriptor.cpp +++ b/src/libcalamares/modulesystem/Descriptor.cpp @@ -50,9 +50,10 @@ interfaceNames() Descriptor::Descriptor() {} Descriptor -Descriptor::fromDescriptorData( const QVariantMap& moduleDesc ) +Descriptor::fromDescriptorData( const QVariantMap& moduleDesc, const QString& descriptorPath ) { Descriptor d; + Logger::Once o; { bool typeOk = false; @@ -60,7 +61,11 @@ Descriptor::fromDescriptorData( const QVariantMap& moduleDesc ) Type t = typeNames().find( typeValue, typeOk ); if ( !typeOk ) { - cWarning() << "Module descriptor contains invalid *type*" << typeValue; + if ( o ) + { + cWarning() << o << "Descriptor file" << descriptorPath; + } + cWarning() << o << "Module descriptor contains invalid *type*" << typeValue; } bool interfaceOk = false; @@ -68,7 +73,11 @@ Descriptor::fromDescriptorData( const QVariantMap& moduleDesc ) Interface i = interfaceNames().find( interfaceValue, interfaceOk ); if ( !interfaceOk ) { - cWarning() << "Module descriptor contains invalid *interface*" << interfaceValue; + if ( o ) + { + cWarning() << o << "Descriptor file" << descriptorPath; + } + cWarning() << o << "Module descriptor contains invalid *interface*" << interfaceValue; } d.m_name = moduleDesc.value( "name" ).toString(); @@ -102,7 +111,11 @@ Descriptor::fromDescriptorData( const QVariantMap& moduleDesc ) d.m_script = CalamaresUtils::getString( moduleDesc, "script" ); if ( d.m_script.isEmpty() ) { - cWarning() << "Module descriptor contains no *script*" << d.name(); + if ( o ) + { + cWarning() << o << "Descriptor file" << descriptorPath; + } + cWarning() << o << "Module descriptor contains no *script*" << d.name(); d.m_isValid = false; } consumedKeys << "script"; @@ -117,7 +130,11 @@ Descriptor::fromDescriptorData( const QVariantMap& moduleDesc ) } if ( d.m_script.isEmpty() ) { - cWarning() << "Module descriptor contains no *script*" << d.name(); + if ( o ) + { + cWarning() << o << "Descriptor file" << descriptorPath; + } + cWarning() << o << "Module descriptor contains no *script*" << d.name(); d.m_isValid = false; } consumedKeys << "command" @@ -141,7 +158,11 @@ Descriptor::fromDescriptorData( const QVariantMap& moduleDesc ) } if ( !superfluousKeys.isEmpty() ) { - cWarning() << "Module descriptor contains extra keys:" << Logger::DebugList( superfluousKeys ); + if ( o ) + { + cWarning() << o << "Descriptor file" << descriptorPath; + } + cWarning() << o << "Module descriptor contains extra keys:" << Logger::DebugList( superfluousKeys ); d.m_isValid = false; } diff --git a/src/libcalamares/modulesystem/Descriptor.h b/src/libcalamares/modulesystem/Descriptor.h index 614d5f15a..fb1a66309 100644 --- a/src/libcalamares/modulesystem/Descriptor.h +++ b/src/libcalamares/modulesystem/Descriptor.h @@ -58,9 +58,12 @@ public: Descriptor(); /** @brief Fills a descriptor from the loaded (YAML) data. + * + * The @p descriptorPath is used only for debug messages, the + * data is only read from @p moduleDesc. * */ - static Descriptor fromDescriptorData( const QVariantMap& moduleDesc ); + static Descriptor fromDescriptorData( const QVariantMap& moduleDesc, const QString& descriptorPath ); bool isValid() const { return m_isValid; } diff --git a/src/libcalamares/modulesystem/Tests.cpp b/src/libcalamares/modulesystem/Tests.cpp index 48da558e2..13a9c864d 100644 --- a/src/libcalamares/modulesystem/Tests.cpp +++ b/src/libcalamares/modulesystem/Tests.cpp @@ -132,9 +132,10 @@ ModuleSystemTests::testBadFromStringCases() void ModuleSystemTests::testBasicDescriptor() { + const QString path = QStringLiteral( "/bogus.desc" ); { QVariantMap m; - auto d = Calamares::ModuleSystem::Descriptor::fromDescriptorData( m ); + auto d = Calamares::ModuleSystem::Descriptor::fromDescriptorData( m, path ); QVERIFY( !d.isValid() ); QVERIFY( d.name().isEmpty() ); @@ -142,7 +143,7 @@ ModuleSystemTests::testBasicDescriptor() { QVariantMap m; m.insert( "name", QVariant() ); - auto d = Calamares::ModuleSystem::Descriptor::fromDescriptorData( m ); + auto d = Calamares::ModuleSystem::Descriptor::fromDescriptorData( m, path ); QVERIFY( !d.isValid() ); QVERIFY( d.name().isEmpty() ); @@ -150,7 +151,7 @@ ModuleSystemTests::testBasicDescriptor() { QVariantMap m; m.insert( "name", 17 ); - auto d = Calamares::ModuleSystem::Descriptor::fromDescriptorData( m ); + auto d = Calamares::ModuleSystem::Descriptor::fromDescriptorData( m, path ); QVERIFY( !d.isValid() ); QVERIFY( !d.name().isEmpty() ); @@ -161,7 +162,7 @@ ModuleSystemTests::testBasicDescriptor() m.insert( "name", "welcome" ); m.insert( "type", "viewmodule" ); m.insert( "interface", "qtplugin" ); - auto d = Calamares::ModuleSystem::Descriptor::fromDescriptorData( m ); + auto d = Calamares::ModuleSystem::Descriptor::fromDescriptorData( m, path ); // QVERIFY( !d.isValid() ); QCOMPARE( d.name(), QStringLiteral( "welcome" ) ); diff --git a/src/libcalamaresui/modulesystem/ModuleManager.cpp b/src/libcalamaresui/modulesystem/ModuleManager.cpp index a957c0c30..1e397b340 100644 --- a/src/libcalamaresui/modulesystem/ModuleManager.cpp +++ b/src/libcalamaresui/modulesystem/ModuleManager.cpp @@ -104,11 +104,20 @@ ModuleManager::doInit() if ( ok && !moduleName.isEmpty() && ( moduleName == currentDir.dirName() ) && !m_availableDescriptorsByModuleName.contains( moduleName ) ) { - auto descriptor - = Calamares::ModuleSystem::Descriptor::fromDescriptorData( moduleDescriptorMap ); + auto descriptor = Calamares::ModuleSystem::Descriptor::fromDescriptorData( + moduleDescriptorMap, descriptorFileInfo.absoluteFilePath() ); descriptor.setDirectory( descriptorFileInfo.absoluteDir().absolutePath() ); m_availableDescriptorsByModuleName.insert( moduleName, descriptor ); } + else + { + // Duplicate modules are ok; other issues like empty name or dir-mismatch are reported. + if ( !m_availableDescriptorsByModuleName.contains( moduleName ) ) + { + cWarning() << deb << "ModuleManager module descriptor" + << descriptorFileInfo.absoluteFilePath() << "has bad name" << moduleName; + } + } } else {