ProjectExplorer: only add Tasks from the main thread

If something fails while extracting the build env from msvc vavars
scripts we want to add task hub entries to inform the user about a most
probably unusable toolchain. As the msvc toolchain detection is threaded
nowadays and the TaskHub is not thread safe we need to propagate the
error message to the main thread before adding the Task entries.

Change-Id: I5c67d3b8d58b22ea895afb6f22ee73f19fa52f17
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

4 files changed, 89 insertions, 60 deletions

diff --git a/src/plugins/projectexplorer/abstractmsvctoolchain.cpp b/src/plugins/projectexplorer/abstractmsvctoolchain.cpp
index 64de2dd707..c47c0b4307 100644
--- a/src/plugins/projectexplorer/abstractmsvctoolchain.cpp
+++ b/src/plugins/projectexplorer/abstractmsvctoolchain.cpp
@@ -316,10 +316,10 @@ bool AbstractMsvcToolChain::canClone() const
     return true;
 }
 
-bool AbstractMsvcToolChain::generateEnvironmentSettings(const Utils::Environment &env,
-                                                        const QString &batchFile,
-                                                        const QString &batchArgs,
-                                                        QMap<QString, QString> &envPairs)
+Utils::optional<QString> AbstractMsvcToolChain::generateEnvironmentSettings(const Utils::Environment &env,
+                                                                            const QString &batchFile,
+                                                                            const QString &batchArgs,
+                                                                            QMap<QString, QString> &envPairs)
 {
     const QString marker = "####################";
     // Create a temporary file name for the output. Use a temporary file here
@@ -342,7 +342,7 @@ bool AbstractMsvcToolChain::generateEnvironmentSettings(const Utils::Environment
     saver.write("@echo " + marker.toLocal8Bit() + "\r\n");
     if (!saver.finalize()) {
         qWarning("%s: %s", Q_FUNC_INFO, qPrintable(saver.errorString()));
-        return false;
+        return QString();
     }
 
     Utils::SynchronousProcess run;
@@ -369,12 +369,9 @@ bool AbstractMsvcToolChain::generateEnvironmentSettings(const Utils::Environment
 
     QString command = QDir::toNativeSeparators(batchFile);
     if (!response.stdErr().isEmpty()) {
-        TaskHub::addTask(Task::Error,
-                         QCoreApplication::translate("ProjectExplorer::Internal::AbstractMsvcToolChain",
-                                                     "Failed to retrieve MSVC Environment from \"%1\":\n"
-                                                     "%2")
-                         .arg(command, response.stdErr()), Constants::TASK_CATEGORY_COMPILE);
-        return false;
+        return QCoreApplication::translate("ProjectExplorer::Internal::AbstractMsvcToolChain",
+                                           "Failed to retrieve MSVC Environment from \"%1\":\n"
+                                           "%2").arg(command, response.stdErr());
     }
 
     if (response.result != Utils::SynchronousProcessResponse::Finished) {
@@ -382,12 +379,9 @@ bool AbstractMsvcToolChain::generateEnvironmentSettings(const Utils::Environment
         qWarning().noquote() << message;
         if (!batchArgs.isEmpty())
             command += ' ' + batchArgs;
-        TaskHub::addTask(Task::Error,
-                         QCoreApplication::translate("ProjectExplorer::Internal::AbstractMsvcToolChain",
-                                                     "Failed to retrieve MSVC Environment from \"%1\":\n"
-                                                     "%2")
-                         .arg(command, message), Constants::TASK_CATEGORY_COMPILE);
-        return false;
+        return QCoreApplication::translate("ProjectExplorer::Internal::AbstractMsvcToolChain",
+                                           "Failed to retrieve MSVC Environment from \"%1\":\n"
+                                           "%2").arg(command, message);
     }
 
     // The SDK/MSVC scripts do not return exit codes != 0. Check on stdout.
@@ -398,13 +392,13 @@ bool AbstractMsvcToolChain::generateEnvironmentSettings(const Utils::Environment
     const int start = stdOut.indexOf(marker);
     if (start == -1) {
         qWarning("Could not find start marker in stdout output.");
-        return false;
+        return QString();
     }
 
     const int end = stdOut.indexOf(marker, start + 1);
     if (end == -1) {
         qWarning("Could not find end marker in stdout output.");
-        return false;
+        return QString();
     }
 
     const QString output = stdOut.mid(start, end - start);
@@ -418,7 +412,7 @@ bool AbstractMsvcToolChain::generateEnvironmentSettings(const Utils::Environment
         }
     }
 
-    return true;
+    return Utils::nullopt;
 }
 
 /**
diff --git a/src/plugins/projectexplorer/abstractmsvctoolchain.h b/src/plugins/projectexplorer/abstractmsvctoolchain.h
index 0851e09cc0..7bfdf3ac46 100644
--- a/src/plugins/projectexplorer/abstractmsvctoolchain.h
+++ b/src/plugins/projectexplorer/abstractmsvctoolchain.h
@@ -33,6 +33,7 @@
 
 #include <utils/environment.h>
 #include <utils/fileutils.h>
+#include <utils/optional.h>
 
 namespace ProjectExplorer {
 namespace Internal {
@@ -71,10 +72,10 @@ public:
 
     bool operator ==(const ToolChain &) const override;
 
-    static bool generateEnvironmentSettings(const Utils::Environment &env,
-                                            const QString &batchFile,
-                                            const QString &batchArgs,
-                                            QMap<QString, QString> &envPairs);
+    static Utils::optional<QString> generateEnvironmentSettings(const Utils::Environment &env,
+                                                                const QString &batchFile,
+                                                                const QString &batchArgs,
+                                                                QMap<QString, QString> &envPairs);
 
 protected:
     class WarningFlagAdder
diff --git a/src/plugins/projectexplorer/msvctoolchain.cpp b/src/plugins/projectexplorer/msvctoolchain.cpp
index 24491e1598..8a277fe5f1 100644
--- a/src/plugins/projectexplorer/msvctoolchain.cpp
+++ b/src/plugins/projectexplorer/msvctoolchain.cpp
@@ -27,6 +27,7 @@
 
 #include "msvcparser.h"
 #include "projectexplorerconstants.h"
+#include "taskhub.h"
 #include "toolchainmanager.h"
 
 #include <utils/algorithm.h>
@@ -523,46 +524,57 @@ static QString winExpandDelayedEnvReferences(QString in, const Utils::Environmen
     return in;
 }
 
-void MsvcToolChain::environmentModifications(QFutureInterface<QList<Utils::EnvironmentItem>> &future,
-                                             QString vcvarsBat, QString varsBatArg)
+void MsvcToolChain::environmentModifications(
+        QFutureInterface<MsvcToolChain::GenerateEnvResult> &future,
+        QString vcvarsBat, QString varsBatArg)
 {
     const Utils::Environment inEnv = Utils::Environment::systemEnvironment();
     Utils::Environment outEnv;
     QMap<QString, QString> envPairs;
-    if (!generateEnvironmentSettings(inEnv, vcvarsBat, varsBatArg, envPairs))
-        return;
-
-    // Now loop through and process them
-    for (auto envIter = envPairs.cbegin(), eend = envPairs.cend(); envIter != eend; ++envIter) {
-        const QString expandedValue = winExpandDelayedEnvReferences(envIter.value(), inEnv);
-        if (!expandedValue.isEmpty())
-            outEnv.set(envIter.key(), expandedValue);
-    }
+    QList<Utils::EnvironmentItem> diff;
+    Utils::optional<QString> error = generateEnvironmentSettings(inEnv, vcvarsBat,
+                                                                 varsBatArg, envPairs);
+    if (!error) {
+
+        // Now loop through and process them
+        for (auto envIter = envPairs.cbegin(), end = envPairs.cend(); envIter != end; ++envIter) {
+            const QString expandedValue = winExpandDelayedEnvReferences(envIter.value(), inEnv);
+            if (!expandedValue.isEmpty())
+                outEnv.set(envIter.key(), expandedValue);
+        }
 
-    if (debug) {
-        const QStringList newVars = outEnv.toStringList();
-        const QStringList oldVars = inEnv.toStringList();
-        QDebug nsp = qDebug().nospace();
-        foreach (const QString &n, newVars) {
-            if (!oldVars.contains(n))
-                nsp << n << '\n';
+        if (debug) {
+            const QStringList newVars = outEnv.toStringList();
+            const QStringList oldVars = inEnv.toStringList();
+            QDebug nsp = qDebug().nospace();
+            foreach (const QString &n, newVars) {
+                if (!oldVars.contains(n))
+                    nsp << n << '\n';
+            }
         }
-    }
 
-    QList<Utils::EnvironmentItem> diff = inEnv.diff(outEnv, true);
-    for (int i = diff.size() - 1; i >= 0; --i) {
-        if (diff.at(i).name.startsWith(QLatin1Char('='))) { // Exclude "=C:", "=EXITCODE"
-            diff.removeAt(i);
+        diff = inEnv.diff(outEnv, true);
+        for (int i = diff.size() - 1; i >= 0; --i) {
+            if (diff.at(i).name.startsWith(QLatin1Char('='))) { // Exclude "=C:", "=EXITCODE"
+                diff.removeAt(i);
+            }
         }
     }
 
-    future.reportResult(diff);
+    future.reportResult({error, diff});
 }
 
-void MsvcToolChain::initEnvModWatcher(const QFuture<QList<Utils::EnvironmentItem> > &future)
+void MsvcToolChain::initEnvModWatcher(const QFuture<GenerateEnvResult> &future)
 {
-    QObject::connect(&m_envModWatcher, &QFutureWatcher<QList<Utils::EnvironmentItem>>::resultReadyAt, [&]() {
-        updateEnvironmentModifications(m_envModWatcher.result());
+    QObject::connect(&m_envModWatcher, &QFutureWatcher<GenerateEnvResult>::resultReadyAt, [&]() {
+        const GenerateEnvResult &result = m_envModWatcher.result();
+        if (result.error) {
+            const QString &errorMessage = *result.error;
+            if (!errorMessage.isEmpty())
+                TaskHub::addTask(Task::Error, errorMessage, Constants::TASK_CATEGORY_COMPILE);
+        } else {
+            updateEnvironmentModifications(result.environmentItems);
+        }
     });
     m_envModWatcher.setFuture(future);
 }
@@ -578,15 +590,23 @@ void MsvcToolChain::updateEnvironmentModifications(QList<Utils::EnvironmentItem>
 
 Utils::Environment MsvcToolChain::readEnvironmentSetting(const Utils::Environment& env) const
 {
-    Utils::Environment result = env;
+    Utils::Environment resultEnv = env;
     if (m_environmentModifications.isEmpty()) {
         m_envModWatcher.waitForFinished();
-        if (m_envModWatcher.future().isFinished() && !m_envModWatcher.future().isCanceled())
-            result.modify(m_envModWatcher.result());
+        if (m_envModWatcher.future().isFinished() && !m_envModWatcher.future().isCanceled()) {
+            const GenerateEnvResult &result = m_envModWatcher.result();
+            if (result.error) {
+                const QString &errorMessage = *result.error;
+                if (!errorMessage.isEmpty())
+                    TaskHub::addTask(Task::Error, errorMessage, Constants::TASK_CATEGORY_COMPILE);
+            } else {
+                resultEnv.modify(result.environmentItems);
+            }
+        }
     } else {
-        result.modify(m_environmentModifications);
+        resultEnv.modify(m_environmentModifications);
     }
-    return result;
+    return resultEnv;
 }
 
 // --------------------------------------------------------------------------
@@ -608,7 +628,14 @@ MsvcToolChain::MsvcToolChain(const MsvcToolChain &other)
         initEnvModWatcher(other.m_envModWatcher.future());
     } else if (m_environmentModifications.isEmpty() && other.m_envModWatcher.future().isFinished()
                && !other.m_envModWatcher.future().isCanceled()) {
-        m_environmentModifications = other.m_envModWatcher.result();
+        const GenerateEnvResult &result = m_envModWatcher.result();
+        if (result.error) {
+            const QString &errorMessage = *result.error;
+            if (!errorMessage.isEmpty())
+                TaskHub::addTask(Task::Error, errorMessage, Constants::TASK_CATEGORY_COMPILE);
+        } else {
+            updateEnvironmentModifications(result.environmentItems);
+        }
     }
 
     setDisplayName(other.displayName());
diff --git a/src/plugins/projectexplorer/msvctoolchain.h b/src/plugins/projectexplorer/msvctoolchain.h
index e71773800e..8cd39af2dc 100644
--- a/src/plugins/projectexplorer/msvctoolchain.h
+++ b/src/plugins/projectexplorer/msvctoolchain.h
@@ -31,6 +31,8 @@
 
 #include <QFutureWatcher>
 
+#include <utils/optional.h>
+
 QT_FORWARD_DECLARE_CLASS(QLabel)
 QT_FORWARD_DECLARE_CLASS(QVersionNumber)
 
@@ -89,13 +91,18 @@ protected:
                                 const Utils::Environment &env) const override;
 
 private:
-    static void environmentModifications(QFutureInterface<QList<Utils::EnvironmentItem> > &future,
+    struct GenerateEnvResult
+    {
+        Utils::optional<QString> error;
+        QList<Utils::EnvironmentItem> environmentItems;
+    };
+    static void environmentModifications(QFutureInterface<GenerateEnvResult> &future,
                                          QString vcvarsBat, QString varsBatArg);
-    void initEnvModWatcher(const QFuture<QList<Utils::EnvironmentItem>> &future);
+    void initEnvModWatcher(const QFuture<GenerateEnvResult> &future);
     void updateEnvironmentModifications(QList<Utils::EnvironmentItem> modifications);
 
     mutable QList<Utils::EnvironmentItem> m_environmentModifications;
-    mutable QFutureWatcher<QList<Utils::EnvironmentItem>> m_envModWatcher;
+    mutable QFutureWatcher<GenerateEnvResult> m_envModWatcher;
 
     QString m_varsBatArg; // Argument
 };