Fix change tracking for properties requested from prepare scripts.

When evaluating prepare scripts, we currently gather values requested
from products as well as artifacts, but we do not differentiate between
the two cases and upon restoring, we always compare the old property
values to the product properties. This results in an insane amount of
recompiling if any build system file changes due to false positives.
With this patch, we record whether a property was requested from a
product or an artifact, and use the right set of properties when
tracking changes.

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

5 files changed, 112 insertions, 34 deletions

diff --git a/src/lib/buildgraph/buildgraphloader.cpp b/src/lib/buildgraph/buildgraphloader.cpp
index 989c85d17..e10aa92c8 100644
--- a/src/lib/buildgraph/buildgraphloader.cpp
+++ b/src/lib/buildgraph/buildgraphloader.cpp
@@ -383,17 +383,20 @@ bool BuildGraphLoader::checkProductForChanges(const ResolvedProductPtr &restored
 bool BuildGraphLoader::checkForPropertyChanges(const ResolvedProductPtr &restoredProduct,
                                                const ResolvedProductPtr &newlyResolvedProduct)
 {
-    QSet<TransformerPtr> seenTransformers;
+    m_logger.qbsDebug() << "Checking for changes in properties requested in prepare scripts for "
+                           "product '"  << restoredProduct->name << "'.";
     if (!restoredProduct->buildData)
         return false;
+    QSet<TransformerConstPtr> seenTransformers;
     foreach (Artifact * const artifact, restoredProduct->buildData->artifacts) {
-        if (!artifact->transformer || seenTransformers.contains(artifact->transformer))
+        const TransformerConstPtr transformer = artifact->transformer;
+        if (!transformer || seenTransformers.contains(transformer))
             continue;
-        seenTransformers.insert(artifact->transformer);
-        if (checkForPropertyChanges(artifact->transformer, newlyResolvedProduct)) {
-                m_logger.qbsDebug() << "Property changes in product '"
-                                    << newlyResolvedProduct->name << "'.";
-                return true;
+        seenTransformers.insert(transformer);
+        if (checkForPropertyChanges(transformer, newlyResolvedProduct)) {
+            m_logger.qbsDebug() << "Property changes in product '"
+                                << newlyResolvedProduct->name << "'.";
+            return true;
         }
     }
     return false;
@@ -552,32 +555,70 @@ void BuildGraphLoader::removeArtifactAndExclusiveDependents(Artifact *artifact,
     project->buildData->removeArtifact(artifact, m_logger);
 }
 
-bool BuildGraphLoader::checkForPropertyChanges(const TransformerPtr &restoredTrafo,
-                                               const ResolvedProductPtr &freshProduct)
+static SourceArtifactConstPtr findSourceArtifact(const ResolvedProductConstPtr &product,
+        const QString &artifactFilePath, QMap<QString, SourceArtifactConstPtr> &artifactMap)
 {
-    PropertyFinder finder;
-    foreach (const Property &property, restoredTrafo->modulePropertiesUsedInPrepareScript) {
-        QVariant v;
-        if (property.kind == Property::PropertyInProduct) {
-            v = freshProduct->properties->value().value(property.propertyName);
-        } else 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);
+    SourceArtifactConstPtr &artifact = artifactMap[artifactFilePath];
+    if (!artifact) {
+        foreach (const SourceArtifactConstPtr &a, product->allFiles()) {
+            if (a->absoluteFilePath == artifactFilePath) {
+                artifact = a;
+                break;
+            }
         }
-        if (property.value != v) {
-            m_logger.qbsDebug() << "Value for property '" << property.moduleName << "."
-                                << property.propertyName << "' has changed.";
-            m_logger.qbsDebug() << "Old value was '" << property.value << "'.";
-            m_logger.qbsDebug() << "New value is '" << v << "'.";
+    }
+    return artifact;
+}
+
+bool BuildGraphLoader::checkForPropertyChanges(const TransformerConstPtr &restoredTrafo,
+        const ResolvedProductPtr &freshProduct)
+{
+    foreach (const Property &property,
+             restoredTrafo->propertiesRequestedFromProductInPrepareScript) {
+        if (checkForPropertyChange(property, freshProduct->properties))
             return true;
+    }
+
+    QMap<QString, SourceArtifactConstPtr> artifactMap;
+    for (QHash<QString, PropertyList>::ConstIterator it =
+         restoredTrafo->propertiesRequestedFromArtifactInPrepareScript.constBegin();
+         it != restoredTrafo->propertiesRequestedFromArtifactInPrepareScript.constEnd(); ++it) {
+        const SourceArtifactConstPtr artifact
+                = findSourceArtifact(freshProduct, it.key(), artifactMap);
+        if (!artifact)
+            continue;
+        foreach (const Property &property, it.value()) {
+            if (checkForPropertyChange(property, artifact->properties))
+                return true;
         }
     }
     return false;
 }
 
+bool BuildGraphLoader::checkForPropertyChange(const Property &restoredProperty,
+                                              const PropertyMapConstPtr &newProperties)
+{
+    PropertyFinder finder;
+    QVariant v;
+    if (restoredProperty.kind == Property::PropertyInProduct) {
+        v = newProperties->value().value(restoredProperty.propertyName);
+    } else if (restoredProperty.value.type() == QVariant::List) {
+        v = finder.propertyValues(newProperties->value(), restoredProperty.moduleName,
+                                  restoredProperty.propertyName);
+    } else {
+        v = finder.propertyValue(newProperties->value(), restoredProperty.moduleName,
+                                 restoredProperty.propertyName);
+    }
+    if (restoredProperty.value != v) {
+        m_logger.qbsDebug() << "Value for property '" << restoredProperty.moduleName << "."
+                            << restoredProperty.propertyName << "' has changed.";
+        m_logger.qbsDebug() << "Old value was '" << restoredProperty.value << "'.";
+        m_logger.qbsDebug() << "New value is '" << v << "'.";
+        return true;
+    }
+    return false;
+}
+
 void BuildGraphLoader::replaceFileDependencyWithArtifact(const ResolvedProductPtr &fileDepProduct,
         FileDependency *filedep, Artifact *artifact)
 {
diff --git a/src/lib/buildgraph/buildgraphloader.h b/src/lib/buildgraph/buildgraphloader.h
index 5057120d1..c2a413f60 100644
--- a/src/lib/buildgraph/buildgraphloader.h
+++ b/src/lib/buildgraph/buildgraphloader.h
@@ -44,6 +44,7 @@ namespace Internal {
 class ArtifactList;
 class FileDependency;
 class FileTime;
+class Property;
 
 class BuildGraphLoadResult
 {
@@ -88,7 +89,10 @@ private:
                           const ResolvedProductPtr &changedProduct);
     void removeArtifactAndExclusiveDependents(Artifact *artifact,
             ArtifactList *removedArtifacts = 0);
-    bool checkForPropertyChanges(const TransformerPtr &restoredTrafo, const ResolvedProductPtr &freshProduct);
+    bool checkForPropertyChanges(const TransformerConstPtr &restoredTrafo,
+            const ResolvedProductPtr &freshProduct);
+    bool checkForPropertyChange(const Property &restoredProperty,
+                                const PropertyMapConstPtr &newProperties);
     void replaceFileDependencyWithArtifact(const ResolvedProductPtr &fileDepProduct,
             FileDependency *filedep, Artifact *artifact);
     void rescueOldBuildData(const ResolvedProductConstPtr &restoredProduct,
diff --git a/src/lib/buildgraph/rulesapplicator.cpp b/src/lib/buildgraph/rulesapplicator.cpp
index 5d3db92cb..d057c3cfb 100644
--- a/src/lib/buildgraph/rulesapplicator.cpp
+++ b/src/lib/buildgraph/rulesapplicator.cpp
@@ -322,7 +322,7 @@ void RulesApplicator::onPropertyRead(const QScriptValue &object, const QString &
                                      const QScriptValue &value)
 {
     if (object.objectId() == m_productObjectId)
-        engine()->addProperty(
+        engine()->addPropertyRequestedFromProduct(
                     Property(QString(), name, value.toVariant(), Property::PropertyInProduct));
 }
 
diff --git a/src/lib/buildgraph/transformer.cpp b/src/lib/buildgraph/transformer.cpp
index 363e08d2a..02c8ec3fe 100644
--- a/src/lib/buildgraph/transformer.cpp
+++ b/src/lib/buildgraph/transformer.cpp
@@ -153,8 +153,9 @@ void Transformer::createCommands(const PrepareScriptConstPtr &script,
     }
 
     QScriptValue scriptValue = script->scriptFunction.call();
-    modulePropertiesUsedInPrepareScript = engine->properties();
-    engine->clearProperties();
+    propertiesRequestedFromProductInPrepareScript = engine->propertiesRequestedFromProduct();
+    propertiesRequestedFromArtifactInPrepareScript = engine->propertiesRequestedFromArtifact();
+    engine->clearPropertiesRequestedInPrepareScripts();
     if (Q_UNLIKELY(engine->hasUncaughtException()))
         throw ErrorInfo("evaluating prepare script: " + engine->uncaughtException().toString(),
                     CodeLocation(script->location.fileName(),
@@ -186,7 +187,7 @@ void Transformer::load(PersistentPool &pool)
     pool.loadContainer(outputs);
     int count;
     pool.stream() >> count;
-    modulePropertiesUsedInPrepareScript.reserve(count);
+    propertiesRequestedFromProductInPrepareScript.reserve(count);
     while (--count >= 0) {
         Property p;
         p.moduleName = pool.idLoadString();
@@ -194,7 +195,25 @@ void Transformer::load(PersistentPool &pool)
         int k;
         pool.stream() >> p.value >> k;
         p.kind = static_cast<Property::Kind>(k);
-        modulePropertiesUsedInPrepareScript += p;
+        propertiesRequestedFromProductInPrepareScript += p;
+    }
+    pool.stream() >> count;
+    propertiesRequestedFromArtifactInPrepareScript.reserve(count);
+    while (--count >= 0) {
+        const QString artifactName = pool.idLoadString();
+        int listCount;
+        pool.stream() >> listCount;
+        PropertyList list;
+        list.reserve(listCount);
+        while (--listCount >= 0) {
+            Property p;
+            p.moduleName = pool.idLoadString();
+            p.propertyName = pool.idLoadString();
+            pool.stream() >> p.value;
+            p.kind = Property::PropertyInModule;
+            list += p;
+        }
+        propertiesRequestedFromArtifactInPrepareScript.insert(artifactName, list);
     }
     int cmdType;
     pool.stream() >> count;
@@ -212,12 +231,24 @@ void Transformer::store(PersistentPool &pool) const
     pool.store(rule);
     pool.storeContainer(inputs);
     pool.storeContainer(outputs);
-    pool.stream() << modulePropertiesUsedInPrepareScript.count();
-    foreach (const Property &p, modulePropertiesUsedInPrepareScript) {
+    pool.stream() << propertiesRequestedFromProductInPrepareScript.count();
+    foreach (const Property &p, propertiesRequestedFromProductInPrepareScript) {
         pool.storeString(p.moduleName);
         pool.storeString(p.propertyName);
         pool.stream() << p.value << static_cast<int>(p.kind);
     }
+    pool.stream() << propertiesRequestedFromArtifactInPrepareScript.count();
+    for (QHash<QString, PropertyList>::ConstIterator it = propertiesRequestedFromArtifactInPrepareScript.constBegin();
+         it != propertiesRequestedFromArtifactInPrepareScript.constEnd(); ++it) {
+        pool.storeString(it.key());
+        const PropertyList &properties = it.value();
+        pool.stream() << properties.count();
+        foreach (const Property &p, properties) {
+            pool.storeString(p.moduleName);
+            pool.storeString(p.propertyName);
+            pool.stream() << p.value; // kind is always PropertyInModule
+        }
+    }
     pool.stream() << commands.count();
     foreach (AbstractCommand *cmd, commands) {
         pool.stream() << int(cmd->type());
diff --git a/src/lib/buildgraph/transformer.h b/src/lib/buildgraph/transformer.h
index d26c391ab..61dc41eeb 100644
--- a/src/lib/buildgraph/transformer.h
+++ b/src/lib/buildgraph/transformer.h
@@ -36,6 +36,7 @@
 #include <language/property.h>
 #include <tools/persistentobject.h>
 
+#include <QHash>
 #include <QScriptEngine>
 
 namespace qbs {
@@ -56,7 +57,8 @@ public:
     ArtifactList outputs;
     RuleConstPtr rule;
     QList<AbstractCommand *> commands;
-    PropertyList modulePropertiesUsedInPrepareScript;
+    PropertyList propertiesRequestedFromProductInPrepareScript;
+    QHash<QString, PropertyList> propertiesRequestedFromArtifactInPrepareScript;
 
     static QScriptValue translateFileConfig(QScriptEngine *scriptEngine,
                                             Artifact *artifact,