Make sure no remains of removed products stay in the build graph.

Namely, artifacts in transformer inputs of (former) parents.

Task-number: QBS-358
Change-Id: I19e6cf6cd50e4d99c49e2e570daf163da25a3a79
Reviewed-by: Joerg Bornemann <joerg.bornemann@digia.com>

11 files changed, 114 insertions, 56 deletions

diff --git a/src/lib/buildgraph/artifact.cpp b/src/lib/buildgraph/artifact.cpp
index 9923ede18..75a284a45 100644
--- a/src/lib/buildgraph/artifact.cpp
+++ b/src/lib/buildgraph/artifact.cpp
@@ -28,12 +28,10 @@
 ****************************************************************************/
 
 #include "artifact.h"
+
 #include "transformer.h"
-#include "buildgraph.h"
 
-#include <language/language.h>
 #include <language/propertymapinternal.h>
-#include <logging/logger.h>
 #include <tools/fileinfo.h>
 #include <tools/persistence.h>
 
@@ -115,33 +113,5 @@ void Artifact::store(PersistentPool &pool) const
             << static_cast<unsigned char>(alwaysUpdated);
 }
 
-void Artifact::disconnectChildren(const Logger &logger)
-{
-    if (logger.traceEnabled()) {
-        logger.qbsTrace() << QString::fromLocal8Bit("[BG] disconnectChildren: '%1'")
-                             .arg(relativeArtifactFileName(this));
-    }
-    foreach (Artifact * const child, children)
-        child->parents.remove(this);
-    children.clear();
-}
-
-void Artifact::disconnectParents(const Logger &logger)
-{
-    if (logger.traceEnabled()) {
-        logger.qbsTrace() << QString::fromLocal8Bit("[BG] disconnectParents: '%1'")
-                             .arg(relativeArtifactFileName(this));
-    }
-    foreach (Artifact * const parent, parents)
-        parent->children.remove(this);
-    parents.clear();
-}
-
-void Artifact::disconnectAll(const Logger &logger)
-{
-    disconnectChildren(logger);
-    disconnectParents(logger);
-}
-
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/buildgraph/artifact.h b/src/lib/buildgraph/artifact.h
index bc2a8025d..ce0aff81a 100644
--- a/src/lib/buildgraph/artifact.h
+++ b/src/lib/buildgraph/artifact.h
@@ -101,15 +101,11 @@ public:
     const QString &filePath() const { return m_filePath; }
     QString dirPath() const { return m_dirPath.toString(); }
     QString fileName() const { return m_fileName.toString(); }
-    void disconnectAll(const Logger &logger);
 
 private:
     void load(PersistentPool &pool);
     void store(PersistentPool &pool) const;
 
-    void disconnectChildren(const Logger &logger);
-    void disconnectParents(const Logger &logger);
-
 private:
     QString m_filePath;
     QStringRef m_dirPath;
diff --git a/src/lib/buildgraph/buildgraphloader.cpp b/src/lib/buildgraph/buildgraphloader.cpp
index afb0ac090..ce04d91cc 100644
--- a/src/lib/buildgraph/buildgraphloader.cpp
+++ b/src/lib/buildgraph/buildgraphloader.cpp
@@ -324,12 +324,13 @@ void BuildGraphLoader::trackProjectChanges(const SetupProjectParameters &paramet
 
     // Products still left in the list do not exist anymore.
     foreach (const ResolvedProductPtr &removedProduct, allRestoredProducts)
-        onProductRemoved(removedProduct);
+        onProductRemoved(removedProduct, m_result.newlyResolvedProject->buildData.data());
 
     CycleDetector(m_logger).visitProject(m_result.newlyResolvedProject);
 }
 
-void BuildGraphLoader::onProductRemoved(const ResolvedProductPtr &product)
+void BuildGraphLoader::onProductRemoved(const ResolvedProductPtr &product,
+                                        ProjectBuildData *projectBuildData)
 {
     m_logger.qbsDebug() << "[BG] product '" << product->name << "' removed.";
 
@@ -337,10 +338,8 @@ void BuildGraphLoader::onProductRemoved(const ResolvedProductPtr &product)
 
     // delete all removed artifacts physically from the disk
     if (product->buildData) {
-        foreach (Artifact *artifact, product->buildData->artifacts) {
-            artifact->disconnectAll(m_logger);
-            removeGeneratedArtifactFromDisk(artifact, m_logger);
-        }
+        foreach (Artifact *artifact, product->buildData->artifacts)
+            projectBuildData->removeArtifact(artifact, projectBuildData, m_logger);
     }
 }
 
diff --git a/src/lib/buildgraph/buildgraphloader.h b/src/lib/buildgraph/buildgraphloader.h
index 6bc0b625b..999d1f205 100644
--- a/src/lib/buildgraph/buildgraphloader.h
+++ b/src/lib/buildgraph/buildgraphloader.h
@@ -65,7 +65,7 @@ private:
     void trackProjectChanges(const SetupProjectParameters &parameters,
                              const QString &buildGraphFilePath,
                              const TopLevelProjectPtr &restoredProject);
-    void onProductRemoved(const ResolvedProductPtr &product);
+    void onProductRemoved(const ResolvedProductPtr &product, ProjectBuildData *projectBuildData);
     void onProductChanged(const ResolvedProductPtr &product,
                           const ResolvedProductPtr &changedProduct);
     void removeArtifactAndExclusiveDependents(Artifact *artifact,
diff --git a/src/lib/buildgraph/projectbuilddata.cpp b/src/lib/buildgraph/projectbuilddata.cpp
index fbcbf4796..2c4e44935 100644
--- a/src/lib/buildgraph/projectbuilddata.cpp
+++ b/src/lib/buildgraph/projectbuilddata.cpp
@@ -107,8 +107,44 @@ static bool commandsEqual(const TransformerConstPtr &t1, const TransformerConstP
     return true;
 }
 
+static void disconnectArtifactChildren(Artifact *artifact, const Logger &logger)
+{
+    if (logger.traceEnabled()) {
+        logger.qbsTrace() << QString::fromLocal8Bit("[BG] disconnectChildren: '%1'")
+                             .arg(relativeArtifactFileName(artifact));
+    }
+    foreach (Artifact * const child, artifact->children)
+        child->parents.remove(artifact);
+    artifact->children.clear();
+}
 
-void ProjectBuildData::removeArtifact(Artifact *artifact, const Logger &logger)
+static void disconnectArtifactParents(Artifact *artifact, ProjectBuildData *projectBuildData,
+                               const Logger &logger)
+{
+    if (logger.traceEnabled()) {
+        logger.qbsTrace() << QString::fromLocal8Bit("[BG] disconnectParents: '%1'")
+                             .arg(relativeArtifactFileName(artifact));
+    }
+    foreach (Artifact * const parent, artifact->parents) {
+        parent->children.remove(artifact);
+        if (parent->transformer) {
+            parent->transformer->inputs.remove(artifact);
+            projectBuildData->artifactsThatMustGetNewTransformers += parent;
+        }
+    }
+
+    artifact->parents.clear();
+}
+
+static void disconnectArtifact(Artifact *artifact, ProjectBuildData *projectBuildData,
+                               const Logger &logger)
+{
+    disconnectArtifactChildren(artifact, logger);
+    disconnectArtifactParents(artifact, projectBuildData, logger);
+}
+
+void ProjectBuildData::removeArtifact(Artifact *artifact, ProjectBuildData *projectBuildData,
+                                      const Logger &logger)
 {
     if (logger.traceEnabled())
         logger.qbsTrace() << "[BG] remove artifact " << relativeArtifactFileName(artifact);
@@ -117,20 +153,16 @@ void ProjectBuildData::removeArtifact(Artifact *artifact, const Logger &logger)
     artifact->product->buildData->artifacts.remove(artifact);
     removeFromArtifactLookupTable(artifact);
     artifact->product->buildData->targetArtifacts.remove(artifact);
-    foreach (Artifact *parent, artifact->parents) {
-        parent->children.remove(artifact);
-        if (parent->transformer) {
-            parent->transformer->inputs.remove(artifact);
-            artifactsThatMustGetNewTransformers += parent;
-        }
-    }
-    foreach (Artifact *child, artifact->children)
-        child->parents.remove(artifact);
-    artifact->children.clear();
-    artifact->parents.clear();
+    disconnectArtifact(artifact, projectBuildData, logger);
+    projectBuildData->artifactsThatMustGetNewTransformers -= artifact;
     isDirty = true;
 }
 
+void ProjectBuildData::removeArtifact(Artifact *artifact, const Logger &logger)
+{
+    removeArtifact(artifact, artifact->topLevelProject->buildData.data(), logger);
+}
+
 void ProjectBuildData::updateNodesThatMustGetNewTransformer(const Logger &logger)
 {
     RulesEvaluationContext::Scope s(evaluationContext.data());
diff --git a/src/lib/buildgraph/projectbuilddata.h b/src/lib/buildgraph/projectbuilddata.h
index 334cf8e75..637889805 100644
--- a/src/lib/buildgraph/projectbuilddata.h
+++ b/src/lib/buildgraph/projectbuilddata.h
@@ -59,8 +59,10 @@ public:
     QList<Artifact *> lookupArtifacts(const QString &dirPath, const QString &fileName) const;
     QList<Artifact *> lookupArtifacts(const Artifact *artifact) const;
     void insertFileDependency(Artifact *artifact);
-    void removeArtifact(Artifact *artifact, const Logger &logger);
     void updateNodesThatMustGetNewTransformer(const Logger &logger);
+    void removeArtifact(Artifact *artifact, const Logger &logger);
+    void removeArtifact(Artifact *artifact, ProjectBuildData *projectBuildData,
+                        const Logger &logger);
 
     ArtifactList dependencyArtifacts;
     RulesEvaluationContextPtr evaluationContext;
@@ -72,7 +74,6 @@ private:
     void store(PersistentPool &pool) const;
     void updateNodeThatMustGetNewTransformer(Artifact *artifact, const Logger &logger);
 
-private:
     QHash<QString, QHash<QString, QList<Artifact *> > > m_artifactLookupTable;
 };
 
diff --git a/tests/auto/blackbox/testdata/renameProduct/lib.cpp b/tests/auto/blackbox/testdata/renameProduct/lib.cpp
new file mode 100644
index 000000000..47ce43bc9
--- /dev/null
+++ b/tests/auto/blackbox/testdata/renameProduct/lib.cpp
@@ -0,0 +1,3 @@
+#include <QtGlobal>
+
+MY_EXPORT void f() { }
diff --git a/tests/auto/blackbox/testdata/renameProduct/main.cpp b/tests/auto/blackbox/testdata/renameProduct/main.cpp
new file mode 100644
index 000000000..6a0bac9f1
--- /dev/null
+++ b/tests/auto/blackbox/testdata/renameProduct/main.cpp
@@ -0,0 +1,6 @@
+void f();
+
+int main()
+{
+    f();
+}
diff --git a/tests/auto/blackbox/testdata/renameProduct/rename.qbs b/tests/auto/blackbox/testdata/renameProduct/rename.qbs
new file mode 100644
index 000000000..a2fe2e22f
--- /dev/null
+++ b/tests/auto/blackbox/testdata/renameProduct/rename.qbs
@@ -0,0 +1,17 @@
+import qbs
+
+Project {
+    CppApplication {
+        Depends { name: "TheLib" }
+        cpp.defines: "MY_EXPORT="
+        files: "main.cpp"
+    }
+
+    DynamicLibrary {
+        name: "TheLib"
+        Depends { name: "cpp" }
+        Depends { name: "Qt.core" }
+        cpp.defines: "MY_EXPORT=Q_DECL_EXPORT"
+        files: "lib.cpp"
+    }
+}
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index d576952e1..79ebb7ae1 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -408,6 +408,39 @@ void TestBlackbox::clean()
     QVERIFY(QFile(depExeFilePath).exists());
 }
 
+void TestBlackbox::renameProduct()
+{
+    QDir::setCurrent(testDataDir + "/renameProduct");
+
+    // Initial run.
+    QCOMPARE(runQbs(), 0);
+
+    // Rename lib and adapt Depends item.
+    waitForNewTimestamp();
+    QFile f("rename.qbs");
+    QVERIFY(f.open(QIODevice::ReadWrite));
+    QByteArray contents = f.readAll();
+    contents.replace("TheLib", "thelib");
+    f.resize(0);
+    f.write(contents);
+    f.close();
+    QCOMPARE(runQbs(), 0);
+
+    // Rename lib and don't adapt Depends item.
+    waitForNewTimestamp();
+    QVERIFY(f.open(QIODevice::ReadWrite));
+    contents = f.readAll();
+    const int libNameIndex = contents.lastIndexOf("thelib");
+    QVERIFY(libNameIndex != -1);
+    contents.replace(libNameIndex, 6, "TheLib");
+    f.resize(0);
+    f.write(contents);
+    f.close();
+    QbsRunParameters params;
+    params.expectFailure = true;
+    QVERIFY(runQbs(params) != 0);
+}
+
 void TestBlackbox::subProjects()
 {
     QDir::setCurrent(testDataDir + "/subprojects");
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index 743edc649..f703a51c7 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -104,6 +104,7 @@ private slots:
     void resolve_project_dry_run_data();
     void resolve_project_dry_run();
     void clean();
+    void renameProduct();
     void subProjects();
     void track_qrc();
     void track_qobject_change();