Sanitize the Executor code flow.

The current behavior is not exactly well-defined, mostly
due to signals calling back on the Executor while it is
calling a function on the object emitting the signal (e.g.
waitForFinished()). These traps are now gone.
Also make sure error messages are in a sensible order
when a build is canceled.
In addition, remove unused functionality.

Change-Id: Icf35eb378995a55b5ba4fc6caea9492eccb43527
Reviewed-by: Joerg Bornemann <joerg.bornemann@digia.com>

7 files changed, 87 insertions, 116 deletions

diff --git a/src/lib/api/internaljobs.cpp b/src/lib/api/internaljobs.cpp
index 6fb182f00..d74c516ab 100644
--- a/src/lib/api/internaljobs.cpp
+++ b/src/lib/api/internaljobs.cpp
@@ -225,25 +225,19 @@ void InternalBuildJob::build(const QList<BuildProduct::Ptr> &products,
 
 void InternalBuildJob::start()
 {
-    Executor * const executor = new Executor(this);
-    connect(executor, SIGNAL(finished()), SLOT(handleFinished()));
-    connect(executor, SIGNAL(error(Error)), SLOT(handleError(Error)));
-    executor->setEngine(new ScriptEngine(this));
-    executor->setBuildOptions(buildOptions());
-    executor->setProgressObserver(observer());
-    executor->build(products());
+    m_executor = new Executor(this);
+    connect(m_executor, SIGNAL(finished()), SLOT(handleFinished()));
+    m_executor->setEngine(new ScriptEngine(this));
+    m_executor->setBuildOptions(buildOptions());
+    m_executor->setProgressObserver(observer());
+    m_executor->build(products());
 }
 
 void InternalBuildJob::handleFinished()
 {
     storeBuildGraph();
-    if (!hasError()) // Already emitted finished() in that case.
-        emit finished(this);
-}
-
-void InternalBuildJob::handleError(const Error &error)
-{
-    setError(error);
+    if (m_executor->hasError())
+        setError(m_executor->error());
     emit finished(this);
 }
 
diff --git a/src/lib/api/internaljobs.h b/src/lib/api/internaljobs.h
index 8eae7682d..ddb3dabe8 100644
--- a/src/lib/api/internaljobs.h
+++ b/src/lib/api/internaljobs.h
@@ -41,6 +41,7 @@
 namespace qbs {
 
 namespace Internal {
+class Executor;
 class JobObserver;
 
 class InternalJob : public QObject
@@ -130,7 +131,9 @@ public:
 private slots:
     void start();
     void handleFinished();
-    void handleError(const Error &error);
+
+private:
+    Executor *m_executor;
 };
 
 
diff --git a/src/lib/buildgraph/executor.cpp b/src/lib/buildgraph/executor.cpp
index d1c609e98..9e9c778bb 100644
--- a/src/lib/buildgraph/executor.cpp
+++ b/src/lib/buildgraph/executor.cpp
@@ -43,6 +43,7 @@
 #include <tools/progressobserver.h>
 
 #include <QSet>
+#include <QTimer>
 
 #include <algorithm>
 
@@ -88,7 +89,6 @@ Executor::Executor(QObject *parent)
     , m_engine(0)
     , m_progressObserver(0)
     , m_state(ExecutorIdle)
-    , m_buildResult(SuccessfulBuild)
 {
     m_inputArtifactScanContext = new InputArtifactScannerContext(&m_scanResultCache);
     m_autoMoc = new AutoMoc;
@@ -124,7 +124,8 @@ void Executor::build(const QList<BuildProduct::Ptr> &productsToBuild)
     try {
         doBuild(productsToBuild);
     } catch (const Error &e) {
-        setError(e);
+        m_error = e;
+        QTimer::singleShot(0, this, SLOT(finish()));
     }
 }
 
@@ -132,10 +133,11 @@ void Executor::doBuild(const QList<BuildProduct::Ptr> &productsToBuild)
 {
     Q_ASSERT(m_buildOptions.maxJobCount > 0);
     Q_ASSERT(m_engine);
-    Q_ASSERT(m_state != ExecutorRunning);
+    Q_ASSERT(m_state == ExecutorIdle);
     m_leaves.clear();
-    m_buildResult = SuccessfulBuild;
     m_productsToBuild = productsToBuild;
+    m_error.clear();
+    m_explicitlyCanceled = false;
 
     QSet<BuildProject *> projects;
     foreach (const BuildProduct::ConstPtr &buildProduct, productsToBuild)
@@ -187,18 +189,10 @@ void Executor::doBuild(const QList<BuildProduct::Ptr> &productsToBuild)
     if (sourceFilesChanged)
         runAutoMoc();
     initLeaves(changedArtifacts);
-    if (!buildNextArtifact())
-        finish();
-}
-
-void Executor::cancelBuild()
-{
-    if (m_state != ExecutorRunning)
-        return;
-    setState(ExecutorCanceled);
-    cancelJobs();
-    m_buildResult = FailedBuild;
-    emit error(Error(Tr::tr("Build canceled.")));
+    if (!scheduleJobs()) {
+        qbsTrace() << "Nothing to do at all, finishing.";
+        QTimer::singleShot(0, this, SLOT(finish())); // Don't call back on the caller.
+    }
 }
 
 void Executor::setEngine(ScriptEngine *engine)
@@ -272,24 +266,13 @@ void Executor::initLeavesTopDown(Artifact *artifact, QSet<Artifact *> &seenArtif
     }
 }
 
-/**
-  * Returns true if there are still artifacts to traverse.
-  */
-bool Executor::buildNextArtifact()
+// Returns true if some artifacts are still waiting to be built or currently building.
+bool Executor::scheduleJobs()
 {
-    while (m_state == ExecutorRunning) {
-        if (m_leaves.isEmpty())
-            return !m_processingJobs.isEmpty();
-
-        if (m_availableJobs.isEmpty()) {
-            if (qbsLogLevel(LoggerDebug))
-                qbsDebug("[EXEC] No jobs available. Trying later.");
-            return true;
-        }
-
+    Q_ASSERT(m_state == ExecutorRunning);
+    while (!m_leaves.isEmpty() && !m_availableJobs.isEmpty())
         buildArtifact(m_leaves.takeFirst());
-    }
-    return false;
+    return !m_leaves.isEmpty() || !m_processingJobs.isEmpty();
 }
 
 static bool isUpToDate(Artifact *artifact)
@@ -340,6 +323,8 @@ static bool mustExecuteTransformer(const QSharedPointer<Transformer> &transforme
 
 void Executor::buildArtifact(Artifact *artifact)
 {
+    Q_ASSERT(!m_availableJobs.isEmpty());
+
     if (qbsLogLevel(LoggerDebug))
         qbsDebug() << "[EXEC] " << fileName(artifact);
 
@@ -451,25 +436,39 @@ void Executor::buildArtifact(Artifact *artifact)
     job->run(artifact->transformer.data(), artifact->product);
 }
 
-void Executor::finishJob(ExecutorJob *job)
+void Executor::finishJob(ExecutorJob *job, bool success)
 {
     Q_ASSERT(job);
+    Q_ASSERT(m_state != ExecutorIdle);
 
-    Artifact *processedArtifact = m_processingJobs.value(job);
-    Q_ASSERT(processedArtifact);
-
-    m_processingJobs.remove(job);
+    const QHash<ExecutorJob *, Artifact *>::Iterator it = m_processingJobs.find(job);
+    Q_ASSERT(it != m_processingJobs.end());
+    if (success)
+        finishArtifact(it.value());
+    m_processingJobs.erase(it);
     m_availableJobs.append(job);
 
-    finishArtifact(processedArtifact);
+    if (!success)
+        cancelJobs();
+
+    if (m_state == ExecutorRunning && m_progressObserver && m_progressObserver->canceled()) {
+        qbsTrace() << "Received cancel request; canceling build.";
+        m_explicitlyCanceled = true;
+        cancelJobs();
+    }
 
-    if (m_progressObserver && m_progressObserver->canceled()) {
-        cancelBuild();
+    if (m_state == ExecutorCanceling) {
+        if (m_processingJobs.isEmpty()) {
+            qbsTrace() << "All pending jobs are done, finishing.";
+            finish();
+        }
         return;
     }
 
-    if (!buildNextArtifact())
+    if (!scheduleJobs()) {
+        qbsTrace() << "Nothing left to build; finishing.";
         finish();
+    }
 }
 
 static bool allChildrenBuilt(Artifact *artifact)
@@ -549,11 +548,11 @@ void Executor::insertLeavesAfterAddingDependencies(QVector<Artifact *> dependenc
 
 void Executor::cancelJobs()
 {
+    qbsTrace() << "Canceling all jobs.";
+    setState(ExecutorCanceling);
     QList<ExecutorJob *> jobs = m_processingJobs.keys();
     foreach (ExecutorJob *job, jobs)
         job->cancel();
-    foreach (ExecutorJob *job, jobs)
-        job->waitForFinished();
 }
 
 void Executor::setupProgressObserver(bool mocWillRun)
@@ -581,10 +580,8 @@ void Executor::addExecutorJobs(int jobNumber)
             job->setMainThreadScriptEngine(m_engine);
         job->setObjectName(QString(QLatin1String("J%1")).arg(i));
         m_availableJobs.append(job);
-        connect(job, SIGNAL(error(QString)),
-                this, SLOT(onProcessError(QString)));
-        connect(job, SIGNAL(success()),
-                this, SLOT(onProcessSuccess()));
+        connect(job, SIGNAL(error(QString)), this, SLOT(onProcessError(QString)));
+        connect(job, SIGNAL(success()), this, SLOT(onProcessSuccess()));
     }
 }
 
@@ -621,17 +618,14 @@ void Executor::runAutoMoc()
         m_progressObserver->incrementProgressValue(m_mocEffort);
 }
 
-void Executor::onProcessError(QString errorString)
+void Executor::onProcessError(const QString &errorString)
 {
-    m_buildResult = FailedBuild;
-    if (m_buildOptions.keepGoing) {
-        qbsWarning() << tr("ignoring error: %1").arg(errorString);
-        ExecutorJob * const job = qobject_cast<ExecutorJob *>(sender());
-        finishJob(job);
-    } else {
-        setError(Error(errorString));
-        finish();
-    }
+    if (m_buildOptions.keepGoing)
+        qbsWarning() << Tr::tr("ignoring error: %1").arg(errorString);
+    else
+        qbsError() << errorString;
+    ExecutorJob * const job = qobject_cast<ExecutorJob *>(sender());
+    finishJob(job, false);
 }
 
 void Executor::onProcessSuccess()
@@ -650,13 +644,12 @@ void Executor::onProcessSuccess()
             artifact->timestamp = FileInfo(artifact->filePath()).lastModified();
     }
 
-    finishJob(job);
+    finishJob(job, true);
 }
 
 void Executor::finish()
 {
-    if (m_state == ExecutorIdle)
-        return;
+    Q_ASSERT(m_state != ExecutorIdle);
 
     QStringList unbuiltProductNames;
     foreach (BuildProduct::Ptr buildProduct, m_productsToBuild) {
@@ -669,13 +662,14 @@ void Executor::finish()
     }
     if (unbuiltProductNames.isEmpty()) {
         qbsInfo() << DontPrintLogLevel << LogOutputStdOut << TextColorGreen
-                  << "Build done.";
+                  << Tr::tr("Build done.");
     } else {
-        qbsError() << tr("The following products could not be built: %1.").arg(unbuiltProductNames.join(", "));
-        qbsInfo() << DontPrintLogLevel << LogOutputStdOut << TextColorRed
-                  << "Build failed.";
+        m_error = Error(Tr::tr("The following products could not be built: %1.")
+                 .arg(unbuiltProductNames.join(", ")));
     }
 
+    if (m_explicitlyCanceled)
+        m_error.append(Tr::tr("Build was canceled due to user request."));
     setState(ExecutorIdle);
     if (m_progressObserver)
         m_progressObserver->setFinished();
@@ -760,14 +754,5 @@ void Executor::setState(ExecutorState s)
     m_state = s;
 }
 
-void Executor::setError(const Error &e)
-{
-    if (m_state != ExecutorRunning)
-        return;
-    setState(ExecutorError);
-    cancelJobs();
-    emit error(e);
-}
-
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/buildgraph/executor.h b/src/lib/buildgraph/executor.h
index e6365e044..1d0cd2aa1 100644
--- a/src/lib/buildgraph/executor.h
+++ b/src/lib/buildgraph/executor.h
@@ -35,6 +35,7 @@
 #include <buildgraph/artifact.h>
 #include <buildgraph/scanresultcache.h>
 #include <tools/buildoptions.h>
+#include <tools/error.h>
 #include <tools/settings.h>
 
 #include <QObject>
@@ -57,53 +58,40 @@ public:
 
     void build(const QList<BuildProduct::Ptr> &productsToBuild);
 
-    enum ExecutorState {
-        ExecutorIdle,
-        ExecutorRunning,
-        ExecutorCanceled,
-        ExecutorError
-    };
-
-    enum BuildResult {
-        SuccessfulBuild,
-        FailedBuild
-    };
-
     void setEngine(ScriptEngine *engine);
     void setBuildOptions(const BuildOptions &buildOptions);
     void setProgressObserver(ProgressObserver *observer) { m_progressObserver = observer; }
-    ExecutorState state() const { return m_state; }
-    BuildResult buildResult() const { return m_buildResult; }
+
+    Error error() const { return m_error; }
+    bool hasError() const { return !error().entries().isEmpty(); }
 
 signals:
-    void error(const Error &error);
     void finished();
 
 private slots:
-    void onProcessError(QString errorString);
+    void onProcessError(const QString &errorString);
     void onProcessSuccess();
+    void finish();
 
 private:
+    enum ExecutorState { ExecutorIdle, ExecutorRunning, ExecutorCanceling };
+
     void doBuild(const QList<BuildProduct::Ptr> &productsToBuild);
-    void cancelBuild();
     void prepareBuildGraph(const Artifact::BuildState buildState, bool *sourceFilesChanged);
     void prepareBuildGraph_impl(Artifact *artifact, const Artifact::BuildState buildState, bool *sourceFilesChanged);
     void updateBuildGraph(Artifact::BuildState buildState);
     void updateBuildGraph_impl(Artifact *artifact, Artifact::BuildState buildState, QSet<Artifact *> &seenArtifacts);
     void initLeaves(const QList<Artifact *> &changedArtifacts);
     void initLeavesTopDown(Artifact *artifact, QSet<Artifact *> &seenArtifacts);
-    bool buildNextArtifact();
+    bool scheduleJobs();
     void buildArtifact(Artifact *artifact);
-    void finishJob(ExecutorJob *job);
+    void finishJob(ExecutorJob *job, bool success);
     void finishArtifact(Artifact *artifact);
-    void finish();
     void initializeArtifactsState();
     void setState(ExecutorState);
-    void setError(const Error &e);
     void addExecutorJobs(int jobNumber);
     void removeExecutorJobs(int jobNumber);
     void runAutoMoc();
-    void printScanningMessageOnce();
     void insertLeavesAfterAddingDependencies(QVector<Artifact *> dependencies);
     void cancelJobs();
     void setupProgressObserver(bool mocWillRun);
@@ -114,7 +102,6 @@ private:
     QList<ExecutorJob*> m_availableJobs;
     QHash<ExecutorJob*, Artifact *> m_processingJobs;
     ExecutorState m_state;
-    BuildResult m_buildResult;
     QList<BuildProduct::Ptr> m_productsToBuild;
     QList<Artifact *> m_roots;
     QList<Artifact *> m_leaves;
@@ -122,8 +109,8 @@ private:
     InputArtifactScannerContext *m_inputArtifactScanContext;
     AutoMoc *m_autoMoc;
     int m_mocEffort;
-
-    friend class ExecutorJob;
+    Error m_error;
+    bool m_explicitlyCanceled;
 };
 
 } // namespace Internal
diff --git a/src/lib/buildgraph/jscommandexecutor.cpp b/src/lib/buildgraph/jscommandexecutor.cpp
index 4b668b2ea..fa94e7d9a 100644
--- a/src/lib/buildgraph/jscommandexecutor.cpp
+++ b/src/lib/buildgraph/jscommandexecutor.cpp
@@ -48,6 +48,7 @@
 #include <QMutex>
 #include <QMutexLocker>
 #include <QThread>
+#include <QTimer>
 
 namespace qbs {
 namespace Internal {
@@ -150,7 +151,7 @@ void JsCommandExecutor::waitForFinished()
 void JsCommandExecutor::doStart()
 {
     if (dryRun()) {
-        emit finished();
+        QTimer::singleShot(0, this, SIGNAL(finished())); // Don't call back on the caller.
         return;
     }
     QFuture<JSRunner::result_type> future = QtConcurrent::run(JSRunner(jsCommand()), transformer());
diff --git a/src/lib/buildgraph/processcommandexecutor.cpp b/src/lib/buildgraph/processcommandexecutor.cpp
index 1dffb3d08..333c9c151 100644
--- a/src/lib/buildgraph/processcommandexecutor.cpp
+++ b/src/lib/buildgraph/processcommandexecutor.cpp
@@ -43,6 +43,7 @@
 #include <QScriptEngine>
 #include <QScriptValue>
 #include <QTemporaryFile>
+#include <QTimer>
 
 namespace qbs {
 namespace Internal {
@@ -103,7 +104,7 @@ void ProcessCommandExecutor::doStart()
         qbsInfo() << DontPrintLogLevel << LogOutputStdOut << m_commandLine;
     qbsDebug() << "[EXEC] " << m_commandLine;
     if (dryRun()) {
-        emit finished();
+        QTimer::singleShot(0, this, SIGNAL(finished())); // Don't call back on the caller.
         return;
     }
 
diff --git a/src/lib/tools/error.h b/src/lib/tools/error.h
index 349f98ef7..f0bfad0ca 100644
--- a/src/lib/tools/error.h
+++ b/src/lib/tools/error.h
@@ -60,7 +60,7 @@ class Error
 public:
     Error();
     Error(const Error &rhs);
-    Error(const QString &description,
+    explicit Error(const QString &description,
           const QString &file = QString(),
           int line = 0, int column = 0);
     Error(const QString &description, const CodeLocation &location);