ProjectExplorer: Rework the build step run interface

Originally, the build manager used to run all build steps in a dedicated
thread. Communication between the step and the manager happened via a
QFutureInterface that was passed into the step's run() function.
Later, new steps were added that operated asynchronously, so the build
manager had to differentiate between the different kinds of steps for
starting and stopping.
These days, almost all build and deploy steps work asynchronously, which
made the QFuture-based interface look increasingly odd.
With this patch, all build steps are expected to work asynchronously, so
the build manager no longer needs to differentiate. Steps are started
and requested to stop via the run() and cancel() functions,
respectively, and emit the finished() signal when they are done. Build
step implementors no longer have to deal with a QFutureInterface. For
steps whose implementation is inherently synchronous, the BuildStep base
class offers a runInThread() function.

Change-Id: If905c68b234c5a669f6e19f43142eaa57d594803
Reviewed-by: hjk <hjk@qt.io>

10 files changed, 93 insertions, 98 deletions

diff --git a/src/plugins/projectexplorer/abstractprocessstep.cpp b/src/plugins/projectexplorer/abstractprocessstep.cpp
index a2f2e752ee..e85a9648ec 100644
--- a/src/plugins/projectexplorer/abstractprocessstep.cpp
+++ b/src/plugins/projectexplorer/abstractprocessstep.cpp
@@ -104,7 +104,6 @@ public:
     Private(AbstractProcessStep *q) : q(q) {}
 
     AbstractProcessStep *q;
-    QFutureInterface<bool> *m_futureInterface = nullptr;
     std::unique_ptr<Utils::QtcProcess> m_process;
     std::unique_ptr<IOutputParser> m_outputParserChain;
     ProcessParameters m_param;
@@ -125,7 +124,6 @@ AbstractProcessStep::AbstractProcessStep(BuildStepList *bsl, Core::Id id) :
     BuildStep(bsl, id),
     d(new Private(this))
 {
-    setRunInGuiThread(true);
 }
 
 AbstractProcessStep::~AbstractProcessStep()
@@ -209,7 +207,7 @@ bool AbstractProcessStep::init()
     YourBuildStep::run().
 */
 
-void AbstractProcessStep::run(QFutureInterface<bool> &fi)
+void AbstractProcessStep::doRun()
 {
     QDir wd(d->m_param.effectiveWorkingDirectory());
     if (!wd.exists()) {
@@ -217,7 +215,7 @@ void AbstractProcessStep::run(QFutureInterface<bool> &fi)
             emit addOutput(tr("Could not create directory \"%1\"")
                            .arg(QDir::toNativeSeparators(wd.absolutePath())),
                            BuildStep::OutputFormat::ErrorMessage);
-            reportRunResult(fi, false);
+            finish(false);
             return;
         }
     }
@@ -225,12 +223,10 @@ void AbstractProcessStep::run(QFutureInterface<bool> &fi)
     QString effectiveCommand = d->m_param.effectiveCommand();
     if (!QFileInfo::exists(effectiveCommand)) {
         processStartupFailed();
-        reportRunResult(fi, false);
+        finish(false);
         return;
     }
 
-    d->m_futureInterface = &fi;
-
     d->m_process.reset(new Utils::QtcProcess());
     d->m_process->setUseCtrlCStub(Utils::HostOsInfo::isWindowsHost());
     d->m_process->setWorkingDirectory(wd.absolutePath());
@@ -249,13 +245,13 @@ void AbstractProcessStep::run(QFutureInterface<bool> &fi)
         processStartupFailed();
         d->m_process.reset();
         d->m_outputParserChain.reset();
-        reportRunResult(fi, false);
+        finish(false);
         return;
     }
     processStarted();
 }
 
-void AbstractProcessStep::cancel()
+void AbstractProcessStep::doCancel()
 {
     Core::Reaper::reap(d->m_process.release());
 }
@@ -275,7 +271,7 @@ void AbstractProcessStep::cleanUp(QProcess *process)
     d->m_process.reset();
 
     // Report result
-    reportRunResult(*d->m_futureInterface, returnValue);
+    finish(returnValue);
 }
 
 /*!
@@ -420,9 +416,9 @@ void AbstractProcessStep::stdError(const QString &line)
     emit addOutput(line, BuildStep::OutputFormat::Stderr, BuildStep::DontAppendNewline);
 }
 
-QFutureInterface<bool> *AbstractProcessStep::futureInterface() const
+void AbstractProcessStep::finish(bool success)
 {
-    return d->m_futureInterface;
+    emit finished(success);
 }
 
 void AbstractProcessStep::taskAdded(const Task &task, int linkedOutputLines, int skipLines)
diff --git a/src/plugins/projectexplorer/abstractprocessstep.h b/src/plugins/projectexplorer/abstractprocessstep.h
index 84b7caa1d5..5d76bf7f4e 100644
--- a/src/plugins/projectexplorer/abstractprocessstep.h
+++ b/src/plugins/projectexplorer/abstractprocessstep.h
@@ -41,10 +41,6 @@ class PROJECTEXPLORER_EXPORT AbstractProcessStep : public BuildStep
     Q_OBJECT
 
 public:
-    bool init() override;
-    void run(QFutureInterface<bool> &) override;
-    void cancel() override;
-
     ProcessParameters *processParameters();
 
     bool ignoreReturnValue();
@@ -59,6 +55,8 @@ public:
 protected:
     AbstractProcessStep(BuildStepList *bsl, Core::Id id);
     ~AbstractProcessStep() override;
+    bool init() override;
+    void doRun() override;
 
     virtual void processStarted();
     virtual void processFinished(int exitCode, QProcess::ExitStatus status);
@@ -67,9 +65,11 @@ protected:
     virtual void stdOutput(const QString &line);
     virtual void stdError(const QString &line);
 
-    QFutureInterface<bool> *futureInterface() const;
+    void doCancel() override;
 
 private:
+    virtual void finish(bool success);
+
     void processReadyReadStdOutput();
     void processReadyReadStdError();
     void slotProcessFinished(int, QProcess::ExitStatus);
diff --git a/src/plugins/projectexplorer/buildmanager.cpp b/src/plugins/projectexplorer/buildmanager.cpp
index 68fe6f1356..544677517a 100644
--- a/src/plugins/projectexplorer/buildmanager.cpp
+++ b/src/plugins/projectexplorer/buildmanager.cpp
@@ -79,9 +79,8 @@ public:
     // is set to true while canceling, so that nextBuildStep knows that the BuildStep finished because of canceling
     bool m_skipDisabled = false;
     bool m_canceling = false;
-    QFutureWatcher<bool> m_watcher;
-    QFutureInterface<bool> m_futureInterfaceForAysnc;
-    BuildStep *m_currentBuildStep;
+    bool m_lastStepSucceeded = true;
+    BuildStep *m_currentBuildStep = nullptr;
     QString m_currentConfiguration;
     // used to decide if we are building a project to decide when to emit buildStateChanged(Project *)
     QHash<Project *, int>  m_activeBuildSteps;
@@ -107,16 +106,6 @@ BuildManager::BuildManager(QObject *parent, QAction *cancelBuildAction)
     m_instance = this;
     d = new BuildManagerPrivate;
 
-    connect(&d->m_watcher, &QFutureWatcherBase::finished,
-            this, &BuildManager::nextBuildQueue, Qt::QueuedConnection);
-
-    connect(&d->m_watcher, &QFutureWatcherBase::progressValueChanged,
-            this, &BuildManager::progressChanged);
-    connect(&d->m_watcher, &QFutureWatcherBase::progressTextChanged,
-            this, &BuildManager::progressTextChanged);
-    connect(&d->m_watcher, &QFutureWatcherBase::progressRangeChanged,
-            this, &BuildManager::progressChanged);
-
     connect(SessionManager::instance(), &SessionManager::aboutToRemoveProject,
             this, &BuildManager::aboutToRemoveProject);
 
@@ -203,14 +192,9 @@ void BuildManager::cancel()
         if (d->m_canceling)
             return;
         d->m_canceling = true;
-        d->m_watcher.cancel();
-        if (d->m_currentBuildStep->runInGuiThread()) {
-            d->m_currentBuildStep->cancel();
-            while (d->m_canceling)
-                QApplication::processEvents(QEventLoop::ExcludeUserInputEvents);
-        } else {
-            d->m_watcher.waitForFinished();
-        }
+        d->m_currentBuildStep->cancel();
+        while (d->m_canceling)
+            QApplication::processEvents(QEventLoop::ExcludeUserInputEvents); // TODO: Is this really necessary?
     }
 }
 
@@ -352,8 +336,6 @@ void BuildManager::addToOutputWindow(const QString &string, BuildStep::OutputFor
 
 void BuildManager::nextBuildQueue()
 {
-    d->m_futureInterfaceForAysnc = QFutureInterface<bool>();
-
     d->m_outputWindow->flush();
     if (d->m_canceling) {
         d->m_canceling = false;
@@ -375,7 +357,7 @@ void BuildManager::nextBuildQueue()
     d->m_progressFutureInterface->setProgressValueAndText(d->m_progress*100, msgProgress(d->m_progress, d->m_maxProgress));
     decrementActiveBuildSteps(d->m_currentBuildStep);
 
-    const bool success = d->m_skipDisabled || d->m_watcher.result();
+    const bool success = d->m_skipDisabled || d->m_lastStepSucceeded;
     if (success) {
         nextStep();
     } else {
@@ -397,28 +379,10 @@ void BuildManager::nextBuildQueue()
     }
 }
 
-void BuildManager::progressChanged()
+void BuildManager::progressChanged(int percent, const QString &text)
 {
-    if (!d->m_progressFutureInterface)
-        return;
-    int range = d->m_watcher.progressMaximum() - d->m_watcher.progressMinimum();
-    if (range != 0) {
-        int percent = (d->m_watcher.progressValue() - d->m_watcher.progressMinimum()) * 100 / range;
-        d->m_progressFutureInterface->setProgressValueAndText(d->m_progress * 100 + percent, msgProgress(d->m_progress, d->m_maxProgress)
-                                                              + QLatin1Char('\n') + d->m_watcher.progressText());
-    }
-}
-
-void BuildManager::progressTextChanged()
-{
-    if (!d->m_progressFutureInterface)
-        return;
-    int range = d->m_watcher.progressMaximum() - d->m_watcher.progressMinimum();
-    int percent = 0;
-    if (range != 0)
-        percent = (d->m_watcher.progressValue() - d->m_watcher.progressMinimum()) * 100 / range;
-    d->m_progressFutureInterface->setProgressValueAndText(d->m_progress*100 + percent, msgProgress(d->m_progress, d->m_maxProgress) +
-                                                          QLatin1Char('\n') + d->m_watcher.progressText());
+    if (d->m_progressFutureInterface)
+        d->m_progressFutureInterface->setProgressValueAndText(percent, text);
 }
 
 void BuildManager::nextStep()
@@ -445,12 +409,16 @@ void BuildManager::nextStep()
             return;
         }
 
-        if (d->m_currentBuildStep->runInGuiThread()) {
-            d->m_watcher.setFuture(d->m_futureInterfaceForAysnc.future());
-            d->m_currentBuildStep->run(d->m_futureInterfaceForAysnc);
-        } else {
-            d->m_watcher.setFuture(Utils::runAsync(&BuildStep::run, d->m_currentBuildStep));
-        }
+        static const auto finishedHandler = [](bool success)  {
+            d->m_lastStepSucceeded = success;
+            disconnect(d->m_currentBuildStep, nullptr, instance(), nullptr);
+            BuildManager::nextBuildQueue();
+        };
+        connect(d->m_currentBuildStep, &BuildStep::finished, instance(), finishedHandler,
+                Qt::QueuedConnection);
+        connect(d->m_currentBuildStep, &BuildStep::progress,
+                instance(), &BuildManager::progressChanged);
+        d->m_currentBuildStep->run();
     } else {
         d->m_running = false;
         d->m_previousBuildStepProject = nullptr;
diff --git a/src/plugins/projectexplorer/buildmanager.h b/src/plugins/projectexplorer/buildmanager.h
index b4cbcd1966..fc23bc851a 100644
--- a/src/plugins/projectexplorer/buildmanager.h
+++ b/src/plugins/projectexplorer/buildmanager.h
@@ -85,8 +85,7 @@ private:
                            BuildStep::OutputNewlineSetting newlineSettings = BuildStep::DoAppendNewline);
 
     static void nextBuildQueue();
-    static void progressChanged();
-    static void progressTextChanged();
+    static void progressChanged(int percent, const QString &text);
     static void emitCancelMessage();
     static void showBuildResults();
     static void updateTaskCount();
diff --git a/src/plugins/projectexplorer/buildstep.cpp b/src/plugins/projectexplorer/buildstep.cpp
index a494162b10..d293b09290 100644
--- a/src/plugins/projectexplorer/buildstep.cpp
+++ b/src/plugins/projectexplorer/buildstep.cpp
@@ -34,8 +34,11 @@
 
 #include <utils/algorithm.h>
 #include <utils/qtcassert.h>
+#include <utils/runextensions.h>
 
 #include <QFormLayout>
+#include <QFutureWatcher>
+#include <QPointer>
 
 /*!
     \class ProjectExplorer::BuildStep
@@ -127,6 +130,18 @@ BuildStep::BuildStep(BuildStepList *bsl, Core::Id id) :
     expander->registerSubProvider([this] { return projectConfiguration()->macroExpander(); });
 }
 
+void BuildStep::run()
+{
+    m_cancelFlag = false;
+    doRun();
+}
+
+void BuildStep::cancel()
+{
+    m_cancelFlag = true;
+    doCancel();
+}
+
 BuildStepConfigWidget *BuildStep::createConfigWidget()
 {
     auto widget = new BuildStepConfigWidget(this);
@@ -218,25 +233,32 @@ void BuildStep::setWidgetExpandedByDefault(bool widgetExpandedByDefault)
     immutable steps are run. The default implementation returns \c false.
 */
 
-bool BuildStep::runInGuiThread() const
+void BuildStep::runInThread(const std::function<bool()> &syncImpl)
 {
-    return m_runInGuiThread;
+    m_runInGuiThread = false;
+    m_cancelFlag = false;
+    auto * const watcher = new QFutureWatcher<bool>(this);
+    connect(watcher, &QFutureWatcher<bool>::finished, this, [this, watcher] {
+        emit finished(watcher->result());
+        watcher->deleteLater();
+    });
+    watcher->setFuture(Utils::runAsync(syncImpl));
 }
 
-void BuildStep::setRunInGuiThread(bool runInGuiThread)
+std::function<bool ()> BuildStep::cancelChecker() const
 {
-    m_runInGuiThread = runInGuiThread;
+    return [step = QPointer<const BuildStep>(this)] { return step && step->isCanceled(); };
 }
 
-/*!
-    This function needs to be reimplemented only for build steps that return
-    \c false from runInGuiThread().
+bool BuildStep::isCanceled() const
+{
+    return m_cancelFlag;
+}
 
-    \sa runInGuiThread()
-*/
-void BuildStep::cancel()
+void BuildStep::doCancel()
 {
-    // Do nothing
+    QTC_ASSERT(!m_runInGuiThread, qWarning() << "Build step" << displayName()
+               << "neeeds to implement the doCancel() function");
 }
 
 void BuildStep::setEnabled(bool b)
diff --git a/src/plugins/projectexplorer/buildstep.h b/src/plugins/projectexplorer/buildstep.h
index 2c53ea30cd..81ba409470 100644
--- a/src/plugins/projectexplorer/buildstep.h
+++ b/src/plugins/projectexplorer/buildstep.h
@@ -33,6 +33,9 @@
 #include <QFutureInterface>
 #include <QWidget>
 
+#include <functional>
+#include <memory>
+
 namespace ProjectExplorer {
 
 class BuildConfiguration;
@@ -54,11 +57,10 @@ protected:
 
 public:
     virtual bool init() = 0;
-    virtual void run(QFutureInterface<bool> &fi) = 0;
+    void run();
+    void cancel();
     virtual BuildStepConfigWidget *createConfigWidget();
 
-    virtual void cancel();
-
     bool fromMap(const QVariantMap &map) override;
     QVariantMap toMap() const override;
 
@@ -88,9 +90,6 @@ public:
     bool isImmutable() const { return m_immutable; }
     void setImmutable(bool immutable) { m_immutable = immutable; }
 
-    bool runInGuiThread() const;
-    void setRunInGuiThread(bool runInGuiThread);
-
 signals:
     /// Adds a \p task to the Issues pane.
     /// Do note that for linking compile output with tasks, you should first emit the task
@@ -104,11 +103,24 @@ signals:
 
     void enabledChanged();
 
+    void progress(int percentage, const QString &message);
+    void finished(bool result);
+
+protected:
+    void runInThread(const std::function<bool()> &syncImpl);
+
+    std::function<bool()> cancelChecker() const;
+    bool isCanceled() const;
+
 private:
+    virtual void doRun() = 0;
+    virtual void doCancel();
+
+    std::atomic_bool m_cancelFlag;
     bool m_enabled = true;
     bool m_immutable = false;
     bool m_widgetExpandedByDefault = true;
-    bool m_runInGuiThread = false;
+    bool m_runInGuiThread = true;
 };
 
 class PROJECTEXPLORER_EXPORT BuildStepInfo
diff --git a/src/plugins/projectexplorer/devicesupport/devicecheckbuildstep.cpp b/src/plugins/projectexplorer/devicesupport/devicecheckbuildstep.cpp
index b1a0921dbe..7935efb5fc 100644
--- a/src/plugins/projectexplorer/devicesupport/devicecheckbuildstep.cpp
+++ b/src/plugins/projectexplorer/devicesupport/devicecheckbuildstep.cpp
@@ -77,9 +77,9 @@ bool DeviceCheckBuildStep::init()
     return true;
 }
 
-void DeviceCheckBuildStep::run(QFutureInterface<bool> &fi)
+void DeviceCheckBuildStep::doRun()
 {
-    reportRunResult(fi, true);
+    emit finished(true);
 }
 
 Core::Id DeviceCheckBuildStep::stepId()
diff --git a/src/plugins/projectexplorer/devicesupport/devicecheckbuildstep.h b/src/plugins/projectexplorer/devicesupport/devicecheckbuildstep.h
index 118c416e7f..6fc984b690 100644
--- a/src/plugins/projectexplorer/devicesupport/devicecheckbuildstep.h
+++ b/src/plugins/projectexplorer/devicesupport/devicecheckbuildstep.h
@@ -39,7 +39,7 @@ public:
     explicit DeviceCheckBuildStep(BuildStepList *bsl);
 
     bool init() override;
-    void run(QFutureInterface<bool> &fi) override;
+    void doRun() override;
 
     static Core::Id stepId();
     static QString displayName();
diff --git a/src/plugins/projectexplorer/processstep.cpp b/src/plugins/projectexplorer/processstep.cpp
index 5c66b38022..c6c6057f4b 100644
--- a/src/plugins/projectexplorer/processstep.cpp
+++ b/src/plugins/projectexplorer/processstep.cpp
@@ -71,9 +71,9 @@ bool ProcessStep::init()
     return AbstractProcessStep::init();
 }
 
-void ProcessStep::run(QFutureInterface<bool> & fi)
+void ProcessStep::doRun()
 {
-    AbstractProcessStep::run(fi);
+    AbstractProcessStep::doRun();
 }
 
 BuildStepConfigWidget *ProcessStep::createConfigWidget()
diff --git a/src/plugins/projectexplorer/processstep.h b/src/plugins/projectexplorer/processstep.h
index bdc0a0812b..3bff8fbdca 100644
--- a/src/plugins/projectexplorer/processstep.h
+++ b/src/plugins/projectexplorer/processstep.h
@@ -45,9 +45,6 @@ class ProcessStep : public AbstractProcessStep
 public:
     explicit ProcessStep(BuildStepList *bsl);
 
-    bool init() override;
-    void run(QFutureInterface<bool> &) override;
-
     BuildStepConfigWidget *createConfigWidget() override;
 
     QString command() const;
@@ -58,9 +55,10 @@ public:
     void setArguments(const QString &arguments);
     void setWorkingDirectory(const QString &workingDirectory);
 
-    QVariantMap toMap() const override;
-
 private:
+    bool init() override;
+    void doRun() override;
+    QVariantMap toMap() const override;
     bool fromMap(const QVariantMap &map) override;
 
     QString m_command;