Merge branch 'calamares' of https://github.com/calamares/calamares into development

CHANGES-3.2

@@ -7,20 +7,52 @@ contributors are listed. Note that Calamares does not have a historical
 changelog -- this log starts with version 3.2.0. The release notes on the
 website will have to do for older versions.
 
-# 3.2.49 (unreleased) #
+# 3.2.50 (unreleased) #
+
+This release contains contributions from (alphabetically by first name):
+ - Erik Dubois
+
+## Core ##
+ - No core changes yet
+
+## Modules ##
+ - *networkcfg* could fail to update the NetworkManager configuration
+   if the SSID or username contained non-ASCII characters **and** the
+   default Python text-file encoding was set to ASCII. The files are
+   now read and written in UTF-8, explicitly. #1848
+ - *preservefiles* was missing some necessary features, needed for it
+   to replace the deprecated log-file-saving functionality in the *umount*
+   module. (Thanks Erik and Joe for testing) #1851
+
+
+# 3.2.49.1 (2021-12-11) #
+
+This is a hot-fix release, to fix a regression in the calculation of
+swap-size. Reported by EndeavourOS (Joe Kamprad) and Xero Linux.
+
+
+# 3.2.49 (2021-12-10) #
 
 This release contains contributions from (alphabetically by first name):
  - Artem Grinev
  - Evan James
 
+Distributions are **specifically** reminded to update the *umount* module
+configuration (and to use *preservefiles* if needed).
+
 ## Core ##
  - Errors (e.g. when an installation fails for whatever reason) are displayed
    in a dialog with a scrollable details panel, rather than growing up
    to the size of the screen. (Thanks Artem)
 
 ## Modules ##
+ - *bootloader* better supports multiple installations of the same OS.
+ - *mount* supports btrfs subvolumes on subdirectories of / now.
  - *partition* now supports "deep" btrfs subvolume names, e.g. a
    separate subvolume for `/usr/local`. (Thanks Evan)
+ - The *umount* module now warns if the "preserve log file" feature is used.
+   This has been deprecated for a long time: use the *preservefiles* module
+   instead. A future release will turn this into an error.
 
 
 # 3.2.48 (2021-12-03) #

CMakeLists.txt

@@ -41,7 +41,7 @@
 # TODO:3.3: Require CMake 3.12
 cmake_minimum_required( VERSION 3.3 FATAL_ERROR )
 project( CALAMARES
-    VERSION 3.2.49
+    VERSION 3.2.50
     LANGUAGES C CXX
 )
 

lang/calamares_sv.ts

@@ -703,7 +703,7 @@ Alla ändringar kommer att gå förlorade.</translation>
     <message>
       <location filename="../src/modules/partition/jobs/ClearMountsJob.cpp" line="306"/>
       <source>Successfully closed mapper device %1.</source>
-      <translation type="unfinished"/>
+      <translation>Framgångsrikt stängde krypterad enhet %1.</translation>
     </message>
     <message>
       <location filename="../src/modules/partition/jobs/ClearMountsJob.cpp" line="319"/>

src/libcalamares/utils/String.cpp

@@ -224,5 +224,26 @@ truncateMultiLine( const QString& string, CalamaresUtils::LinesStartEnd lines, C
     return front + back.right( chars.total / 2 );
 }
 
+void
+removeLeading( QString& string, QChar c )
+{
+    int count = 0;
+    while ( string.length() > count && string[ count ] == c )
+    {
+        count++;
+    }
+    string.remove( 0, count );
+}
+
+void
+removeTrailing( QString& string, QChar c )
+{
+    int lastIndex = string.length();
+    while ( lastIndex > 0 && string[ lastIndex - 1 ] == c )
+    {
+        lastIndex--;
+    }
+    string.remove( lastIndex, string.length() );
+}
 
 }  // namespace CalamaresUtils

src/libcalamares/utils/String.h

@@ -100,6 +100,19 @@ DLLEXPORT QString truncateMultiLine( const QString& string,
                                      LinesStartEnd lines = LinesStartEnd { 3, 5 },
                                      CharCount chars = CharCount { 812 } );
 
+/** @brief Remove all @p c at the beginning of @p string
+ *
+ * Modifies the @p string in-place. If @p c is not the first character
+ * of @p string, the string is left unchanged; otherwise the first character
+ * is removed and the process repeats.
+ */
+DLLEXPORT void removeLeading( QString& string, QChar c );
+/** @brief Remove all @p c at the end of @p string
+ *
+ * Like removeLeading(), but at the end of the string.
+ */
+DLLEXPORT void removeTrailing( QString& string, QChar c );
+
 }  // namespace CalamaresUtils
 
 #endif

src/libcalamares/utils/Tests.cpp

@@ -70,6 +70,10 @@ private Q_SLOTS:
     void testStringTruncation();
     void testStringTruncationShorter();
     void testStringTruncationDegenerate();
+    void testStringRemoveLeading_data();
+    void testStringRemoveLeading();
+    void testStringRemoveTrailing_data();
+    void testStringRemoveTrailing();
 
     /** @section Test Runner directory-manipulation. */
     void testRunnerDirs();
@@ -754,6 +758,64 @@ LibCalamaresTests::testStringTruncationDegenerate()
     }
 }
 
+void
+LibCalamaresTests::testStringRemoveLeading_data()
+{
+    QTest::addColumn< QString >( "string" );
+    QTest::addColumn< char >( "c" );
+    QTest::addColumn< QString >( "result" );
+
+    QTest::newRow( "empty" ) << QString() << '/' << QString();
+    QTest::newRow( "one-slash" ) << QStringLiteral( "/tmp" ) << '/' << QStringLiteral( "tmp" );
+    QTest::newRow( "two-slash" ) << QStringLiteral( "//tmp" ) << '/' << QStringLiteral( "tmp" );
+    QTest::newRow( "multi-slash" ) << QStringLiteral( "/tmp/p" ) << '/' << QStringLiteral( "tmp/p" );
+    QTest::newRow( "later-slash" ) << QStringLiteral( "@/tmp" ) << '/' << QStringLiteral( "@/tmp" );
+    QTest::newRow( "all-one-slash" ) << QStringLiteral( "/" ) << '/' << QString();
+    QTest::newRow( "all-many-slash" ) << QStringLiteral( "////////////////////" ) << '/' << QString();
+    QTest::newRow( "trailing" ) << QStringLiteral( "tmp/" ) << '/' << QStringLiteral( "tmp/" );
+}
+
+void
+LibCalamaresTests::testStringRemoveLeading()
+{
+    QFETCH( QString, string );
+    QFETCH( char, c );
+    QFETCH( QString, result );
+
+    const QString initial = string;
+    CalamaresUtils::removeLeading( string, c );
+    QCOMPARE( string, result );
+}
+
+void
+LibCalamaresTests::testStringRemoveTrailing_data()
+{
+    QTest::addColumn< QString >( "string" );
+    QTest::addColumn< char >( "c" );
+    QTest::addColumn< QString >( "result" );
+
+    QTest::newRow( "empty" ) << QString() << '/' << QString();
+    QTest::newRow( "one-slash" ) << QStringLiteral( "/tmp" ) << '/' << QStringLiteral( "/tmp" );
+    QTest::newRow( "two-slash" ) << QStringLiteral( "//tmp" ) << '/' << QStringLiteral( "//tmp" );
+    QTest::newRow( "multi-slash" ) << QStringLiteral( "/tmp//p/" ) << '/' << QStringLiteral( "/tmp//p" );
+    QTest::newRow( "later-slash" ) << QStringLiteral( "@/tmp/@" ) << '/' << QStringLiteral( "@/tmp/@" );
+    QTest::newRow( "later-slash2" ) << QStringLiteral( "@/tmp/@//" ) << '/' << QStringLiteral( "@/tmp/@" );
+    QTest::newRow( "all-one-slash" ) << QStringLiteral( "/" ) << '/' << QString();
+    QTest::newRow( "all-many-slash" ) << QStringLiteral( "////////////////////" ) << '/' << QString();
+    QTest::newRow( "trailing" ) << QStringLiteral( "tmp/" ) << '/' << QStringLiteral( "tmp" );
+}
+
+void
+LibCalamaresTests::testStringRemoveTrailing()
+{
+    QFETCH( QString, string );
+    QFETCH( char, c );
+    QFETCH( QString, result );
+
+    const QString initial = string;
+    CalamaresUtils::removeTrailing( string, c );
+    QCOMPARE( string, result );
+}
 
 static QString
 dirname( const QTemporaryDir& d )
@@ -1001,10 +1063,10 @@ LibCalamaresTests::testReadWriteFile()
     {
         auto fullPath = ss->createTargetFile( "test0", otherContents );
         QVERIFY( !fullPath );  // Failed, because it won't overwrite
-        QCOMPARE( fullPath.code(), decltype(fullPath)::Code::AlreadyExists );
-        QVERIFY( fullPath.path().isEmpty() ); // Because it wasn't written
+        QCOMPARE( fullPath.code(), decltype( fullPath )::Code::AlreadyExists );
+        QVERIFY( fullPath.path().isEmpty() );  // Because it wasn't written
 
-        QFileInfo fi( tempRoot.filePath( "test0" ) ); // Compute the name some other way
+        QFileInfo fi( tempRoot.filePath( "test0" ) );  // Compute the name some other way
         QVERIFY( fi.exists() );
         QVERIFY( fi.isFile() );
         QCOMPARE( fi.size(), 0 );

src/modules/networkcfg/main.py

@@ -73,12 +73,12 @@ def replace_username(nm_config_filename, live_user, target_user):
     if not os.path.exists(nm_config_filename):
         return
 
-    with open(nm_config_filename, "r") as network_conf:
+    with open(nm_config_filename, "r", encoding="UTF-8") as network_conf:
         text = network_conf.readlines()
 
     live_permissions = 'permissions=user:{}:;'.format(live_user)
     target_permissions = 'permissions=user:{}:;\n'.format(target_user)
-    with open(nm_config_filename, "w") as network_conf:
+    with open(nm_config_filename, "w", encoding="UTF-8") as network_conf:
         for line in text:
             if live_permissions in line:
                 line = target_permissions

src/modules/partition/core/PartitionActions.cpp

@@ -71,7 +71,7 @@ swapSuggestion( const qint64 availableSpaceB, Config::SwapChoice swap )
 
 
     // Allow for a fudge factor
-    suggestedSwapSizeB = qRound( suggestedSwapSizeB * overestimationFactor );
+    suggestedSwapSizeB = qRound64( suggestedSwapSizeB * overestimationFactor );
 
     // don't use more than 10% of available space
     if ( !ensureSuspendToDisk )

src/modules/partition/gui/DeviceInfoWidget.cpp

@@ -100,7 +100,9 @@ DeviceInfoWidget::retranslateUi()
                             "that makes a file accessible as a block device. "
                             "This kind of setup usually only contains a single filesystem." );
         break;
+#if defined( WITH_KPMCORE42API )
     case PartitionTable::none:
+#endif
     case PartitionTable::unknownTableType:
         typeString = " ? ";
         toolTipString = tr( "This installer <strong>cannot detect a partition table</strong> on the "

src/modules/preservefiles/CMakeLists.txt

@@ -3,14 +3,20 @@
 #   SPDX-FileCopyrightText: 2020 Adriaan de Groot <groot@kde.org>
 #   SPDX-License-Identifier: BSD-2-Clause
 #
-include_directories( ${PROJECT_BINARY_DIR}/src/libcalamaresui )
-
 calamares_add_plugin( preservefiles
     TYPE job
     EXPORT_MACRO PLUGINDLLEXPORT_PRO
     SOURCES
+        Item.cpp
         PreserveFiles.cpp
     # REQUIRES mount  # To set the rootMountPoint
     SHARED_LIB
     EMERGENCY
 )
+
+calamares_add_test(
+    preservefilestest
+    SOURCES
+        Item.cpp
+        Tests.cpp
+)

src/modules/preservefiles/Item.cpp

@@ -0,0 +1,159 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *  SPDX-FileCopyrightText: 2018, 2021 Adriaan de Groot <groot@kde.org>
+ *  SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ */
+
+#include "Item.h"
+
+#include "GlobalStorage.h"
+#include "JobQueue.h"
+#include "utils/CalamaresUtilsSystem.h"
+#include "utils/Logger.h"
+#include "utils/Units.h"
+#include "utils/Variant.h"
+
+#include <QFile>
+
+using namespace CalamaresUtils::Units;
+
+static bool
+copy_file( const QString& source, const QString& dest )
+{
+    QFile sourcef( source );
+    if ( !sourcef.open( QFile::ReadOnly ) )
+    {
+        cWarning() << "Could not read" << source;
+        return false;
+    }
+
+    QFile destf( dest );
+    if ( !destf.open( QFile::WriteOnly ) )
+    {
+        sourcef.close();
+        cWarning() << "Could not open" << destf.fileName() << "for writing; could not copy" << source;
+        return false;
+    }
+
+    QByteArray b;
+    do
+    {
+        b = sourcef.read( 1_MiB );
+        destf.write( b );
+    } while ( b.count() > 0 );
+
+    sourcef.close();
+    destf.close();
+
+    return true;
+}
+
+Item
+Item::fromVariant( const QVariant& v, const CalamaresUtils::Permissions& defaultPermissions )
+{
+    if ( v.type() == QVariant::String )
+    {
+        QString filename = v.toString();
+        if ( !filename.isEmpty() )
+        {
+            return { filename, filename, defaultPermissions, ItemType::Path, false };
+        }
+        else
+        {
+            cWarning() << "Empty filename for preservefiles, item" << v;
+            return {};
+        }
+    }
+    else if ( v.type() == QVariant::Map )
+    {
+        const auto map = v.toMap();
+
+        CalamaresUtils::Permissions perm( defaultPermissions );
+        ItemType t = ItemType::None;
+        bool optional = CalamaresUtils::getBool( map, "optional", false );
+
+        {
+            QString perm_string = map[ "perm" ].toString();
+            if ( !perm_string.isEmpty() )
+            {
+                perm = CalamaresUtils::Permissions( perm_string );
+            }
+        }
+
+        {
+            QString from = map[ "from" ].toString();
+            t = ( from == "log" ) ? ItemType::Log : ( from == "config" ) ? ItemType::Config : ItemType::None;
+
+            if ( t == ItemType::None && !map[ "src" ].toString().isEmpty() )
+            {
+                t = ItemType::Path;
+            }
+        }
+
+        QString dest = map[ "dest" ].toString();
+        if ( dest.isEmpty() )
+        {
+            cWarning() << "Empty dest for preservefiles, item" << v;
+            return {};
+        }
+
+        switch ( t )
+        {
+        case ItemType::Config:
+            return { QString(), dest, perm, t, optional };
+        case ItemType::Log:
+            return { QString(), dest, perm, t, optional };
+        case ItemType::Path:
+            return { map[ "src" ].toString(), dest, perm, t, optional };
+        case ItemType::None:
+            cWarning() << "Invalid type for preservefiles, item" << v;
+            return {};
+        }
+    }
+    cWarning() << "Invalid type for preservefiles, item" << v;
+    return {};
+}
+
+
+bool
+Item::exec( const std::function< QString( QString ) >& replacements ) const
+{
+    QString expanded_dest = replacements( dest );
+    QString full_dest = CalamaresUtils::System::instance()->targetPath( expanded_dest );
+
+    bool success = false;
+    switch ( m_type )
+    {
+    case ItemType::None:
+        cWarning() << "Invalid item for preservefiles skipped.";
+        return false;
+    case ItemType::Config:
+        if ( !( success = Calamares::JobQueue::instance()->globalStorage()->saveJson( full_dest ) ) )
+        {
+            cWarning() << "Could not write a JSON dump of global storage to" << full_dest;
+        }
+        break;
+    case ItemType::Log:
+        if ( !( success = copy_file( Logger::logFile(), full_dest ) ) )
+        {
+            cWarning() << "Could not preserve log file to" << full_dest;
+        }
+        break;
+    case ItemType::Path:
+        if ( !( success = copy_file( source, full_dest ) ) )
+        {
+            cWarning() << "Could not preserve" << source << "to" << full_dest;
+        }
+        break;
+    }
+    if ( !success )
+    {
+        CalamaresUtils::System::instance()->removeTargetFile( expanded_dest );
+        return false;
+    }
+    else
+    {
+        return perm.apply( full_dest );
+    }
+}

src/modules/preservefiles/Item.h

@@ -0,0 +1,76 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *  SPDX-FileCopyrightText: 2018, 2021 Adriaan de Groot <groot@kde.org>
+ *  SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ */
+#ifndef PRESERVEFILES_ITEM_H
+#define PRESERVEFILES_ITEM_H
+
+#include "utils/Permissions.h"
+
+#include <QString>
+#include <QVariant>
+
+#include <memory>
+
+enum class ItemType
+{
+    None,
+    Path,
+    Log,
+    Config
+};
+
+/** @brief Represents one item to copy
+ *
+ * All item types need a destination (to place the data), this is
+ * intepreted within the target system. All items need a permission,
+ * which is applied to the data once written.
+ *
+ * The source may be a path, but not all types need a source.
+ */
+class Item
+{
+    QString source;
+    QString dest;
+    CalamaresUtils::Permissions perm;
+    ItemType m_type = ItemType::None;
+    bool m_optional = false;
+
+public:
+    Item( const QString& src, const QString& d, CalamaresUtils::Permissions p, ItemType t, bool optional )
+        : source( src )
+        , dest( d )
+        , perm( std::move( p ) )
+        , m_type( t )
+        , m_optional( optional )
+    {
+    }
+
+    Item()
+        : m_type( ItemType::None )
+    {
+    }
+
+    operator bool() const { return m_type != ItemType::None; }
+    ItemType type() const { return m_type; }
+    bool isOptional() const { return m_optional; }
+
+    bool exec( const std::function< QString( QString ) >& replacements ) const;
+
+
+    /** @brief Create an Item -- or one of its subclasses -- from @p v
+     *
+     * Depending on the structure and contents of @p v, a pointer
+     * to an Item is returned. If @p v cannot be interpreted meaningfully,
+     * then a nullptr is returned.
+     *
+     * When the entry contains a *perm* key, use that permission, otherwise
+     * apply @p defaultPermissions to the item.
+     */
+    static Item fromVariant( const QVariant& v, const CalamaresUtils::Permissions& defaultPermissions );
+};
+
+
+#endif

src/modules/preservefiles/PreserveFiles.cpp

@@ -7,46 +7,20 @@
 
 #include "PreserveFiles.h"
 
+#include "Item.h"
+
 #include "CalamaresVersion.h"
 #include "GlobalStorage.h"
 #include "JobQueue.h"
 #include "utils/CalamaresUtilsSystem.h"
 #include "utils/CommandList.h"
 #include "utils/Logger.h"
-#include "utils/Permissions.h"
 #include "utils/Units.h"
 
 #include <QFile>
 
 using namespace CalamaresUtils::Units;
 
-QString
-targetPrefix()
-{
-    if ( CalamaresUtils::System::instance()->doChroot() )
-    {
-        Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
-        if ( gs && gs->contains( "rootMountPoint" ) )
-        {
-            QString r = gs->value( "rootMountPoint" ).toString();
-            if ( !r.isEmpty() )
-            {
-                return r;
-            }
-            else
-            {
-                cDebug() << "RootMountPoint is empty";
-            }
-        }
-        else
-        {
-            cDebug() << "No rootMountPoint defined, preserving files to '/'";
-        }
-    }
-
-    return QLatin1String( "/" );
-}
-
 QString
 atReplacements( QString s )
 {
@@ -79,95 +53,34 @@ PreserveFiles::prettyName() const
     return tr( "Saving files for later ..." );
 }
 
-static bool
-copy_file( const QString& source, const QString& dest )
-{
-    QFile sourcef( source );
-    if ( !sourcef.open( QFile::ReadOnly ) )
-    {
-        cWarning() << "Could not read" << source;
-        return false;
-    }
-
-    QFile destf( dest );
-    if ( !destf.open( QFile::WriteOnly ) )
-    {
-        sourcef.close();
-        cWarning() << "Could not open" << destf.fileName() << "for writing; could not copy" << source;
-        return false;
-    }
-
-    QByteArray b;
-    do
-    {
-        b = sourcef.read( 1_MiB );
-        destf.write( b );
-    } while ( b.count() > 0 );
-
-    sourcef.close();
-    destf.close();
-
-    return true;
-}
-
 Calamares::JobResult
 PreserveFiles::exec()
 {
-    if ( m_items.isEmpty() )
+    if ( m_items.empty() )
     {
         return Calamares::JobResult::error( tr( "No files configured to save for later." ) );
     }
 
-    QString prefix = targetPrefix();
-    if ( !prefix.endsWith( '/' ) )
-    {
-        prefix.append( '/' );
-    }
-
     int count = 0;
-    for ( const auto& it : m_items )
+    for ( const auto& it : qAsConst( m_items ) )
     {
-        QString source = it.source;
-        QString bare_dest = atReplacements( it.dest );
-        QString dest = prefix + bare_dest;
-
-        if ( it.type == ItemType::Log )
+        if ( !it )
         {
-            source = Logger::logFile();
+            // Invalid entries are nullptr, ignore them but count as a success
+            // because they shouldn't block the installation. There are
+            // warnings in the log showing what the configuration problem is.
+            ++count;
+            continue;
         }
-        if ( it.type == ItemType::Config )
+        // Try to preserve the file. If it's marked as optional, count it
+        // as a success regardless.
+        if ( it.exec( atReplacements ) || it.isOptional() )
         {
-            if ( !Calamares::JobQueue::instance()->globalStorage()->saveJson( dest ) )
-            {
-                cWarning() << "Could not write a JSON dump of global storage to" << dest;
-            }
-            else
-            {
-                ++count;
-            }
-        }
-        else if ( source.isEmpty() )
-        {
-            cWarning() << "Skipping unnamed source file for" << dest;
-        }
-        else
-        {
-            if ( copy_file( source, dest ) )
-            {
-                if ( it.perm.isValid() )
-                {
-                    if ( !it.perm.apply( CalamaresUtils::System::instance()->targetPath( bare_dest ) ) )
-                    {
-                        cWarning() << "Could not set attributes of" << bare_dest;
-                    }
-                }
-
-                ++count;
-            }
+            ++count;
         }
     }
 
-    return count == m_items.count()
+    return count == m_items.size()
         ? Calamares::JobResult::ok()
         : Calamares::JobResult::error( tr( "Not all of the configured files could be preserved." ) );
 }
@@ -193,53 +106,11 @@ PreserveFiles::setConfigurationMap( const QVariantMap& configurationMap )
     {
         defaultPermissions = QStringLiteral( "root:root:0400" );
     }
+    CalamaresUtils::Permissions perm( defaultPermissions );
 
-    QVariantList l = files.toList();
-    unsigned int c = 0;
-    for ( const auto& li : l )
+    for ( const auto& li : files.toList() )
     {
-        if ( li.type() == QVariant::String )
-        {
-            QString filename = li.toString();
-            if ( !filename.isEmpty() )
-                m_items.append(
-                    Item { filename, filename, CalamaresUtils::Permissions( defaultPermissions ), ItemType::Path } );
-            else
-            {
-                cDebug() << "Empty filename for preservefiles, item" << c;
-            }
-        }
-        else if ( li.type() == QVariant::Map )
-        {
-            const auto map = li.toMap();
-            QString dest = map[ "dest" ].toString();
-            QString from = map[ "from" ].toString();
-            ItemType t = ( from == "log" ) ? ItemType::Log : ( from == "config" ) ? ItemType::Config : ItemType::None;
-            QString perm = map[ "perm" ].toString();
-            if ( perm.isEmpty() )
-            {
-                perm = defaultPermissions;
-            }
-
-            if ( dest.isEmpty() )
-            {
-                cDebug() << "Empty dest for preservefiles, item" << c;
-            }
-            else if ( t == ItemType::None )
-            {
-                cDebug() << "Invalid type for preservefiles, item" << c;
-            }
-            else
-            {
-                m_items.append( Item { QString(), dest, CalamaresUtils::Permissions( perm ), t } );
-            }
-        }
-        else
-        {
-            cDebug() << "Invalid type for preservefiles, item" << c;
-        }
-
-        ++c;
+        m_items.push_back( Item::fromVariant( li, perm ) );
     }
 }
 

src/modules/preservefiles/PreserveFiles.h

@@ -10,33 +10,14 @@
 
 #include "CppJob.h"
 #include "DllMacro.h"
-#include "utils/Permissions.h"
 #include "utils/PluginFactory.h"
 
-#include <QList>
-#include <QObject>
-#include <QVariantMap>
+class Item;
 
 class PLUGINDLLEXPORT PreserveFiles : public Calamares::CppJob
 {
     Q_OBJECT
 
-    enum class ItemType
-    {
-        None,
-        Path,
-        Log,
-        Config
-    };
-
-    struct Item
-    {
-        QString source;
-        QString dest;
-        CalamaresUtils::Permissions perm;
-        ItemType type;
-    };
-
     using ItemList = QList< Item >;
 
 public:

src/modules/preservefiles/Tests.cpp

@@ -0,0 +1,93 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *   SPDX-FileCopyrightText: 2021 Adriaan de Groot <groot@kde.org>
+ *   SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ *   Calamares is Free Software: see the License-Identifier above.
+ *
+ */
+
+#include "Item.h"
+
+#include "Settings.h"
+#include "utils/CalamaresUtilsSystem.h"
+#include "utils/Logger.h"
+#include "utils/NamedEnum.h"
+#include "utils/Yaml.h"
+
+#include <QtTest/QtTest>
+
+class PreserveFilesTests : public QObject
+{
+    Q_OBJECT
+public:
+    PreserveFilesTests();
+    ~PreserveFilesTests() override {}
+
+private Q_SLOTS:
+    void initTestCase();
+
+    void testItems_data();
+    void testItems();
+};
+
+PreserveFilesTests::PreserveFilesTests() {}
+
+void
+PreserveFilesTests::initTestCase()
+{
+    Logger::setupLogLevel( Logger::LOGDEBUG );
+    cDebug() << "PreserveFiles test started.";
+
+    // Ensure we have a system object, expect it to be a "bogus" one
+    CalamaresUtils::System* system = CalamaresUtils::System::instance();
+    QVERIFY( system );
+    cDebug() << Logger::SubEntry << "System @" << Logger::Pointer( system );
+
+    const auto* settings = Calamares::Settings::instance();
+    if ( !settings )
+    {
+        (void)new Calamares::Settings( true );
+    }
+}
+
+void
+PreserveFilesTests::testItems_data()
+{
+    QTest::addColumn< QString >( "filename" );
+    QTest::addColumn< bool >( "ok" );
+    QTest::addColumn< int >( "type_i" );
+
+    QTest::newRow( "log     " ) << QString( "1a-log.conf" ) << true << smash( ItemType::Log );
+    QTest::newRow( "config  " ) << QString( "1b-config.conf" ) << true << smash( ItemType::Config );
+    QTest::newRow( "src     " ) << QString( "1c-src.conf" ) << true << smash( ItemType::Path );
+    QTest::newRow( "filename" ) << QString( "1d-filename.conf" ) << true << smash( ItemType::Path );
+    QTest::newRow( "empty   " ) << QString( "1e-empty.conf" ) << false << smash( ItemType::None );
+    QTest::newRow( "bad     " ) << QString( "1f-bad.conf" ) << false << smash( ItemType::None );
+}
+
+void
+PreserveFilesTests::testItems()
+{
+    QFETCH( QString, filename );
+    QFETCH( bool, ok );
+    QFETCH( int, type_i );
+
+    QFile fi( QString( "%1/tests/%2" ).arg( BUILD_AS_TEST, filename ) );
+    QVERIFY( fi.exists() );
+
+    bool config_file_ok = false;
+    const auto map = CalamaresUtils::loadYaml( fi, &config_file_ok );
+    QVERIFY( config_file_ok );
+
+    CalamaresUtils::Permissions perm( QStringLiteral( "adridg:adridg:0750" ) );
+    auto i = Item::fromVariant( map[ "item" ], perm );
+    QCOMPARE( bool( i ), ok );
+    QCOMPARE( smash( i.type() ), type_i );
+}
+
+QTEST_GUILESS_MAIN( PreserveFilesTests )
+
+#include "utils/moc-warnings.h"
+
+#include "Tests.moc"

src/modules/preservefiles/preservefiles.conf

@@ -7,42 +7,58 @@
 # the list should have one of these forms:
 #
 # - an absolute path (probably within the host system). This will be preserved
-#   as the same path within the target system (chroot). If, globally, dontChroot
-#   is true, then these items are ignored (since the destination is the same
-#   as the source).
+#   as the same path within the target system (chroot). If, globally,
+#   *dontChroot* is true, then these items will be ignored (since the
+#   destination is the same as the source).
 # - a map with a *dest* key. The *dest* value is a path interpreted in the
-#   target system (if dontChroot is true, in the host system). Relative paths
-#   are not recommended. There are three possible other keys in the map:
+#   target system (if the global *dontChroot* is true, then the host is the
+#   target as well). Relative paths are not recommended. There are two
+#   ways to select the source data for the file:
 #   - *from*, which must have one of the values, below; it is used to
 #     preserve files whose pathname is known to Calamares internally.
 #   - *src*, to refer to a path interpreted in the host system. Relative
 #     paths are not recommended, and are interpreted relative to where
 #     Calamares is being run.
+#   Exactly one of the two source keys (either *from* or *src*) must be set.
+#
+#   Special values for the key *from* are:
+#     - *log*, for the complete log file (up to the moment the preservefiles
+#       module is run),
+#     - *config*, for a JSON dump of the contents of global storage.
+#       Note that this may contain sensitive information, and should be
+#       given restrictive permissions.
+#
+#  A map with a *dest* key can have these additional fields:
 #   - *perm*, is a colon-separated tuple of <user>:<group>:<mode>
 #     where <mode> is in octal (e.g. 4777 for wide-open, 0400 for read-only
 #     by owner). If set, the file's ownership and permissions are set to
 #     those values within the target system; if not set, no permissions
 #     are changed.
-#   Only one of the two source keys (either *from* or *src*) may be set.
+#   - *optional*, is a boolean; if this is set to `true` then failure to
+#     preserve the file will **not** be counted as a failure of the
+#     module, and installation will proceed. Set this for files that might
+#     not exist in the host system (e.g. nvidia configuration files that
+#     are created in some boot scenarios and not in others).
 #
-# The target filename is modified as follows:
-#   - `@@ROOT@@` is replaced by the path to the target root (may be /)
+# The target path (*dest*) is modified as follows:
+#   - `@@ROOT@@` is replaced by the path to the target root (may be /).
+#     There is never any reason to use this, since the *dest* is already
+#     interpreted in the target system.
 #   - `@@USER@@` is replaced by the username entered by on the user
 #       page (may be empty, for instance if no user page is enabled)
 #
-# Special values for the key *from* are:
-#   - *log*, for the complete log file (up to the moment the preservefiles
-#     module is run),
-#   - *config*, for a JSON dump of the contents of global storage
----
+#
+#
 files:
-  - /etc/oem-information
   - from: log
-    dest: /root/install.log
-    perm: root:wheel:644
+    dest: /var/log/Calamares.log
+    perm: root:wheel:600
   - from: config
-    dest: /root/install.json
-    perm: root:wheel:400
+    dest: /var/log/Calamares-install.json
+    perm: root:wheel:600
+# - src: /var/log/nvidia.conf
+#   dest: /var/log/Calamares-nvidia.conf
+#   optional: true
 
 # The *perm* key contains a default value to apply to all files listed
 # above that do not have a *perm* key of their own. If not set,

src/modules/preservefiles/preservefiles.schema.yaml

@@ -0,0 +1,37 @@
+# SPDX-FileCopyrightText: 2020 Adriaan de Groot <groot@kde.org>
+# SPDX-License-Identifier: GPL-3.0-or-later
+---
+$schema: https://json-schema.org/schema#
+$id: https://calamares.io/schemas/preservefiles
+additionalProperties: false
+type: object
+properties:
+    # TODO: it's a particularly-formatted string
+    perm: { type: string }
+    files:
+        type: array
+        items:
+            # There are three entries here because: string, or an entry with
+            # a src (but no from) or an entry with from (but no src).
+            anyOf:
+              - type: string
+              - type: object
+                properties:
+                    dest: { type: string }
+                    src: { type: string }
+                    # TODO: it's a particularly-formatted string
+                    perm: { type: string }
+                    optional: { type: boolean }
+                required: [ dest ]
+                additionalProperties: false
+              - type: object
+                properties:
+                    dest: { type: string }
+                    from: { type: string, enum: [config, log] }
+                    # TODO: it's a particularly-formatted string
+                    perm: { type: string }
+                    optional: { type: boolean }
+                required: [ dest ]
+                additionalProperties: false
+
+required: [ files ]

src/modules/preservefiles/tests/1a-log.conf

@@ -0,0 +1,7 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+item:
+    from: log
+    dest: /var/log/Calamares.log
+    perm: root:wheel:601

src/modules/preservefiles/tests/1b-config.conf

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item:
+    from: config
+    dest: /var/log/Calamares-install.json
+    perm: root:wheel:600

src/modules/preservefiles/tests/1c-src.conf

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item:
+    src: /root/.cache/calamares/session.log
+    dest: /var/log/Calamares.log
+    perm: root:wheel:600

src/modules/preservefiles/tests/1d-filename.conf

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item:
+    src: /root/.cache/calamares/session.log
+    dest: /var/log/Calamares.log
+    perm: root:wheel:600

src/modules/preservefiles/tests/1e-empty.conf

@@ -0,0 +1,3 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item: []

src/modules/preservefiles/tests/1f-bad.conf

@@ -0,0 +1,4 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item:
+    bop: 1

src/modules/umount/main.py

@@ -80,6 +80,7 @@ def run():
     if(libcalamares.job.configuration and
        "srcLog" in libcalamares.job.configuration and
        "destLog" in libcalamares.job.configuration):
+        libcalamares.utils.warning("Log-file preserving is **deprecated** in the *umount* module")
         log_source = libcalamares.job.configuration["srcLog"]
         log_destination = libcalamares.job.configuration["destLog"]
         # Relocate log_destination into target system

src/modules/umount/umount.conf

@@ -10,16 +10,21 @@
 # The "copy log files" functionality is deprecated; use the *preservefiles*
 # module instead, which is more flexible.
 #
-# This module has two configuration keys:
-#   srcLog  location in the live system where the log is
-#   destLog location in the target system to copy the log
 #
 
 ---
-# example when using the normal Calamares log:
-srcLog:      "/root/.cache/calamares/session.log"
-destLog:     "/var/log/Calamares.log"
-
+# This is a **deprecated** example. Use the *preservefiles* module
+# instead, where the equivalent configuration is this:
+#
+# files:
+#   - from: log
+#     dest: /var/log/installation.log
+#
+# Note that the "equivalent configuration" always finds the log,
+# and is not dependent on specific user names or the vagaries of
+# polkit configuration -- so it is a **better** "equivalent".
+#
 # example when using a log created by `sudo calamares -d`:
 #srcLog:      "/home/live/installation.log"
 #destLog:     "/var/log/installation.log"
+srcLog: "/bogus/just/do/not/use/this/anymore.txt"