UpdateInfo: Retrieve output of maintenancetool calls separately

ShellCommand doesn't support retrieval of individual sub-jobs.
Simply use chained QtcProcesses and add a progress item directly.

This gets rid of fragile hacks that were needed to combine the output of
the two calls into a valid XML document.

Change-Id: I426c74f5fd2c522850daf9c077bfd09ce3f88bb1
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Christian Stenger <christian.stenger@qt.io>

src/plugins/updateinfo/updateinfoplugin.cpp

@@ -32,8 +32,9 @@
 #include <coreplugin/actionmanager/actionmanager.h>
 #include <coreplugin/coreconstants.h>
 #include <coreplugin/icore.h>
+#include <coreplugin/progressmanager/futureprogress.h>
+#include <coreplugin/progressmanager/progressmanager.h>
 #include <coreplugin/settingsdatabase.h>
-#include <coreplugin/shellcommand.h>
 #include <utils/algorithm.h>
 #include <utils/fileutils.h>
 #include <utils/infobar.h>
@@ -53,6 +54,8 @@
 #include <QTimer>
 #include <QVersionNumber>
 
+#include <memory>
+
 Q_LOGGING_CATEGORY(log, "qtc.updateinfo", QtWarningMsg)
 
 const char UpdaterGroup[] = "Updater";
@@ -68,6 +71,7 @@ const char InstallUpdates[] = "UpdateInfo.InstallUpdates";
 const char InstallQtUpdates[] = "UpdateInfo.InstallQtUpdates";
 
 using namespace Core;
+using namespace Utils;
 
 namespace UpdateInfo {
 namespace Internal {
@@ -76,11 +80,11 @@ class UpdateInfoPluginPrivate
 {
 public:
     QString m_maintenanceTool;
-    QPointer<ShellCommand> m_checkUpdatesCommand;
+    std::unique_ptr<QtcProcess> m_maintenanceToolProcess;
     QPointer<FutureProgress> m_progress;
-    QString m_collectedOutput;
+    QString m_updateOutput;
+    QString m_packagesOutput;
     QTimer *m_checkUpdatesTimer = nullptr;
-
     struct Settings
     {
         bool automaticCheck = true;
@@ -125,7 +129,7 @@ void UpdateInfoPlugin::stopAutoCheckForUpdates()
 
 void UpdateInfoPlugin::doAutoCheckForUpdates()
 {
-    if (d->m_checkUpdatesCommand)
+    if (d->m_maintenanceToolProcess)
         return; // update task is still running (might have been run manually just before)
 
     if (nextCheckDate().isValid() && nextCheckDate() > QDate::currentDate())
@@ -138,51 +142,86 @@ void UpdateInfoPlugin::startCheckForUpdates()
 {
     stopCheckForUpdates();
 
-    d->m_checkUpdatesCommand = new ShellCommand({}, Utils::Environment::systemEnvironment());
+    QFutureInterface<void> futureIf;
-    d->m_checkUpdatesCommand->setDisplayName(tr("Checking for Updates"));
+    FutureProgress *futureProgress
-    connect(d->m_checkUpdatesCommand, &ShellCommand::stdOutText, this, &UpdateInfoPlugin::collectCheckForUpdatesOutput);
+        = ProgressManager::addTimedTask(futureIf,
-    connect(d->m_checkUpdatesCommand, &ShellCommand::finished, this, &UpdateInfoPlugin::checkForUpdatesFinished);
+                                        tr("Checking for Updates"),
-    d->m_checkUpdatesCommand->addJob({Utils::FilePath::fromString(d->m_maintenanceTool),
+                                        Id("UpdateInfo.CheckingForUpdates"),
-                                      {"ch", "-g", "*=false,ifw.package.*=true"}},
+                                        60);
-                                     60 * 3, // 3 minutes timeout
+    futureProgress->setKeepOnFinish(FutureProgress::KeepOnFinishTillUserInteraction);
-                                     /*workingDirectory=*/{},
+    futureProgress->setSubtitleVisibleInStatusBar(true);
-                                     [](int /*exitCode*/) {
+    connect(futureProgress, &FutureProgress::canceled, this, [this, futureIf]() mutable {
-                                         return Utils::ProcessResult::FinishedWithSuccess;
+        futureIf.reportCanceled();
+        futureIf.reportFinished();
+        stopCheckForUpdates();
     });
+
+    d->m_maintenanceToolProcess.reset(new QtcProcess);
+    d->m_maintenanceToolProcess->setCommand({Utils::FilePath::fromString(d->m_maintenanceTool),
+                                             {"ch", "-g", "*=false,ifw.package.*=true"}});
+    d->m_maintenanceToolProcess->setTimeoutS(3 * 60); // 3 minutes
+    // TODO handle error
+    connect(
+        d->m_maintenanceToolProcess.get(),
+        &QtcProcess::done,
+        this,
+        [this, futureIf]() mutable {
+            if (d->m_maintenanceToolProcess->result() == ProcessResult::FinishedWithSuccess) {
+                d->m_updateOutput = d->m_maintenanceToolProcess->stdOut();
                 if (d->m_settings.checkForQtVersions) {
-        d->m_checkUpdatesCommand
+                    d->m_maintenanceToolProcess.reset(new QtcProcess);
-            ->addJob({Utils::FilePath::fromString(d->m_maintenanceTool),
+                    d->m_maintenanceToolProcess->setCommand(
-                      {"se", "qt[.]qt[0-9][.][0-9]+$", "-g", "*=false,ifw.package.*=true"}},
+                        {Utils::FilePath::fromString(d->m_maintenanceTool),
-                     60 * 3, // 3 minutes timeout
+                         {"se", "qt[.]qt[0-9][.][0-9]+$", "-g", "*=false,ifw.package.*=true"}});
-                     /*workingDirectory=*/{},
+                    d->m_maintenanceToolProcess->setTimeoutS(3 * 60); // 3 minutes
-                     [](int /*exitCode*/) { return Utils::ProcessResult::FinishedWithSuccess; });
+                    connect(
+                        d->m_maintenanceToolProcess.get(),
+                        &QtcProcess::done,
+                        this,
+                        [this, futureIf]() mutable {
+                            if (d->m_maintenanceToolProcess->result()
+                                == ProcessResult::FinishedWithSuccess) {
+                                d->m_packagesOutput = d->m_maintenanceToolProcess->stdOut();
+                                d->m_maintenanceToolProcess.reset();
+                                futureIf.reportFinished();
+                                checkForUpdatesFinished();
+                            } else {
+                                futureIf.reportCanceled(); // is used to indicate error
+                                futureIf.reportFinished();
                             }
-    d->m_checkUpdatesCommand->execute();
+                        },
-    d->m_progress = d->m_checkUpdatesCommand->futureProgress();
+                        Qt::QueuedConnection);
-    if (d->m_progress) {
+                    d->m_maintenanceToolProcess->start();
-        d->m_progress->setKeepOnFinish(FutureProgress::KeepOnFinishTillUserInteraction);
+                } else {
-        d->m_progress->setSubtitleVisibleInStatusBar(true);
+                    d->m_maintenanceToolProcess.reset();
+                    futureIf.reportFinished();
+                    checkForUpdatesFinished();
                 }
+            } else {
+                futureIf.reportCanceled(); // is used to indicate error
+                futureIf.reportFinished();
+            }
+        },
+        Qt::QueuedConnection);
+
+    d->m_maintenanceToolProcess->start();
+    futureIf.reportStarted();
+
     emit checkForUpdatesRunningChanged(true);
 }
 
 void UpdateInfoPlugin::stopCheckForUpdates()
 {
-    if (!d->m_checkUpdatesCommand)
+    if (!d->m_maintenanceToolProcess)
         return;
 
-    d->m_collectedOutput.clear();
+    d->m_maintenanceToolProcess->disconnect();
-    d->m_checkUpdatesCommand->disconnect();
+    d->m_maintenanceToolProcess.reset();
-    d->m_checkUpdatesCommand->cancel();
+    d->m_updateOutput.clear();
-    d->m_checkUpdatesCommand = nullptr;
+    d->m_packagesOutput.clear();
     emit checkForUpdatesRunningChanged(false);
 }
 
-void UpdateInfoPlugin::collectCheckForUpdatesOutput(const QString &contents)
-{
-    d->m_collectedOutput += contents;
-}
-
 static void showUpdateInfo(const QList<Update> &updates, const std::function<void()> &startUpdater)
 {
     Utils::InfoBarEntry info(InstallUpdates,
@@ -235,14 +274,15 @@ void UpdateInfoPlugin::checkForUpdatesFinished()
 {
     setLastCheckDate(QDate::currentDate());
 
-    qCDebug(log) << "--- MaintenanceTool output (combined):";
+    qCDebug(log) << "--- MaintenanceTool output (updates):";
-    qCDebug(log) << qPrintable(d->m_collectedOutput);
+    qCDebug(log) << qPrintable(d->m_updateOutput);
-    std::unique_ptr<QDomDocument> document = documentForResponse(d->m_collectedOutput);
+    qCDebug(log) << "--- MaintenanceTool output (packages):";
+    qCDebug(log) << qPrintable(d->m_packagesOutput);
 
     stopCheckForUpdates();
 
-    const QList<Update> updates = availableUpdates(*document);
+    const QList<Update> updates = availableUpdates(d->m_updateOutput);
-    const QList<QtPackage> qtPackages = availableQtPackages(*document);
+    const QList<QtPackage> qtPackages = availableQtPackages(d->m_packagesOutput);
     if (log().isDebugEnabled()) {
         qCDebug(log) << "--- Available updates:";
         for (const Update &u : updates)
@@ -275,7 +315,7 @@ void UpdateInfoPlugin::checkForUpdatesFinished()
 
 bool UpdateInfoPlugin::isCheckForUpdatesRunning() const
 {
-    return d->m_checkUpdatesCommand;
+    return d->m_maintenanceToolProcess.get() != nullptr;
 }
 
 void UpdateInfoPlugin::extensionsInitialized()

src/plugins/updateinfo/updateinfoplugin.h

@@ -89,7 +89,6 @@ private:
     void startPackageManager();
     void stopCheckForUpdates();
 
-    void collectCheckForUpdatesOutput(const QString &contents);
     void checkForUpdatesFinished();
 
     void loadSettings() const;

src/plugins/updateinfo/updateinfotools.h

@@ -34,23 +34,8 @@
 #include <QRegularExpression>
 #include <QVersionNumber>
 
-#include <memory>
-
 Q_DECLARE_LOGGING_CATEGORY(log)
 
-std::unique_ptr<QDomDocument> documentForResponse(const QString &response)
-{
-    // since the output can contain two toplevel items from the two separate MaintenanceTool runs,
-    // clean up any <?xml version="1.0"?> and surround with a toplevel element
-    QString responseWithoutHeader = response;
-    responseWithoutHeader.remove(QRegularExpression("<\\?xml.*\\?>"));
-    const QString xml = response.isEmpty() ? QString()
-                                           : ("<doc>" + responseWithoutHeader + "</doc>");
-    std::unique_ptr<QDomDocument> doc(new QDomDocument);
-    doc->setContent(xml);
-    return doc;
-}
-
 struct Update
 {
     QString name;
@@ -62,8 +47,10 @@ struct Update
     };
 };
 
-QList<Update> availableUpdates(const QDomDocument &document)
+QList<Update> availableUpdates(const QString &updateXml)
 {
+    QDomDocument document;
+    document.setContent(updateXml);
     if (document.isNull() || !document.firstChildElement().hasChildNodes())
         return {};
     QList<Update> result;
@@ -93,8 +80,10 @@ struct QtPackage
     }
 };
 
-QList<QtPackage> availableQtPackages(const QDomDocument &document)
+QList<QtPackage> availableQtPackages(const QString &packageXml)
 {
+    QDomDocument document;
+    document.setContent(packageXml);
     if (document.isNull() || !document.firstChildElement().hasChildNodes())
         return {};
     QList<QtPackage> result;

tests/auto/updateinfo/tst_updateinfo.cpp

@@ -40,7 +40,8 @@ private slots:
 
 void tst_UpdateInfo::updates_data()
 {
-    QTest::addColumn<QString>("xml");
+    QTest::addColumn<QString>("updateXml");
+    QTest::addColumn<QString>("packageXml");
     QTest::addColumn<QList<Update>>("xupdates");
     QTest::addColumn<QList<QtPackage>>("xpackages");
 
@@ -48,8 +49,8 @@ void tst_UpdateInfo::updates_data()
         << R"raw(<?xml version="1.0"?>
         <updates>
         <update name="Qt Design Studio 3.2.0" version="3.2.0-0-202203291247" size="3113234690" id="qt.tools.qtdesignstudio"/>
-        </updates>
+        </updates>)raw"
-        <?xml version="1.0"?>
+        << R"raw(<?xml version="1.0"?>
         <availablepackages>
         <package name="qt.qt6.621" displayname="Qt 6.2.1" version="6.2.1-0-202110220854"/>
         <package name="qt.qt5.5152" displayname="Qt 5.15.2" version="5.15.2-0-202011130607" installedVersion="5.15.2-0-202011130607"/>
@@ -64,14 +65,14 @@ void tst_UpdateInfo::updates_data()
 
 void tst_UpdateInfo::updates()
 {
-    QFETCH(QString, xml);
+    QFETCH(QString, updateXml);
+    QFETCH(QString, packageXml);
     QFETCH(QList<Update>, xupdates);
     QFETCH(QList<QtPackage>, xpackages);
 
-    std::unique_ptr<QDomDocument> doc = documentForResponse(xml);
+    const QList<Update> updates = availableUpdates(updateXml);
-    const QList<Update> updates = availableUpdates(*doc);
     QCOMPARE(updates, xupdates);
-    const QList<QtPackage> packages = availableQtPackages(*doc);
+    const QList<QtPackage> packages = availableQtPackages(packageXml);
     QCOMPARE(packages, xpackages);
 }