Merge branch 'issue-1164'

FIXES #1164

src/libcalamares/JobQueue.cpp

@@ -44,7 +44,7 @@ public:
     }
 
     virtual ~JobThread() override;
-    
+
     void setJobs( const JobList& jobs )
     {
         m_jobs = jobs;
@@ -157,6 +157,14 @@ JobQueue::JobQueue( QObject* parent )
 
 JobQueue::~JobQueue()
 {
+    if ( m_thread->isRunning() )
+    {
+        m_thread->terminate();
+        if ( !m_thread->wait(300) )
+            cError() << "Could not terminate job thread (expect a crash now).";
+        delete m_thread;
+    }
+
     delete m_storage;
 }
 

src/libcalamares/ProcessJob.cpp

@@ -48,10 +48,8 @@ ProcessJob::~ProcessJob()
 QString
 ProcessJob::prettyName() const
 {
-    //TODO: show something more meaningful
+    return ( m_runInChroot ? tr( "Run command '%1' in target system." ) : tr( " Run command '%1'." ) )
-    return tr( "Run command %1 %2" )
+            .arg( m_command );
-            .arg( m_command )
-            .arg( m_runInChroot ? "in chroot." : " ." );
 }
 
 
@@ -67,83 +65,23 @@ ProcessJob::prettyStatusMessage() const
 JobResult
 ProcessJob::exec()
 {
-    int ec = 0;
+    using CalamaresUtils::System;
-    QString output;
+
     if ( m_runInChroot )
-        ec = CalamaresUtils::System::instance()->
+        return CalamaresUtils::System::instance()->
-             targetEnvOutput( m_command,
+             targetEnvCommand( { m_command },
-                              output,
                               m_workingPath,
                               QString(),
-                              m_timeoutSec );
+                              m_timeoutSec )
+             .explainProcess( m_command, m_timeoutSec );
     else
-        ec = callOutput( m_command,
+        return
-                         output,
+            System::runCommand( System::RunLocation::RunInHost,
-                         m_workingPath,
+                                { "/bin/sh", "-c", m_command },
-                         QString(),
+                                m_workingPath,
-                         m_timeoutSec );
+                                QString(),
-
+                                m_timeoutSec )
-    return CalamaresUtils::ProcessResult::explainProcess( ec, m_command, output, m_timeoutSec );
+            .explainProcess( m_command, m_timeoutSec );
-}
-
-
-int
-ProcessJob::callOutput( const QString& command,
-                        QString& output,
-                        const QString& workingPath,
-                        const QString& stdInput,
-                        int timeoutSec )
-{
-    output.clear();
-
-    QProcess process;
-    process.setProgram( "/bin/sh" );
-    process.setArguments( { "-c", command } );
-    process.setProcessChannelMode( QProcess::MergedChannels );
-
-    if ( !workingPath.isEmpty() )
-    {
-        if ( QDir( workingPath ).exists() )
-            process.setWorkingDirectory( QDir( workingPath ).absolutePath() );
-        else
-        {
-            cWarning() << "Invalid working directory:" << workingPath;
-            return -3;
-        }
-    }
-
-    cDebug() << "Running" << command;
-    process.start();
-    if ( !process.waitForStarted() )
-    {
-        cWarning() << "Process failed to start" << process.error();
-        return -2;
-    }
-
-    if ( !stdInput.isEmpty() )
-    {
-        process.write( stdInput.toLocal8Bit() );
-        process.closeWriteChannel();
-    }
-
-    if ( !process.waitForFinished( timeoutSec ? ( timeoutSec * 1000 ) : -1 ) )
-    {
-        cWarning() << "Timed out. output so far:";
-        output.append( QString::fromLocal8Bit( process.readAllStandardOutput() ).trimmed() );
-        cWarning() << output;
-        return -4;
-    }
-
-    output.append( QString::fromLocal8Bit( process.readAllStandardOutput() ).trimmed() );
-
-    if ( process.exitStatus() == QProcess::CrashExit )
-    {
-        cWarning() << "Process crashed";
-        return -1;
-    }
-
-    cDebug() << "Finished. Exit code:" << process.exitCode();
-    return process.exitCode();
 }
 
 } // namespace Calamares

src/libcalamares/ProcessJob.h

@@ -1,7 +1,7 @@
 /* === This file is part of Calamares - <https://github.com/calamares> ===
  *
  *   Copyright 2014, Teo Mrnjavac <teo@kde.org>
- *   Copyright 2017-2018, Adriaan de Groot <groot@kde.org>
+ *   Copyright 2017-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
@@ -40,11 +40,6 @@ public:
     JobResult exec() override;
 
 private:
-    int callOutput( const QString& command,
-                    QString& output,
-                    const QString& workingPath = QString(),
-                    const QString& stdInput = QString(),
-                    int timeoutSec = 0 );
     QString m_command;
     QString m_workingPath;
     bool m_runInChroot;

src/libcalamares/Tests.cpp

@@ -18,9 +18,12 @@
 
 #include "Tests.h"
 
+#include "utils/CalamaresUtilsSystem.h"
 #include "utils/Logger.h"
 #include "utils/Yaml.h"
 
+#include <QTemporaryFile>
+
 #include <QtTest/QtTest>
 
 QTEST_GUILESS_MAIN( LibCalamaresTests )
@@ -113,3 +116,45 @@ LibCalamaresTests::testLoadSaveYamlExtended()
     }
     QFile::remove( "out.yaml" );
 }
+
+void
+LibCalamaresTests::testCommands()
+{
+    using CalamaresUtils::System;
+    auto r = System::runCommand(
+        System::RunLocation::RunInHost,
+        { "/bin/ls", "/tmp" }
+        );
+
+    QVERIFY( r.getExitCode() == 0 );
+
+    QTemporaryFile tf( "/tmp/calamares-test-XXXXXX" );
+    QVERIFY( tf.open() );
+    QVERIFY( !tf.fileName().isEmpty() );
+
+    QFileInfo tfn( tf.fileName() );
+    QVERIFY( !r.getOutput().contains( tfn.fileName() ) );
+
+    // Run ls again, now that the file exists
+    r = System::runCommand(
+        System::RunLocation::RunInHost,
+        { "/bin/ls", "/tmp" }
+        );
+    QVERIFY( r.getOutput().contains( tfn.fileName() ) );
+
+    // .. and without a working directory set, assume builddir != /tmp
+    r = System::runCommand(
+        System::RunLocation::RunInHost,
+        { "/bin/ls" }
+        );
+    QVERIFY( !r.getOutput().contains( tfn.fileName() ) );
+
+    r = System::runCommand(
+        System::RunLocation::RunInHost,
+        { "/bin/ls" },
+        "/tmp"
+        );
+    QVERIFY( r.getOutput().contains( tfn.fileName() ) );
+
+
+}

src/libcalamares/Tests.h

@@ -34,6 +34,8 @@ private Q_SLOTS:
 
     void testLoadSaveYaml();  // Just settings.conf
     void testLoadSaveYamlExtended();  // Do a find() in the src dir
+
+    void testCommands();
 };
 
 #endif

src/libcalamares/utils/CalamaresUtilsSystem.cpp

@@ -114,14 +114,24 @@ System::mount( const QString& devicePath,
        const QString& options )
 {
     if ( devicePath.isEmpty() || mountPoint.isEmpty() )
-        return -3;
+    {
+        if ( devicePath.isEmpty() )
+            cWarning() << "Can't mount an empty device.";
+        if ( mountPoint.isEmpty() )
+            cWarning() << "Can't mount on an empty mountpoint.";
+
+        return static_cast<int>(ProcessResult::Code::NoWorkingDirectory);
+    }
 
     QDir mountPointDir( mountPoint );
     if ( !mountPointDir.exists() )
     {
         bool ok = mountPointDir.mkpath( mountPoint );
         if ( !ok )
-            return -3;
+        {
+            cWarning() << "Could not create mountpoint" << mountPoint;
+            return static_cast<int>(ProcessResult::Code::NoWorkingDirectory);
+        }
     }
 
     QString program( "mount" );
@@ -146,15 +156,13 @@ System::runCommand(
 {
     QString output;
 
-    if ( !Calamares::JobQueue::instance() )
+    Calamares::GlobalStorage* gs = Calamares::JobQueue::instance() ? Calamares::JobQueue::instance()->globalStorage() : nullptr;
-        return -3;
 
-    Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
     if ( ( location == System::RunLocation::RunInTarget ) &&
          ( !gs || !gs->contains( "rootMountPoint" ) ) )
     {
         cWarning() << "No rootMountPoint in global storage";
-        return -3;
+        return ProcessResult::Code::NoWorkingDirectory;
     }
 
     QProcess process;
@@ -167,7 +175,7 @@ System::runCommand(
         if ( !QDir( destDir ).exists() )
         {
             cWarning() << "rootMountPoint points to a dir which does not exist";
-            return -3;
+            return ProcessResult::Code::NoWorkingDirectory;
         }
 
         program = "chroot";
@@ -189,8 +197,10 @@ System::runCommand(
         if ( QDir( workingPath ).exists() )
             process.setWorkingDirectory( QDir( workingPath ).absolutePath() );
         else
+        {
             cWarning() << "Invalid working directory:" << workingPath;
-            return -3;
+            return ProcessResult::Code::NoWorkingDirectory;
+        }
     }
 
     cDebug() << "Running" << program << RedactedList( arguments );
@@ -198,20 +208,20 @@ System::runCommand(
     if ( !process.waitForStarted() )
     {
         cWarning() << "Process failed to start" << process.error();
-        return -2;
+        return ProcessResult::Code::FailedToStart;
     }
 
     if ( !stdInput.isEmpty() )
     {
         process.write( stdInput.toLocal8Bit() );
-        process.closeWriteChannel();
     }
+    process.closeWriteChannel();
 
     if ( !process.waitForFinished( timeoutSec ? ( timeoutSec * 1000 ) : -1 ) )
     {
         cWarning().noquote().nospace() << "Timed out. Output so far:\n" <<
             process.readAllStandardOutput();
-        return -4;
+        return ProcessResult::Code::TimedOut;
     }
 
     output.append( QString::fromLocal8Bit( process.readAllStandardOutput() ).trimmed() );
@@ -219,12 +229,13 @@ System::runCommand(
     if ( process.exitStatus() == QProcess::CrashExit )
     {
         cWarning().noquote().nospace() << "Process crashed. Output so far:\n" << output;
-        return -1;
+        return ProcessResult::Code::Crashed;
     }
 
     auto r = process.exitCode();
     cDebug() << "Finished. Exit code:" << r;
-    if ( ( r != 0 ) || Calamares::Settings::instance()->debugMode() )
+    bool showDebug = ( !Calamares::Settings::instance() ) || ( Calamares::Settings::instance()->debugMode() );
+    if ( ( r != 0 ) || showDebug )
     {
         cDebug() << "Target cmd:" << RedactedList( args );
         cDebug().noquote().nospace() << "Target output:\n" << output;
@@ -306,22 +317,22 @@ ProcessResult::explainProcess( int ec, const QString& command, const QString& ou
         ? QCoreApplication::translate( "ProcessResult", "\nThere was no output from the command.")
         : (QCoreApplication::translate( "ProcessResult", "\nOutput:\n") + output);
 
-    if ( ec == -1 ) //Crash!
+    if ( ec == static_cast<int>(ProcessResult::Code::Crashed) ) //Crash!
         return JobResult::error( QCoreApplication::translate( "ProcessResult", "External command crashed." ),
                                  QCoreApplication::translate( "ProcessResult", "Command <i>%1</i> crashed." )
                                         .arg( command )
                                         + outputMessage );
 
-    if ( ec == -2 )
+    if ( ec == static_cast<int>(ProcessResult::Code::FailedToStart) )
         return JobResult::error( QCoreApplication::translate( "ProcessResult", "External command failed to start." ),
                                  QCoreApplication::translate( "ProcessResult", "Command <i>%1</i> failed to start." )
                                     .arg( command ) );
 
-    if ( ec == -3 )
+    if ( ec == static_cast<int>(ProcessResult::Code::NoWorkingDirectory) )
         return JobResult::error( QCoreApplication::translate( "ProcessResult", "Internal error when starting command." ),
                                  QCoreApplication::translate( "ProcessResult", "Bad parameters for process job call." ) );
 
-    if ( ec == -4 )
+    if ( ec == static_cast<int>(ProcessResult::Code::TimedOut) )
         return JobResult::error( QCoreApplication::translate( "ProcessResult", "External command failed to finish." ),
                                  QCoreApplication::translate( "ProcessResult", "Command <i>%1</i> failed to finish in %2 seconds." )
                                     .arg( command )

src/libcalamares/utils/CalamaresUtilsSystem.h

@@ -32,8 +32,16 @@ namespace CalamaresUtils
 class ProcessResult : public QPair< int, QString >
 {
 public:
+    enum class Code : int
+    {
+        Crashed = -1,        // Must match special return values from QProcess
+        FailedToStart = -2,  // Must match special return values from QProcess
+        NoWorkingDirectory = -3,
+        TimedOut = -4
+    } ;
+
     /** @brief Implicit one-argument constructor has no output, only a return code */
-    ProcessResult( int r ) : QPair< int, QString >( r, QString() ) {}
+    ProcessResult( Code r ) : QPair< int, QString >( static_cast<int>(r), QString() ) {}
     ProcessResult( int r, QString s ) : QPair< int, QString >( r, s ) {}
 
     int getExitCode() const { return first; }
@@ -93,9 +101,9 @@ public:
       * @param filesystemName the name of the filesystem (optional).
       * @param options any additional options as passed to mount -o (optional).
       * @returns the program's exit code, or:
-      *             -1 = QProcess crash
+      *             Crashed = QProcess crash
-      *             -2 = QProcess cannot start
+      *             FailedToStart = QProcess cannot start
-      *             -3 = bad arguments
+      *             NoWorkingDirectory = bad arguments
       */
     DLLEXPORT int mount( const QString& devicePath,
                          const QString& mountPoint,
@@ -120,10 +128,10 @@ public:
       *
       * @returns the program's exit code and its output (if any). Special
       *     exit codes (which will never have any output) are:
-      *             -1 = QProcess crash
+      *             Crashed = QProcess crash
-      *             -2 = QProcess cannot start
+      *             FailedToStart = QProcess cannot start
-      *             -3 = bad arguments
+      *             NoWorkingDirectory = bad arguments
-      *             -4 = QProcess timeout
+      *             TimedOut = QProcess timeout
       */
     static DLLEXPORT ProcessResult runCommand(
         RunLocation location,