Better handling of property changes when restoring a build graph.

If a property change is discovered in any given product, the current
code throws away the whole build graph, generates a new one and then re-
inserts selected data from the old one. With this patch, we only
regenerate the build data of the affected product (and still re-insert
some existing data into it as to not rebuild artifacts that are up to
date).

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

6 files changed, 137 insertions, 112 deletions

diff --git a/src/lib/buildgraph/buildgraph.cpp b/src/lib/buildgraph/buildgraph.cpp
index 4be34c644..9509a2f9b 100644
--- a/src/lib/buildgraph/buildgraph.cpp
+++ b/src/lib/buildgraph/buildgraph.cpp
@@ -315,11 +315,11 @@ QString relativeArtifactFileName(const Artifact *artifact)
     return str;
 }
 
-Artifact *lookupArtifact(const ResolvedProductConstPtr &product, const QString &dirPath,
-                         const QString &fileName)
+Artifact *lookupArtifact(const ResolvedProductConstPtr &product,
+        const ProjectBuildData *projectBuildData, const QString &dirPath, const QString &fileName)
 {
     const QList<FileResourceBase *> lookupResults
-            = product->topLevelProject()->buildData->lookupFiles(dirPath, fileName);
+            = projectBuildData->lookupFiles(dirPath, fileName);
     for (QList<FileResourceBase *>::const_iterator it = lookupResults.constBegin();
             it != lookupResults.constEnd(); ++it) {
         Artifact *artifact = dynamic_cast<Artifact *>(*it);
@@ -329,6 +329,12 @@ Artifact *lookupArtifact(const ResolvedProductConstPtr &product, const QString &
     return 0;
 }
 
+Artifact *lookupArtifact(const ResolvedProductConstPtr &product, const QString &dirPath,
+                         const QString &fileName)
+{
+    return lookupArtifact(product, product->topLevelProject()->buildData.data(), dirPath, fileName);
+}
+
 Artifact *lookupArtifact(const ResolvedProductConstPtr &product, const QString &filePath)
 {
     QString dirPath, fileName;
diff --git a/src/lib/buildgraph/buildgraph.h b/src/lib/buildgraph/buildgraph.h
index ac2a0f313..d1f8cd477 100644
--- a/src/lib/buildgraph/buildgraph.h
+++ b/src/lib/buildgraph/buildgraph.h
@@ -43,6 +43,10 @@ class Logger;
 class ScriptEngine;
 class ScriptPropertyObserver;
 
+
+Artifact *lookupArtifact(const ResolvedProductConstPtr &product,
+                         const ProjectBuildData *projectBuildData,
+                         const QString &dirPath, const QString &fileName);
 Artifact *lookupArtifact(const ResolvedProductConstPtr &product, const QString &dirPath,
                          const QString &fileName);
 Artifact *lookupArtifact(const ResolvedProductConstPtr &product, const QString &filePath);
diff --git a/src/lib/buildgraph/buildgraphloader.cpp b/src/lib/buildgraph/buildgraphloader.cpp
index 80513251a..4466d381e 100644
--- a/src/lib/buildgraph/buildgraphloader.cpp
+++ b/src/lib/buildgraph/buildgraphloader.cpp
@@ -31,6 +31,7 @@
 #include "artifact.h"
 #include "artifactlist.h"
 #include "buildgraph.h"
+#include "command.h"
 #include "cycledetector.h"
 #include "productbuilddata.h"
 #include "projectbuilddata.h"
@@ -195,27 +196,13 @@ void BuildGraphLoader::trackProjectChanges(const SetupProjectParameters &paramet
     foreach (const ResolvedProductPtr &cp, allNewlyResolvedProducts)
         freshProductsByName.insert(cp->name, cp);
 
-    QSet<TransformerPtr> seenTransformers;
-    foreach (const ResolvedProductPtr &product, allRestoredProducts) {
-        if (!product->buildData)
-            continue;
-        foreach (Artifact *artifact, product->buildData->artifacts) {
-            if (!artifact->transformer || seenTransformers.contains(artifact->transformer))
-                continue;
-            seenTransformers.insert(artifact->transformer);
-            ResolvedProductPtr freshProduct = freshProductsByName.value(product->name);
-            if (freshProduct && checkForPropertyChanges(artifact->transformer, freshProduct)) {
-                m_logger.qbsDebug() << "Cannot re-use build graph due to property changes "
-                                       "in product '" << freshProduct->name << "'.";
-                m_result.discardLoadedProject = true;
-                return;
-            }
-        }
-    }
-
     checkAllProductsForChanges(allRestoredProducts, freshProductsByName, changedProducts,
                                productsWithChangedFiles);
 
+    QSharedPointer<ProjectBuildData> oldBuildData;
+    if (!changedProducts.isEmpty())
+        oldBuildData.reset(new ProjectBuildData(restoredProject->buildData.data()));
+
     // For products with "serious" changes such as different prepare scripts, we set up the
     // build data from scratch to be on the safe side. This can be made more fine-grained
     // if needed.
@@ -223,7 +210,7 @@ void BuildGraphLoader::trackProjectChanges(const SetupProjectParameters &paramet
         ResolvedProductPtr freshProduct = freshProductsByName.value(product->name);
         if (!freshProduct)
             continue;
-        onProductRemoved(product, product->topLevelProject()->buildData.data());
+        onProductRemoved(product, product->topLevelProject()->buildData.data(), false);
         allRestoredProducts.removeOne(product);
         productsWithChangedFiles.removeOne(product);
     }
@@ -274,6 +261,11 @@ void BuildGraphLoader::trackProjectChanges(const SetupProjectParameters &paramet
     foreach (const ResolvedProductPtr &removedProduct, allRestoredProducts)
         onProductRemoved(removedProduct, m_result.newlyResolvedProject->buildData.data());
 
+    foreach (const ResolvedProductConstPtr &changedProduct, changedProducts) {
+        rescueOldBuildData(changedProduct, freshProductsByName.value(changedProduct->name),
+                           oldBuildData.data());
+    }
+
     CycleDetector(m_logger).visitProject(m_result.newlyResolvedProject);
 }
 
@@ -383,21 +375,42 @@ bool BuildGraphLoader::checkProductForChanges(const ResolvedProductPtr &restored
                                               const ResolvedProductPtr &newlyResolvedProduct)
 {
     return !transformerListsAreEqual(restoredProduct->transformers,
-                                     newlyResolvedProduct->transformers);
+                                     newlyResolvedProduct->transformers)
+            || checkForPropertyChanges(restoredProduct, newlyResolvedProduct);
     // TODO: Check for more stuff.
 }
 
+bool BuildGraphLoader::checkForPropertyChanges(const ResolvedProductPtr &restoredProduct,
+                                               const ResolvedProductPtr &newlyResolvedProduct)
+{
+    QSet<TransformerPtr> seenTransformers;
+    if (!restoredProduct->buildData)
+        return false;
+    foreach (Artifact * const artifact, restoredProduct->buildData->artifacts) {
+        if (!artifact->transformer || seenTransformers.contains(artifact->transformer))
+            continue;
+        seenTransformers.insert(artifact->transformer);
+        if (checkForPropertyChanges(artifact->transformer, newlyResolvedProduct)) {
+                m_logger.qbsDebug() << "Property changes in product '"
+                                    << newlyResolvedProduct->name << "'.";
+                return true;
+        }
+    }
+    return false;
+}
+
 void BuildGraphLoader::onProductRemoved(const ResolvedProductPtr &product,
-                                        ProjectBuildData *projectBuildData)
+        ProjectBuildData *projectBuildData, bool removeArtifactsFromDisk)
 {
     m_logger.qbsDebug() << "[BG] product '" << product->name << "' removed.";
 
     product->project->products.removeOne(product);
 
-    // delete all removed artifacts physically from the disk
     if (product->buildData) {
-        foreach (Artifact *artifact, product->buildData->artifacts)
-            projectBuildData->removeArtifact(artifact, projectBuildData, m_logger);
+        foreach (Artifact *artifact, product->buildData->artifacts) {
+            projectBuildData->removeArtifact(artifact, projectBuildData, m_logger,
+                                             removeArtifactsFromDisk);
+        }
     }
 }
 
@@ -589,6 +602,71 @@ void BuildGraphLoader::replaceFileDependencyWithArtifact(const ResolvedProductPt
     delete filedep;
 }
 
+static bool commandsEqual(const TransformerConstPtr &t1, const TransformerConstPtr &t2)
+{
+    if (t1->commands.count() != t2->commands.count())
+        return false;
+    for (int i = 0; i < t1->commands.count(); ++i)
+        if (!t1->commands.at(i)->equals(t2->commands.at(i)))
+            return false;
+    return true;
+}
+
+/**
+ * Rescues the following data from the restoredProduct to newlyResolvedProduct:
+ *    - dependencies between artifacts,
+ *    - time stamps of artifacts, if their commands have not changed.
+ */
+void BuildGraphLoader::rescueOldBuildData(const ResolvedProductConstPtr &restoredProduct,
+        const ResolvedProductPtr &newlyResolvedProduct,
+        const ProjectBuildData *oldBuildData)
+{
+    if (!restoredProduct->enabled || !newlyResolvedProduct->enabled)
+        return;
+
+    if (m_logger.traceEnabled()) {
+        m_logger.qbsTrace() << QString::fromLocal8Bit("[BG] rescue data of "
+                                                      "product '%1'").arg(restoredProduct->name);
+    }
+
+    foreach (Artifact *artifact, newlyResolvedProduct->buildData->artifacts) {
+        if (m_logger.traceEnabled()) {
+            m_logger.qbsTrace() << QString::fromLocal8Bit("[BG]    artifact '%1'")
+                                   .arg(artifact->fileName());
+        }
+
+        Artifact * const oldArtifact = lookupArtifact(restoredProduct, oldBuildData,
+                                                      artifact->dirPath(), artifact->fileName());
+        if (!oldArtifact || !oldArtifact->transformer) {
+            if (m_logger.traceEnabled())
+                m_logger.qbsTrace() << QString::fromLocal8Bit("[BG]    no transformer data");
+            continue;
+        }
+
+        if (artifact->transformer
+                && !commandsEqual(artifact->transformer, oldArtifact->transformer)) {
+            if (m_logger.traceEnabled())
+                m_logger.qbsTrace() << QString::fromLocal8Bit("[BG]    artifact invalidated");
+            removeGeneratedArtifactFromDisk(oldArtifact, m_logger);
+            continue;
+        }
+        artifact->setTimestamp(oldArtifact->timestamp());
+
+        foreach (Artifact * const oldChild, oldArtifact->children) {
+            // skip transform edges
+            if (oldArtifact->transformer->inputs.contains(oldChild))
+                continue;
+
+            foreach (FileResourceBase *childFileRes,
+                     newlyResolvedProduct->topLevelProject()->buildData->lookupFiles(oldChild)) {
+                Artifact * const child = dynamic_cast<Artifact *>(childFileRes);
+                if (child && !artifact->children.contains(child))
+                    safeConnect(artifact, child, m_logger);
+            }
+        }
+    }
+}
+
 void addTargetArtifacts(const ResolvedProductPtr &product,
                         ArtifactsPerFileTagMap &artifactsPerFileTag, const Logger &logger)
 {
diff --git a/src/lib/buildgraph/buildgraphloader.h b/src/lib/buildgraph/buildgraphloader.h
index 0c6f1d5fe..3086a0217 100644
--- a/src/lib/buildgraph/buildgraphloader.h
+++ b/src/lib/buildgraph/buildgraphloader.h
@@ -47,11 +47,8 @@ class FileTime;
 class BuildGraphLoadResult
 {
 public:
-    BuildGraphLoadResult() : discardLoadedProject(false) {}
-
     TopLevelProjectPtr newlyResolvedProject;
     TopLevelProjectPtr loadedProject;
-    bool discardLoadedProject;
 };
 
 
@@ -81,7 +78,10 @@ private:
             QList<ResolvedProductPtr> &productsWithChangedFiles);
     bool checkProductForChanges(const ResolvedProductPtr &restoredProduct,
                                 const ResolvedProductPtr &newlyResolvedProduct);
-    void onProductRemoved(const ResolvedProductPtr &product, ProjectBuildData *projectBuildData);
+    bool checkForPropertyChanges(const ResolvedProductPtr &restoredProduct,
+                                 const ResolvedProductPtr &newlyResolvedProduct);
+    void onProductRemoved(const ResolvedProductPtr &product, ProjectBuildData *projectBuildData,
+                          bool removeArtifactsFromDisk = true);
     void onProductFileListChanged(const ResolvedProductPtr &product,
                           const ResolvedProductPtr &changedProduct);
     void removeArtifactAndExclusiveDependents(Artifact *artifact,
@@ -89,6 +89,9 @@ private:
     bool checkForPropertyChanges(const TransformerPtr &restoredTrafo, const ResolvedProductPtr &freshProduct);
     void replaceFileDependencyWithArtifact(const ResolvedProductPtr &fileDepProduct,
             FileDependency *filedep, Artifact *artifact);
+    void rescueOldBuildData(const ResolvedProductConstPtr &restoredProduct,
+                            const ResolvedProductPtr &newlyResolvedProduct,
+                            const ProjectBuildData *oldBuildData);
 
     RulesEvaluationContextPtr m_evalContext;
     BuildGraphLoadResult m_result;
diff --git a/src/lib/buildgraph/projectbuilddata.cpp b/src/lib/buildgraph/projectbuilddata.cpp
index dea04669f..73478f25d 100644
--- a/src/lib/buildgraph/projectbuilddata.cpp
+++ b/src/lib/buildgraph/projectbuilddata.cpp
@@ -46,12 +46,20 @@
 namespace qbs {
 namespace Internal {
 
-ProjectBuildData::ProjectBuildData() : isDirty(true)
+ProjectBuildData::ProjectBuildData(const ProjectBuildData *other)
+    : isDirty(true), m_doCleanupInDestructor(true)
 {
+    // This is needed for temporary duplication of build data when doing change tracking.
+    if (other) {
+        *this = *other;
+        m_doCleanupInDestructor = false;
+    }
 }
 
 ProjectBuildData::~ProjectBuildData()
 {
+    if (!m_doCleanupInDestructor)
+        return;
     qDeleteAll(fileDependencies);
 }
 
@@ -97,16 +105,6 @@ void ProjectBuildData::insertFileDependency(FileDependency *dependency)
     insertIntoLookupTable(dependency);
 }
 
-static bool commandsEqual(const TransformerConstPtr &t1, const TransformerConstPtr &t2)
-{
-    if (t1->commands.count() != t2->commands.count())
-        return false;
-    for (int i = 0; i < t1->commands.count(); ++i)
-        if (!t1->commands.at(i)->equals(t2->commands.at(i)))
-            return false;
-    return true;
-}
-
 static void disconnectArtifactChildren(Artifact *artifact, const Logger &logger)
 {
     if (logger.traceEnabled()) {
@@ -144,12 +142,13 @@ static void disconnectArtifact(Artifact *artifact, ProjectBuildData *projectBuil
 }
 
 void ProjectBuildData::removeArtifact(Artifact *artifact, ProjectBuildData *projectBuildData,
-                                      const Logger &logger)
+                                      const Logger &logger, bool removeFromDisk)
 {
     if (logger.traceEnabled())
         logger.qbsTrace() << "[BG] remove artifact " << relativeArtifactFileName(artifact);
 
-    removeGeneratedArtifactFromDisk(artifact, logger);
+    if (removeFromDisk)
+        removeGeneratedArtifactFromDisk(artifact, logger);
     artifact->product->buildData->artifacts.remove(artifact);
     removeFromLookupTable(artifact);
     artifact->product->buildData->targetArtifacts.remove(artifact);
@@ -246,69 +245,6 @@ void BuildDataResolver::resolveProductBuildDataForExistingProject(const TopLevel
         resolveProductBuildData(product);
 }
 
-/**
- * Rescues the following data from the source to target:
- *    - dependencies between artifacts,
- *    - time stamps of artifacts, if their commands have not changed.
- */
-void BuildDataResolver::rescueBuildData(const TopLevelProjectConstPtr &source,
-                                      const TopLevelProjectPtr &target, Logger logger)
-{
-    QHash<QString, ResolvedProductConstPtr> sourceProductsByName;
-    foreach (const ResolvedProductConstPtr &product, source->allProducts())
-        sourceProductsByName.insert(product->name, product);
-
-    foreach (const ResolvedProductPtr &product, target->allProducts()) {
-        ResolvedProductConstPtr sourceProduct = sourceProductsByName.value(product->name);
-        if (!sourceProduct)
-            continue;
-
-        if (!product->enabled || !sourceProduct->enabled)
-            continue;
-        QBS_CHECK(product->buildData);
-
-        if (logger.traceEnabled()) {
-            logger.qbsTrace() << QString::fromLocal8Bit("[BG] rescue data of "
-                    "product '%1'").arg(product->name);
-        }
-
-        foreach (Artifact *artifact, product->buildData->artifacts) {
-            if (logger.traceEnabled()) {
-                logger.qbsTrace() << QString::fromLocal8Bit("[BG]    artifact '%1'")
-                                       .arg(artifact->fileName());
-            }
-
-            Artifact *otherArtifact = lookupArtifact(sourceProduct, artifact);
-            if (!otherArtifact || !otherArtifact->transformer) {
-                if (logger.traceEnabled())
-                    logger.qbsTrace() << QString::fromLocal8Bit("[BG]    no transformer data");
-                continue;
-            }
-
-            if (artifact->transformer
-                    && !commandsEqual(artifact->transformer, otherArtifact->transformer)) {
-                if (logger.traceEnabled())
-                    logger.qbsTrace() << QString::fromLocal8Bit("[BG]    artifact invalidated");
-                continue;
-            }
-            artifact->setTimestamp(otherArtifact->timestamp());
-
-            foreach (Artifact *otherChild, otherArtifact->children) {
-                // skip transform edges
-                if (otherArtifact->transformer->inputs.contains(otherChild))
-                    continue;
-
-                foreach (FileResourceBase *childFileRes,
-                         target->buildData->lookupFiles(otherChild)) {
-                    Artifact *child = dynamic_cast<Artifact *>(childFileRes);
-                    if (child && !artifact->children.contains(child))
-                        safeConnect(artifact, child, logger);
-                }
-            }
-        }
-    }
-}
-
 void BuildDataResolver::resolveProductBuildData(const ResolvedProductPtr &product)
 {
     if (product->buildData)
diff --git a/src/lib/buildgraph/projectbuilddata.h b/src/lib/buildgraph/projectbuilddata.h
index c123d2ca6..bf1496a4a 100644
--- a/src/lib/buildgraph/projectbuilddata.h
+++ b/src/lib/buildgraph/projectbuilddata.h
@@ -50,7 +50,7 @@ class ScriptEngine;
 class ProjectBuildData : public PersistentObject
 {
 public:
-    ProjectBuildData();
+    ProjectBuildData(const ProjectBuildData *other = 0);
     ~ProjectBuildData();
 
     static QString deriveBuildGraphFilePath(const QString &buildDir, const QString &projectId);
@@ -65,7 +65,7 @@ public:
     void updateNodesThatMustGetNewTransformer(const Logger &logger);
     void removeArtifact(Artifact *artifact, const Logger &logger);
     void removeArtifact(Artifact *artifact, ProjectBuildData *projectBuildData,
-                        const Logger &logger);
+                        const Logger &logger, bool removeFromDisk = true);
 
     QSet<FileDependency *> fileDependencies;
     RulesEvaluationContextPtr evaluationContext;
@@ -80,6 +80,7 @@ private:
     typedef QHash<QString, QList<FileResourceBase *> > ResultsPerDirectory;
     typedef QHash<QString, ResultsPerDirectory> ArtifactLookupTable;
     ArtifactLookupTable m_artifactLookupTable;
+    bool m_doCleanupInDestructor;
 };
 
 
@@ -92,9 +93,6 @@ public:
     void resolveProductBuildDataForExistingProject(const TopLevelProjectPtr &project,
             const QList<ResolvedProductPtr> &freshProducts);
 
-    static void rescueBuildData(const TopLevelProjectConstPtr &source,
-                                const TopLevelProjectPtr &target, Logger logger);
-
 private:
     void resolveProductBuildData(const ResolvedProductPtr &product);
     RulesEvaluationContextPtr evalContext() const;