Handle changes in transformers when re-using a stored build graph.

Also fixes a bug when comparing file lists in products and sets up the
infrastructure for additional checks.

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

7 files changed, 206 insertions, 21 deletions

diff --git a/src/lib/buildgraph/buildgraphloader.cpp b/src/lib/buildgraph/buildgraphloader.cpp
index 67d645a44..80513251a 100644
--- a/src/lib/buildgraph/buildgraphloader.cpp
+++ b/src/lib/buildgraph/buildgraphloader.cpp
@@ -166,8 +166,9 @@ void BuildGraphLoader::trackProjectChanges(const SetupProjectParameters &paramet
     QSet<QString> buildSystemFiles = restoredProject->buildSystemFiles;
     QList<ResolvedProductPtr> allRestoredProducts = restoredProject->allProducts();
     QList<ResolvedProductPtr> changedProducts;
+    QList<ResolvedProductPtr> productsWithChangedFiles;
     bool reResolvingNecessary = hasProductFileChanged(allRestoredProducts, buildGraphTimeStamp,
-                                                      buildSystemFiles, changedProducts);
+                                                      buildSystemFiles, productsWithChangedFiles);
 
     // "External" changes, e.g. in the environment or in a JavaScript file,
     // can make the list of source files in a product change without the respective file
@@ -179,8 +180,8 @@ void BuildGraphLoader::trackProjectChanges(const SetupProjectParameters &paramet
         reResolvingNecessary = true;
     }
 
-   if (!reResolvingNecessary)
-       return;
+    if (!reResolvingNecessary)
+        return;
 
     restoredProject->buildData->isDirty = true;
     Loader ldr(m_evalContext->engine(), m_logger);
@@ -212,14 +213,28 @@ void BuildGraphLoader::trackProjectChanges(const SetupProjectParameters &paramet
         }
     }
 
-    checkAllProductsForChanges(allRestoredProducts, freshProductsByName, changedProducts);
+    checkAllProductsForChanges(allRestoredProducts, freshProductsByName, changedProducts,
+                               productsWithChangedFiles);
+
+    // 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.
     foreach (const ResolvedProductPtr &product, changedProducts) {
         ResolvedProductPtr freshProduct = freshProductsByName.value(product->name);
         if (!freshProduct)
             continue;
-        onProductChanged(product, freshProduct);
-        if (m_result.discardLoadedProject)
-            return;
+        onProductRemoved(product, product->topLevelProject()->buildData.data());
+        allRestoredProducts.removeOne(product);
+        productsWithChangedFiles.removeOne(product);
+    }
+
+    // For products where only the list of files has changed, we adapt the existing build data
+    // so we won't recompile existing files just because new ones have been added.
+    foreach (const ResolvedProductPtr &product, productsWithChangedFiles) {
+        ResolvedProductPtr freshProduct = freshProductsByName.value(product->name);
+        if (!freshProduct)
+            continue;
+        onProductFileListChanged(product, freshProduct);
     }
 
     // Move over restored build data to newly resolved project.
@@ -291,7 +306,7 @@ bool BuildGraphLoader::hasFileExistsResultChanged(const TopLevelProjectConstPtr
 
 bool BuildGraphLoader::hasProductFileChanged(const QList<ResolvedProductPtr> &restoredProducts,
         const FileTime &referenceTime, QSet<QString> &remainingBuildSystemFiles,
-        QList<ResolvedProductPtr> &changedProducts)
+        QList<ResolvedProductPtr> &productsWithChangedFiles)
 {
     bool hasChanged = false;
     foreach (const ResolvedProductPtr &product, restoredProducts) {
@@ -303,7 +318,7 @@ bool BuildGraphLoader::hasProductFileChanged(const QList<ResolvedProductPtr> &re
             hasChanged = true;
         } else if (referenceTime < pfi.lastModified()) {
             m_logger.qbsDebug() << "A product was changed, must re-resolve project";
-            changedProducts += product;
+            hasChanged = true;
         } else {
             foreach (const GroupPtr &group, product->groups) {
                 if (!group->wildcards)
@@ -315,13 +330,14 @@ bool BuildGraphLoader::hasProductFileChanged(const QList<ResolvedProductPtr> &re
                     wcFiles += sourceArtifact->absoluteFilePath;
                 if (files == wcFiles)
                     continue;
-                changedProducts += product;
+                hasChanged = true;
+                productsWithChangedFiles += product;
                 break;
             }
         }
     }
 
-    return hasChanged | !changedProducts.isEmpty();
+    return hasChanged;
 }
 
 bool BuildGraphLoader::hasBuildSystemFileChanged(const QSet<QString> &buildSystemFiles,
@@ -339,7 +355,8 @@ bool BuildGraphLoader::hasBuildSystemFileChanged(const QSet<QString> &buildSyste
 
 void BuildGraphLoader::checkAllProductsForChanges(const QList<ResolvedProductPtr> &restoredProducts,
         const QMap<QString, ResolvedProductPtr> &newlyResolvedProductsByName,
-        QList<ResolvedProductPtr> &changedProducts)
+        QList<ResolvedProductPtr> &changedProducts,
+        QList<ResolvedProductPtr> &productsWithChangedFiles)
 {
     foreach (const ResolvedProductPtr &restoredProduct, restoredProducts) {
         if (changedProducts.contains(restoredProduct))
@@ -348,14 +365,28 @@ void BuildGraphLoader::checkAllProductsForChanges(const QList<ResolvedProductPtr
                 = newlyResolvedProductsByName.value(restoredProduct->name);
         if (!newlyResolvedProduct)
             continue;
-        if (newlyResolvedProduct->allFiles() == restoredProduct->allFiles())
-            continue;
-        m_logger.qbsDebug() << "Product '" << restoredProduct->name
-                            << "' was changed, must re-resolve project";
-        changedProducts << restoredProduct;
+        if (!sourceArtifactListsAreEqual(restoredProduct->allFiles(),
+                                         newlyResolvedProduct->allFiles())) {
+            m_logger.qbsDebug() << "File list of product '" << restoredProduct->name
+                                << "' was changed.";
+            productsWithChangedFiles += restoredProduct;
+        }
+        if (checkProductForChanges(restoredProduct, newlyResolvedProduct)) {
+            m_logger.qbsDebug() << "Product '" << restoredProduct->name
+                                << "' was changed, must set up build data from scratch";
+            changedProducts << restoredProduct;
+        }
     }
 }
 
+bool BuildGraphLoader::checkProductForChanges(const ResolvedProductPtr &restoredProduct,
+                                              const ResolvedProductPtr &newlyResolvedProduct)
+{
+    return !transformerListsAreEqual(restoredProduct->transformers,
+                                     newlyResolvedProduct->transformers);
+    // TODO: Check for more stuff.
+}
+
 void BuildGraphLoader::onProductRemoved(const ResolvedProductPtr &product,
                                         ProjectBuildData *projectBuildData)
 {
@@ -370,7 +401,7 @@ void BuildGraphLoader::onProductRemoved(const ResolvedProductPtr &product,
     }
 }
 
-void BuildGraphLoader::onProductChanged(const ResolvedProductPtr &product,
+void BuildGraphLoader::onProductFileListChanged(const ResolvedProductPtr &product,
                                         const ResolvedProductPtr &changedProduct)
 {
     m_logger.qbsDebug() << "[BG] product '" << product->name << "' changed.";
@@ -429,6 +460,8 @@ void BuildGraphLoader::onProductChanged(const ResolvedProductPtr &product,
             removeArtifactAndExclusiveDependents(artifact, &artifactsToRemove);
             continue;
         }
+
+        // TODO: overrideFileTags and properties have to be checked for changes as well.
         if (changedArtifact->fileTags != a->fileTags) {
             // artifact's filetags have changed
             m_logger.qbsDebug() << "[BG] filetags have changed for artifact '"
diff --git a/src/lib/buildgraph/buildgraphloader.h b/src/lib/buildgraph/buildgraphloader.h
index 31fe52cad..0c6f1d5fe 100644
--- a/src/lib/buildgraph/buildgraphloader.h
+++ b/src/lib/buildgraph/buildgraphloader.h
@@ -72,14 +72,17 @@ private:
     bool hasProductFileChanged(const QList<ResolvedProductPtr> &restoredProducts,
                                const FileTime &referenceTime,
                                QSet<QString> &remainingBuildSystemFiles,
-                               QList<ResolvedProductPtr> &changedProducts);
+                               QList<ResolvedProductPtr> &productsWithChangedFiles);
     bool hasBuildSystemFileChanged(const QSet<QString> &buildSystemFiles,
                                    const FileTime &referenceTime);
     void checkAllProductsForChanges(const QList<ResolvedProductPtr> &restoredProducts,
             const QMap<QString, ResolvedProductPtr> &newlyResolvedProductsByName,
-            QList<ResolvedProductPtr> &changedProducts);
+            QList<ResolvedProductPtr> &changedProducts,
+            QList<ResolvedProductPtr> &productsWithChangedFiles);
+    bool checkProductForChanges(const ResolvedProductPtr &restoredProduct,
+                                const ResolvedProductPtr &newlyResolvedProduct);
     void onProductRemoved(const ResolvedProductPtr &product, ProjectBuildData *projectBuildData);
-    void onProductChanged(const ResolvedProductPtr &product,
+    void onProductFileListChanged(const ResolvedProductPtr &product,
                           const ResolvedProductPtr &changedProduct);
     void removeArtifactAndExclusiveDependents(Artifact *artifact,
             ArtifactList *removedArtifacts = 0);
diff --git a/src/lib/language/jsimports.h b/src/lib/language/jsimports.h
index 364950827..fa763f0b3 100644
--- a/src/lib/language/jsimports.h
+++ b/src/lib/language/jsimports.h
@@ -31,6 +31,7 @@
 #define QBS_JSIMPORTS_H
 
 #include <tools/codelocation.h>
+#include <QSet>
 #include <QStringList>
 
 namespace qbs {
@@ -53,6 +54,11 @@ public:
 
 typedef QList<JsImport> JsImports;
 
+inline bool operator==(const JsImport &jsi1, const JsImport &jsi2)
+{
+    return jsi1.scopeName == jsi2.scopeName && jsi1.fileNames.toSet() == jsi2.fileNames.toSet();
+}
+
 } // namespace Internal
 } // namespace qbs
 
diff --git a/src/lib/language/language.cpp b/src/lib/language/language.cpp
index e492559da..ec2bf4876 100644
--- a/src/lib/language/language.cpp
+++ b/src/lib/language/language.cpp
@@ -45,6 +45,7 @@
 
 #include <QDir>
 #include <QDirIterator>
+#include <QMap>
 #include <QMutexLocker>
 #include <QScriptValue>
 
@@ -271,6 +272,25 @@ void ResolvedModule::store(PersistentPool &pool) const
       << setupRunEnvironmentScript;
 }
 
+bool operator==(const ResolvedModule &m1, const ResolvedModule &m2)
+{
+    return m1.name == m2.name
+            && m1.moduleDependencies.toSet() == m2.moduleDependencies.toSet()
+            && m1.jsImports == m2.jsImports
+            && m1.jsExtensions.toSet() == m2.jsExtensions.toSet()
+            && m1.setupBuildEnvironmentScript == m2.setupBuildEnvironmentScript
+            && m1.setupRunEnvironmentScript == m2.setupRunEnvironmentScript;
+}
+
+static bool modulesAreEqual(const ResolvedModuleConstPtr &m1, const ResolvedModuleConstPtr &m2)
+{
+    if (!m1 && !m2)
+        return true;
+    if ((!m1 && m2) || (m1 && !m2))
+        return false;
+    return *m1 == *m2;
+}
+
 QString Rule::toString() const
 {
     return QLatin1Char('[') + inputs.toStringList().join(QLatin1String(",")) + QLatin1String(" -> ")
@@ -910,5 +930,63 @@ void ResolvedTransformer::store(PersistentPool &pool) const
     pool.stream() << jsImports << jsExtensions;
 }
 
+
+template<typename T> QMap<QString, T> listToMap(const QList<T> &list)
+{
+    QMap<QString, T> map;
+    foreach (const T &elem, list)
+        map.insert(keyFromElem(elem), elem);
+    return map;
+}
+
+template<typename T> bool listsAreEqual(const QList<T> &l1, const QList<T> &l2)
+{
+    if (l1.count() != l2.count())
+        return false;
+    const QMap<QString, T> map1 = listToMap(l1);
+    const QMap<QString, T> map2 = listToMap(l2);
+    foreach (const QString &key, map1.keys()) {
+        const T value2 = map2.value(key);
+        if (!value2)
+            return false;
+        if (*map1.value(key) != *value2)
+            return false;
+    }
+    return true;
+}
+
+QString keyFromElem(const SourceArtifactPtr &sa) { return sa->absoluteFilePath; }
+QString keyFromElem(const ResolvedTransformer::Ptr &t) { return t->transform->script; }
+
+bool operator==(const SourceArtifact &sa1, const SourceArtifact &sa2)
+{
+    return sa1.absoluteFilePath == sa2.absoluteFilePath
+            && sa1.fileTags == sa2.fileTags
+            && sa1.overrideFileTags == sa2.overrideFileTags
+            && sa1.properties->value() == sa2.properties->value();
+}
+
+bool sourceArtifactListsAreEqual(const QList<SourceArtifactPtr> &l1,
+                                 const QList<SourceArtifactPtr> &l2)
+{
+    return listsAreEqual(l1, l2);
+}
+
+bool operator==(const ResolvedTransformer &t1, const ResolvedTransformer &t2)
+{
+    return modulesAreEqual(t1.module, t2.module)
+            && t1.inputs.toSet() == t2.inputs.toSet()
+            && sourceArtifactListsAreEqual(t1.outputs, t2.outputs)
+            && t1.transform->script == t2.transform->script
+            && t1.jsImports == t2.jsImports
+            && t1.jsExtensions.toSet() == t2.jsExtensions.toSet();
+}
+
+bool transformerListsAreEqual(const QList<ResolvedTransformer::Ptr> &l1,
+                              const QList<ResolvedTransformer::Ptr> &l2)
+{
+    return listsAreEqual(l1, l2);
+}
+
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/language/language.h b/src/lib/language/language.h
index 9aa32ec98..999b5225a 100644
--- a/src/lib/language/language.h
+++ b/src/lib/language/language.h
@@ -132,6 +132,13 @@ private:
     void load(PersistentPool &pool);
     void store(PersistentPool &pool) const;
 };
+bool operator==(const SourceArtifact &sa1, const SourceArtifact &sa2);
+inline bool operator!=(const SourceArtifact &sa1, const SourceArtifact &sa2) {
+    return !(sa1 == sa2);
+}
+
+bool sourceArtifactListsAreEqual(const QList<SourceArtifactPtr> &l1,
+                                 const QList<SourceArtifactPtr> &l2);
 
 class SourceWildCards : public PersistentObject
 {
@@ -220,6 +227,8 @@ private:
     void load(PersistentPool &pool);
     void store(PersistentPool &pool) const;
 };
+bool operator==(const ResolvedModule &m1, const ResolvedModule &m2);
+inline bool operator!=(const ResolvedModule &m1, const ResolvedModule &m2) { return !(m1 == m2); }
 
 /**
   * Per default each rule is a "non-multiplex rule".
@@ -280,6 +289,13 @@ private:
     void store(PersistentPool &pool) const;
 };
 
+bool operator==(const ResolvedTransformer &t1, const ResolvedTransformer &t2);
+inline bool operator!=(const ResolvedTransformer &t1, const ResolvedTransformer &t2) {
+    return !(t1 == t2);
+}
+bool transformerListsAreEqual(const QList<ResolvedTransformer::Ptr> &l1,
+                              const QList<ResolvedTransformer::Ptr> &l2);
+
 class TopLevelProject;
 class ScriptEngine;
 
diff --git a/tests/auto/blackbox/testdata/propertyChanges/project.qbs b/tests/auto/blackbox/testdata/propertyChanges/project.qbs
index b3960dae3..045c96f22 100644
--- a/tests/auto/blackbox/testdata/propertyChanges/project.qbs
+++ b/tests/auto/blackbox/testdata/propertyChanges/project.qbs
@@ -1,4 +1,5 @@
 import qbs 1.0
+import qbs.TextFile
 
 Project {
     property var projectDefines: ["blubb2"]
@@ -17,4 +18,24 @@ Project {
         cpp.defines: qbs.getenv("QBS_BLACKBOX_DEFINE")
         files: "source3.cpp"
     }
+
+    Product {
+        name: "generated text file"
+
+        Transformer {
+            Artifact { fileName: "generated.txt" }
+            prepare: {
+                var cmd = new JavaScriptCommand();
+                cmd.description = "generating " + output.fileName;
+                cmd.highlight = "codegen";
+                cmd.sourceCode = function() {
+                    file = new TextFile(output.fileName, TextFile.WriteOnly);
+                    file.truncate();
+                    file.write("contents 1");
+                    file.close();
+                }
+                return cmd;
+            }
+        }
+    }
 }
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 88f68c2e6..c45581f7c 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -984,12 +984,18 @@ void TestBlackbox::propertyChanges()
     QVERIFY(m_qbsStdout.contains("compiling source1.cpp"));
     QVERIFY(m_qbsStdout.contains("compiling source2.cpp"));
     QVERIFY(m_qbsStdout.contains("compiling source3.cpp"));
+    QVERIFY(m_qbsStdout.contains("generated.txt"));
+    QFile generatedFile(buildDir + QLatin1String("/generated.txt"));
+    QVERIFY(generatedFile.open(QIODevice::ReadOnly));
+    QCOMPARE(generatedFile.readAll(), QByteArray("contents 1"));
+    generatedFile.close();
 
     // Incremental build with no changes.
     QCOMPARE(runQbs(), 0);
     QVERIFY(!m_qbsStdout.contains("compiling source1.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source2.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source3.cpp"));
+    QVERIFY(!m_qbsStdout.contains("generated.txt"));
 
     // Incremental build with no changes, but updated project file timestamp.
     waitForNewTimestamp();
@@ -1000,6 +1006,7 @@ void TestBlackbox::propertyChanges()
     QVERIFY(!m_qbsStdout.contains("compiling source1.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source2.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source3.cpp"));
+    QVERIFY(!m_qbsStdout.contains("generated.txt"));
 
     // Incremental build, input property changed for first product
     waitForNewTimestamp();
@@ -1013,6 +1020,7 @@ void TestBlackbox::propertyChanges()
     QVERIFY(m_qbsStdout.contains("compiling source1.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source2.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source3.cpp"));
+    QVERIFY(!m_qbsStdout.contains("generated.txt"));
 
     // Incremental build, input property changed via project for second product.
     waitForNewTimestamp();
@@ -1026,6 +1034,7 @@ void TestBlackbox::propertyChanges()
     QVERIFY(!m_qbsStdout.contains("compiling source1.cpp"));
     QVERIFY(m_qbsStdout.contains("compiling source2.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source3.cpp"));
+    QVERIFY(!m_qbsStdout.contains("generated.txt"));
 
     // Incremental build, input property changed via command line for second product.
     waitForNewTimestamp();
@@ -1034,6 +1043,7 @@ void TestBlackbox::propertyChanges()
     QEXPECT_FAIL(0, "Command-line overrides not taking part in property tracking atm", Continue);
     QVERIFY(m_qbsStdout.contains("compiling source2.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source3.cpp"));
+    QVERIFY(!m_qbsStdout.contains("generated.txt"));
 
     // Incremental build, input property changed via environment for third product.
     QbsRunParameters params;
@@ -1042,6 +1052,24 @@ void TestBlackbox::propertyChanges()
     QVERIFY(!m_qbsStdout.contains("compiling source1.cpp"));
     QVERIFY(!m_qbsStdout.contains("compiling source2.cpp"));
     QVERIFY(m_qbsStdout.contains("compiling source3.cpp"));
+    QVERIFY(!m_qbsStdout.contains("generated.txt"));
+
+    // Incremental build, prepare script of a transformer changed.
+    waitForNewTimestamp();
+    QVERIFY(projectFile.open(QIODevice::ReadWrite));
+    contents = projectFile.readAll();
+    contents.replace("contents 1", "contents 2");
+    projectFile.resize(0);
+    projectFile.write(contents);
+    projectFile.close();
+    QCOMPARE(runQbs(params), 0);
+    QVERIFY(!m_qbsStdout.contains("compiling source1.cpp"));
+    QVERIFY(!m_qbsStdout.contains("compiling source2.cpp"));
+    QVERIFY(!m_qbsStdout.contains("compiling source3.cpp"));
+    QVERIFY(m_qbsStdout.contains("generated.txt"));
+    QVERIFY(generatedFile.open(QIODevice::ReadOnly));
+    QCOMPARE(generatedFile.readAll(), QByteArray("contents 2"));
+    generatedFile.close();
 }
 
 void TestBlackbox::disabledProduct()