Change tracking: Do not rescue outdated child artifacts

If a former child artifact is not in the list anymore after the rules
applicator has run and it was not originally added as a result of
scanning, then it is no longer a child artifact.
We used to add these artifacts back to the list of children, so that an
existing parent/child connection persisted even after changing the rule
input tags in a way that the child would not match anymore.

Change-Id: I700f515bbe5732bdb9251d48e3fc5ad53fb181cc
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@qt.io>
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

4 files changed, 84 insertions, 9 deletions

diff --git a/src/lib/corelib/buildgraph/executor.cpp b/src/lib/corelib/buildgraph/executor.cpp
index 24f484ad2..8713b9b41 100644
--- a/src/lib/corelib/buildgraph/executor.cpp
+++ b/src/lib/corelib/buildgraph/executor.cpp
@@ -771,8 +771,7 @@ void Executor::rescueOldBuildData(Artifact *artifact, bool *childrenAdded = 0)
         return;
     qCDebug(lcBuildGraph) << "Attempting to rescue data of artifact" << artifact->fileName();
 
-    typedef std::pair<Artifact *, bool> ChildArtifactData;
-    QList<ChildArtifactData> childrenToConnect;
+    std::vector<Artifact *> childrenToConnect;
     bool canRescue = artifact->transformer->commands == rad.commands;
     if (canRescue) {
         ResolvedProductPtr pseudoProduct = ResolvedProduct::create();
@@ -796,10 +795,15 @@ void Executor::rescueOldBuildData(Artifact *artifact, bool *childrenAdded = 0)
                     removeGeneratedArtifactFromDisk(cd.childFilePath, m_logger);
                 }
             }
-            // TODO: Shouldn't addedByScanner always be true here? Otherwise the child would be
-            //       in the list already, no?
+            if (!cd.addedByScanner) {
+                // If an artifact has disappeared from the list of children, the commands
+                // might need to run again.
+                canRescue = false;
+                qCDebug(lcBuildGraph) << "Former child artifact" << cd.childFilePath <<
+                                         "is no longer in the list of children";
+            }
             if (canRescue)
-                childrenToConnect.push_back({child, cd.addedByScanner});
+                childrenToConnect.push_back(child);
         }
         for (const QString &depPath : rad.fileDependencies) {
             const QList<FileResourceBase *> depList = m_project->buildData->lookupFiles(depPath);
@@ -865,10 +869,9 @@ void Executor::rescueOldBuildData(Artifact *artifact, bool *childrenAdded = 0)
         artifact->knownOutOfDate = artifact->knownOutOfDate || rad.knownOutOfDate;
         if (childrenAdded && !childrenToConnect.empty())
             *childrenAdded = true;
-        for (const ChildArtifactData &cad : qAsConst(childrenToConnect)) {
-            safeConnect(artifact, cad.first);
-            if (cad.second)
-                artifact->childrenAddedByScanner << cad.first;
+        for (Artifact * const child : childrenToConnect) {
+            safeConnect(artifact, child);
+            artifact->childrenAddedByScanner << child;
         }
         qCDebug(lcBuildGraph) << "Data was rescued.";
     } else {
diff --git a/tests/auto/blackbox/testdata/changed-rule-inputs/changed-rule-inputs.qbs b/tests/auto/blackbox/testdata/changed-rule-inputs/changed-rule-inputs.qbs
new file mode 100644
index 000000000..8aef7b9b6
--- /dev/null
+++ b/tests/auto/blackbox/testdata/changed-rule-inputs/changed-rule-inputs.qbs
@@ -0,0 +1,42 @@
+import qbs
+
+Project {
+    Product {
+        name: "p1"
+        type: "p1"
+        Rule {
+            alwaysRun: true
+            multiplex: true
+            Artifact {
+                fileTags: "p1"
+                filePath: "p1-dummy"
+            }
+            prepare: {
+                var cmd = new JavaScriptCommand();
+                cmd.description = "generating " + output.fileName;
+                cmd.sourceCode = function() {};
+                return cmd;
+            }
+        }
+    }
+    Product {
+        name: "p2"
+        type: "p2"
+        Depends { name: "p1" }
+        Rule {
+            requiresInputs: false
+            multiplex: true
+            inputsFromDependencies: "p1"
+            Artifact {
+                fileTags: "p2"
+                filePath: "p2-dummy"
+            }
+            prepare: {
+                var cmd = new JavaScriptCommand();
+                cmd.description = "generating " + output.fileName;
+                cmd.sourceCode = function() {};
+                return cmd;
+            }
+        }
+    }
+}
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 0c3f8b1bc..6987ae662 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -707,6 +707,35 @@ void TestBlackbox::changedFiles()
     QVERIFY2(m_qbsStdout.contains("file1.cpp"), m_qbsStdout.constData());
 }
 
+void TestBlackbox::changedRuleInputs()
+{
+    QDir::setCurrent(testDataDir + "/changed-rule-inputs");
+
+    // Initial build.
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(m_qbsStdout.contains("generating p1-dummy"), m_qbsStdout.constData());
+    QVERIFY2(m_qbsStdout.contains("generating p2-dummy"), m_qbsStdout.constData());
+
+    // Re-build: p1 is always regenerated, and p2 has a dependency on it.
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(m_qbsStdout.contains("generating p1-dummy"), m_qbsStdout.constData());
+    QVERIFY2(m_qbsStdout.contains("generating p2-dummy"), m_qbsStdout.constData());
+
+    // Remove the dependency. p2 gets re-generated one last time, because its set of
+    // inputs changed.
+    WAIT_FOR_NEW_TIMESTAMP();
+    REPLACE_IN_FILE("changed-rule-inputs.qbs", "inputsFromDependencies: \"p1\"",
+                    "inputsFromDependencies: \"p3\"");
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(m_qbsStdout.contains("generating p1-dummy"), m_qbsStdout.constData());
+    QVERIFY2(m_qbsStdout.contains("generating p2-dummy"), m_qbsStdout.constData());
+
+    // Now the artifacts are no longer connected, and p2 must not get rebuilt anymore.
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(m_qbsStdout.contains("generating p1-dummy"), m_qbsStdout.constData());
+    QVERIFY2(!m_qbsStdout.contains("generating p2-dummy"), m_qbsStdout.constData());
+}
+
 void TestBlackbox::changeInDisabledProduct()
 {
     QDir::setCurrent(testDataDir + "/change-in-disabled-product");
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index 70a950a84..35ba2c5b7 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -57,6 +57,7 @@ private slots:
     void buildGraphVersions();
     void changedFiles_data();
     void changedFiles();
+    void changedRuleInputs();
     void changeInDisabledProduct();
     void changeInImportedFile();
     void changeTrackingAndMultiplexing();