react smarter on changed properties and added products

Instead of throwing away the whole build graph, we detect
changed properties in products and invalidate only those.
Also, adding products does not cause a full rebuild anymore.

Task-number: QBS-6
Task-number: QBS-235

Change-Id: I3ab1ca84eaeaf28bb083da4e6926e7b1ebbd7694
Reviewed-by: Christian Kandeler <christian.kandeler@digia.com>

6 files changed, 138 insertions, 37 deletions

diff --git a/src/lib/api/internaljobs.cpp b/src/lib/api/internaljobs.cpp
index 26e0f2ed4..e29919fe3 100644
--- a/src/lib/api/internaljobs.cpp
+++ b/src/lib/api/internaljobs.cpp
@@ -226,7 +226,7 @@ void InternalSetupProjectJob::execute()
         TimedActivityLogger resolveLogger(logger(), QLatin1String("Resolving build project"));
         m_buildProject = BuildProjectResolver(logger()).resolveProject(rProject, evalContext);
         if (loadResult.loadedProject)
-            m_buildProject->rescueDependencies(loadResult.loadedProject);
+            m_buildProject->rescueData(loadResult.loadedProject);
     }
 
     if (!m_parameters.dryRun)
diff --git a/src/lib/buildgraph/buildproject.cpp b/src/lib/buildgraph/buildproject.cpp
index 1027d4326..297dc513c 100644
--- a/src/lib/buildgraph/buildproject.cpp
+++ b/src/lib/buildgraph/buildproject.cpp
@@ -31,6 +31,7 @@
 #include "artifact.h"
 #include "buildgraph.h"
 #include "buildproduct.h"
+#include "command.h"
 #include "cycledetector.h"
 #include "rulesapplicator.h"
 #include "rulesevaluationcontext.h"
@@ -41,6 +42,7 @@
 #include <logging/translator.h>
 #include <tools/error.h>
 #include <tools/persistence.h>
+#include <tools/propertyfinder.h>
 #include <tools/setupprojectparameters.h>
 #include <tools/qbsassert.h>
 
@@ -145,22 +147,34 @@ void BuildProject::insertFileDependency(Artifact *artifact)
     insertIntoArtifactLookupTable(artifact);
 }
 
+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;
+}
+
 /**
- * Copies dependencies between artifacts from the other project to this project.
+ * Rescues the following data from the other project to this project:
+ *    - dependencies between artifacts,
+ *    - time stamps of artifacts, if their commands have not changed.
  */
-void BuildProject::rescueDependencies(const BuildProjectPtr &other)
+void BuildProject::rescueData(const BuildProjectConstPtr &other)
 {
-    QHash<QString, BuildProductPtr> otherProductsByName;
-    foreach (const BuildProductPtr &product, other->m_buildProducts)
+    QHash<QString, BuildProductConstPtr> otherProductsByName;
+    foreach (const BuildProductConstPtr &product, other->m_buildProducts)
         otherProductsByName.insert(product->rProduct->name, product);
 
     foreach (const BuildProductPtr &product, m_buildProducts) {
-        BuildProductPtr otherProduct = otherProductsByName.value(product->rProduct->name);
+        BuildProductConstPtr otherProduct = otherProductsByName.value(product->rProduct->name);
         if (!otherProduct)
             continue;
 
         if (m_logger.traceEnabled()) {
-            m_logger.qbsTrace() << QString::fromLocal8Bit("[BG] rescue dependencies of "
+            m_logger.qbsTrace() << QString::fromLocal8Bit("[BG] rescue data of "
                     "product '%1'").arg(product->rProduct->name);
         }
 
@@ -171,8 +185,19 @@ void BuildProject::rescueDependencies(const BuildProjectPtr &other)
             }
 
             Artifact *otherArtifact = otherProduct->lookupArtifact(artifact);
-            if (!otherArtifact || !otherArtifact->transformer)
+            if (!otherArtifact || !otherArtifact->transformer) {
+                if (m_logger.traceEnabled())
+                    m_logger.qbsTrace() << QString::fromLocal8Bit("[BG]    no transformer data");
                 continue;
+            }
+
+            if (artifact->transformer
+                    && !commandsEqual(artifact->transformer, otherArtifact->transformer)) {
+                if (m_logger.traceEnabled())
+                    m_logger.qbsTrace() << QString::fromLocal8Bit("[BG]    artifact invalidated");
+                continue;
+            }
+            artifact->timestamp = otherArtifact->timestamp;
 
             foreach (Artifact *otherChild, otherArtifact->children) {
                 // skip transform edges
@@ -317,6 +342,14 @@ BuildProjectPtr BuildProjectResolver::resolveProject(const ResolvedProjectPtr &r
     return m_project;
 }
 
+void BuildProjectResolver::resolveProductsForExistingProject(const BuildProjectPtr &project,
+        const QList<ResolvedProductPtr> &freshProducts)
+{
+    m_project = project;
+    foreach (const ResolvedProductPtr &product, freshProducts)
+        resolveProduct(product);
+}
+
 static void addTargetArtifacts(const BuildProductPtr &product,
                               ArtifactsPerFileTagMap &artifactsPerFileTag, const Logger &logger)
 {
@@ -516,12 +549,11 @@ BuildProjectLoader::LoadResult BuildProjectLoader::load(const SetupProjectParame
     project->resolvedProject()->buildDirectory = buildDir;
     m_result.loadedProject = project;
     loadLogger.finishActivity();
-    trackProjectChanges(parameters, evalContext, buildGraphFilePath, project);
+    trackProjectChanges(parameters, buildGraphFilePath, project);
     return m_result;
 }
 
-void BuildProjectLoader::trackProjectChanges(const SetupProjectParameters &parameters,
-        const RulesEvaluationContextPtr &evalContext, const QString &buildGraphFilePath,
+void BuildProjectLoader::trackProjectChanges(const SetupProjectParameters &parameters, const QString &buildGraphFilePath,
         const BuildProjectPtr &restoredProject)
 {
     const FileInfo bgfi(buildGraphFilePath);
@@ -530,7 +562,7 @@ void BuildProjectLoader::trackProjectChanges(const SetupProjectParameters &param
 
     bool referencedProductRemoved = false;
     QList<BuildProductPtr> changedProducts;
-    foreach (BuildProductPtr product, restoredProject->buildProducts()) {
+    foreach (const BuildProductPtr &product, restoredProject->buildProducts()) {
         const ResolvedProductConstPtr &resolvedProduct = product->rProduct;
         const FileInfo pfi(resolvedProduct->location.fileName);
         if (!pfi.exists()) {
@@ -557,16 +589,29 @@ void BuildProjectLoader::trackProjectChanges(const SetupProjectParameters &param
     if (!projectFileChanged && !referencedProductRemoved && changedProducts.isEmpty())
         return;
 
-    Loader ldr(evalContext->engine(), m_logger);
+    Loader ldr(m_evalContext->engine(), m_logger);
     ldr.setSearchPaths(parameters.searchPaths);
     const ResolvedProjectPtr freshProject = ldr.loadProject(parameters);
     m_result.changedResolvedProject = freshProject;
 
     QMap<QString, ResolvedProductPtr> freshProductsByName;
-    if (!changedProducts.isEmpty()) {
-        foreach (const ResolvedProductPtr &cp, freshProject->products)
-            freshProductsByName.insert(cp->name, cp);
+    foreach (const ResolvedProductPtr &cp, freshProject->products)
+        freshProductsByName.insert(cp->name, cp);
+
+    QSet<TransformerPtr> seenTransformers;
+    foreach (const BuildProductPtr &product, restoredProject->buildProducts()) {
+        foreach (Artifact *artifact, product->artifacts) {
+            if (!artifact->transformer || seenTransformers.contains(artifact->transformer))
+                continue;
+            seenTransformers.insert(artifact->transformer);
+            ResolvedProductPtr freshProduct = freshProductsByName.value(product->rProduct->name);
+            if (freshProduct && checkForPropertyChanges(artifact->transformer, freshProduct)) {
+                m_result.discardLoadedProject = true;
+                return;
+            }
+        }
     }
+
     foreach (const BuildProductPtr &product, changedProducts) {
         ResolvedProductPtr freshProduct = freshProductsByName.value(product->rProduct->name);
         if (!freshProduct)
@@ -581,13 +626,7 @@ void BuildProjectLoader::trackProjectChanges(const SetupProjectParameters &param
         oldProductNames += product->name;
     foreach (const ResolvedProductConstPtr &product, freshProject->products)
         newProductNames += product->name;
-    const QSet<QString> addedProductNames = newProductNames - oldProductNames;
-    if (!addedProductNames.isEmpty()) {
-        // TODO: apply rules for the new products
-        m_logger.qbsDebug() << "New products were added, discarding the loaded project.";
-        m_result.discardLoadedProject = true;
-        return;
-    }
+
     const QSet<QString> removedProductsNames = oldProductNames - newProductNames;
     if (!removedProductsNames.isEmpty()) {
         foreach (const BuildProductPtr &product, restoredProject->buildProducts()) {
@@ -596,6 +635,23 @@ void BuildProjectLoader::trackProjectChanges(const SetupProjectParameters &param
         }
     }
 
+    const QSet<QString> addedProductNames = newProductNames - oldProductNames;
+    QList<ResolvedProductPtr> addedProducts;
+    foreach (const QString &productName, addedProductNames) {
+        const ResolvedProductPtr &freshProduct = freshProductsByName.value(productName);
+        QBS_ASSERT(freshProduct, continue);
+        addedProducts.append(freshProduct);
+    }
+    if (!addedProducts.isEmpty()) {
+        ResolvedProjectPtr restoredResolvedProject = restoredProject->resolvedProject();
+        foreach (const ResolvedProductPtr &product, addedProducts) {
+            product->project = restoredResolvedProject;
+            restoredResolvedProject->products.append(product);
+        }
+        BuildProjectResolver bpr(m_logger);
+        bpr.resolveProductsForExistingProject(restoredProject, addedProducts);
+    }
+
     CycleDetector(m_logger).visitProject(restoredProject);
 }
 
@@ -648,15 +704,6 @@ void BuildProjectLoader::onProductChanged(const BuildProductPtr &product,
             removeArtifactAndExclusiveDependents(artifact, &artifactsToRemove);
             continue;
         }
-        if (!addedSourceArtifacts.contains(changedArtifact)
-            && changedArtifact->properties->value() != a->properties->value())
-        {
-            m_logger.qbsInfo() << Tr::tr("Some properties changed. Regenerating build graph.");
-            m_logger.qbsDebug() << "Artifact with changed properties: "
-                                << changedArtifact->absoluteFilePath;
-            m_result.discardLoadedProject = true;
-            return;
-        }
         if (changedArtifact->fileTags != a->fileTags) {
             // artifact's filetags have changed
             m_logger.qbsDebug() << "[BG] filetags have changed for artifact '"
@@ -733,5 +780,24 @@ void BuildProjectLoader::removeArtifactAndExclusiveDependents(Artifact *artifact
     artifact->project->removeArtifact(artifact);
 }
 
+bool BuildProjectLoader::checkForPropertyChanges(const TransformerPtr &restoredTrafo,
+        const ResolvedProductPtr &freshProduct)
+{
+    PropertyFinder finder;
+    foreach (const Property &property, restoredTrafo->modulePropertiesUsedInPrepareScript) {
+        QVariant v;
+        if (property.value.type() == QVariant::List) {
+            v = finder.propertyValues(freshProduct->properties->value(), property.moduleName,
+                                           property.propertyName);
+        } else {
+            v = finder.propertyValue(freshProduct->properties->value(), property.moduleName,
+                                           property.propertyName);
+        }
+        if (property.value != v)
+            return true;
+    }
+    return false;
+}
+
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/buildgraph/buildproject.h b/src/lib/buildgraph/buildproject.h
index 37610b6d5..76fa3439f 100644
--- a/src/lib/buildgraph/buildproject.h
+++ b/src/lib/buildgraph/buildproject.h
@@ -76,7 +76,7 @@ public:
     QList<Artifact *> lookupArtifacts(const QString &dirPath, const QString &fileName) const;
     QList<Artifact *> lookupArtifacts(const Artifact *artifact) const;
     void insertFileDependency(Artifact *artifact);
-    void rescueDependencies(const BuildProjectPtr &other);
+    void rescueData(const BuildProjectConstPtr &other);
     void removeArtifact(Artifact *artifact);
     void updateNodesThatMustGetNewTransformer();
     void removeFromArtifactsThatMustGetNewTransformers(Artifact *a) {
@@ -111,10 +111,11 @@ public:
     BuildProjectResolver(const Logger &logger);
     BuildProjectPtr resolveProject(const ResolvedProjectPtr &resolvedProject,
                                    const RulesEvaluationContextPtr &evalContext);
+    void resolveProductsForExistingProject(const BuildProjectPtr &project,
+        const QList<ResolvedProductPtr> &freshProducts);
 
 private:
     BuildProductPtr resolveProduct(const ResolvedProductPtr &rProduct);
-
     RulesEvaluationContextPtr evalContext() const;
     ScriptEngine *engine() const;
     QScriptValue scope() const;
@@ -145,7 +146,6 @@ public:
 
 private:
     void trackProjectChanges(const SetupProjectParameters &parameters,
-                             const RulesEvaluationContextPtr &evalContext,
                              const QString &buildGraphFilePath,
                              const BuildProjectPtr &restoredProject);
     void onProductRemoved(const BuildProductPtr &product);
@@ -153,6 +153,7 @@ private:
                           const ResolvedProductPtr &changedProduct);
     void removeArtifactAndExclusiveDependents(Artifact *artifact,
                                               QList<Artifact*> *removedArtifacts = 0);
+    bool checkForPropertyChanges(const TransformerPtr &restoredTrafo, const ResolvedProductPtr &freshProduct);
 
     RulesEvaluationContextPtr m_evalContext;
     LoadResult m_result;
diff --git a/src/lib/buildgraph/command.cpp b/src/lib/buildgraph/command.cpp
index 372d4b469..64b4c57e3 100644
--- a/src/lib/buildgraph/command.cpp
+++ b/src/lib/buildgraph/command.cpp
@@ -60,6 +60,12 @@ AbstractCommand *AbstractCommand::createByType(AbstractCommand::CommandType comm
     return 0;
 }
 
+bool AbstractCommand::equals(const AbstractCommand *other) const
+{
+    return m_description == other->m_description && m_highlight == other->m_highlight
+            && m_silent == other->m_silent && type() == other->type();
+}
+
 void AbstractCommand::fillFromScriptValue(const QScriptValue *scriptValue, const CodeLocation &codeLocation)
 {
     Q_UNUSED(codeLocation);
@@ -143,6 +149,22 @@ void ProcessCommand::getEnvironmentFromList(const QStringList &envList)
     }
 }
 
+bool ProcessCommand::equals(const AbstractCommand *otherAbstractCommand) const
+{
+    if (!AbstractCommand::equals(otherAbstractCommand))
+        return false;
+    const ProcessCommand * const other = static_cast<const ProcessCommand *>(otherAbstractCommand);
+    return m_program == other->m_program
+            && m_arguments == other->m_arguments
+            && m_workingDir == other->m_workingDir
+            && m_maxExitCode == other->m_maxExitCode
+            && m_stdoutFilterFunction == other->m_stdoutFilterFunction
+            && m_stderrFilterFunction == other->m_stderrFilterFunction
+            && m_responseFileThreshold == other->m_responseFileThreshold
+            && m_responseFileUsagePrefix == other->m_responseFileUsagePrefix
+            && m_environment == other->m_environment;
+}
+
 void ProcessCommand::fillFromScriptValue(const QScriptValue *scriptValue, const CodeLocation &codeLocation)
 {
     AbstractCommand::fillFromScriptValue(scriptValue, codeLocation);
@@ -214,6 +236,17 @@ JavaScriptCommand::JavaScriptCommand()
 {
 }
 
+bool JavaScriptCommand::equals(const AbstractCommand *otherAbstractCommand) const
+{
+    if (!AbstractCommand::equals(otherAbstractCommand))
+        return false;
+    const JavaScriptCommand * const other
+            = static_cast<const JavaScriptCommand *>(otherAbstractCommand);
+    return m_sourceCode == other->m_sourceCode
+            && m_properties == other->m_properties
+            && m_codeLocation == other->m_codeLocation;
+}
+
 void JavaScriptCommand::fillFromScriptValue(const QScriptValue *scriptValue, const CodeLocation &codeLocation)
 {
     m_codeLocation = codeLocation;
diff --git a/src/lib/buildgraph/command.h b/src/lib/buildgraph/command.h
index d59783f0f..c9add47c2 100644
--- a/src/lib/buildgraph/command.h
+++ b/src/lib/buildgraph/command.h
@@ -56,6 +56,7 @@ public:
     static bool defaultIsSilent() { return true; }
 
     virtual CommandType type() const = 0;
+    virtual bool equals(const AbstractCommand *other) const;
     virtual void fillFromScriptValue(const QScriptValue *scriptValue, const CodeLocation &codeLocation);
     virtual void load(QDataStream &s);
     virtual void store(QDataStream &s);
@@ -86,6 +87,7 @@ public:
     ProcessCommand();
 
     CommandType type() const { return ProcessCommandType; }
+    bool equals(const AbstractCommand *otherAbstractCommand) const;
     void fillFromScriptValue(const QScriptValue *scriptValue, const CodeLocation &codeLocation);
     void load(QDataStream &s);
     void store(QDataStream &s);
@@ -138,6 +140,7 @@ public:
     JavaScriptCommand();
 
     virtual CommandType type() const { return JavaScriptCommandType; }
+    bool equals(const AbstractCommand *otherAbstractCommand) const;
     void fillFromScriptValue(const QScriptValue *scriptValue, const CodeLocation &codeLocation);
     void load(QDataStream &s);
     void store(QDataStream &s);
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index e321fe3ac..40aa2c8af 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -648,7 +648,6 @@ void TestBlackbox::trackAddProduct()
     QCOMPARE(runQbs(args), 0);
     QVERIFY(m_qbsStdout.contains("compiling zoo.cpp"));
     QVERIFY(m_qbsStdout.contains("linking product3"));
-    QEXPECT_FAIL("", "QBS-235", Abort);
     QVERIFY(!m_qbsStdout.contains("compiling foo.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling bar.cpp"));
     QVERIFY(!m_qbsStdout.contains("linking product1"));
@@ -798,7 +797,6 @@ void TestBlackbox::propertyChanges()
     projectFile.close();
     QCOMPARE(runQbs(), 0);
     QVERIFY(m_qbsStdout.contains("compiling source1.cpp"));
-    QEXPECT_FAIL("", "FIXME: implement fine-grained checks for property changes", Abort);
     QVERIFY(!m_qbsStdout.contains("compiling source2.cpp"));
 
     // Incremental build, input property changed indirectly for second build.