Clean up the list of file dependencies before building

After adding an external file dependency, we would never remove it from
the global list again, meaning the look-up table would contain stale
entries if an external header file was moved around. Because we still
checked the status of the artifact-local list of dependencies, the
result was that in such a situation, the files including such a header
would get recompiled on every build.

[ChangeLog] Fixed constant rebuilding after moving an external header
file.

Task-number: QBS-1285
Change-Id: Id764da7485dd540f2ff64bcd890e43723f18e6cd
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

8 files changed, 88 insertions, 9 deletions

diff --git a/changelogs/changes-1.10.1.md b/changelogs/changes-1.10.1.md
index 45b0cfdb3..d3be09e65 100644
--- a/changelogs/changes-1.10.1.md
+++ b/changelogs/changes-1.10.1.md
@@ -2,6 +2,7 @@
 * Fix assertion on project loading (QBS-1275).
 * Fix crash when the "original" value is misused (QBS-1255).
 * Fix qtquickcompiler support for qml files in subdirectories (QBS-1261).
+* Fix constant rebuilding after moving an external header file (QBS-1285).
 * Fix GCC support for "bare metal" systems (QBS-1263, QBS-1265).
 * Fix using ids in Depends items (QBS-1264).
 * Fix access to module instances in dependency parameters (QBS-1253).
diff --git a/src/lib/corelib/buildgraph/executor.cpp b/src/lib/corelib/buildgraph/executor.cpp
index 61533c4f0..b2cf22325 100644
--- a/src/lib/corelib/buildgraph/executor.cpp
+++ b/src/lib/corelib/buildgraph/executor.cpp
@@ -147,7 +147,7 @@ void Executor::retrieveSourceFileTimestamp(Artifact *artifact) const
 void Executor::build()
 {
     try {
-        m_partialBuild = m_productsToBuild.count() != m_project->allProducts().count();
+        m_partialBuild = m_productsToBuild.count() != m_allProducts.count();
         doBuild();
     } catch (const ErrorInfo &e) {
         handleError(e);
@@ -157,6 +157,7 @@ void Executor::build()
 void Executor::setProject(const TopLevelProjectPtr &project)
 {
     m_project = project;
+    m_allProducts = project->allProducts();
 }
 
 void Executor::setProducts(const QList<ResolvedProductPtr> &productsToBuild)
@@ -166,23 +167,22 @@ void Executor::setProducts(const QList<ResolvedProductPtr> &productsToBuild)
 
 class ProductPrioritySetter
 {
-    const TopLevelProject *m_topLevelProject;
+    const QList<ResolvedProductPtr> &m_allProducts;
     unsigned int m_priority;
     Set<ResolvedProductPtr> m_seenProducts;
 public:
-    ProductPrioritySetter(const TopLevelProject *tlp)
-        : m_topLevelProject(tlp)
+    ProductPrioritySetter(const QList<ResolvedProductPtr> &allProducts) // TODO: Use only products to build?
+        : m_allProducts(allProducts)
     {
     }
 
     void apply()
     {
-        const QList<ResolvedProductPtr> &allProducts = m_topLevelProject->allProducts();
         Set<ResolvedProductPtr> allDependencies;
-        for (const ResolvedProductPtr &product : allProducts)
+        for (const ResolvedProductPtr &product : m_allProducts)
             allDependencies += product->dependencies;
         const Set<ResolvedProductPtr> rootProducts
-                = Set<ResolvedProductPtr>::fromList(allProducts) - allDependencies;
+                = Set<ResolvedProductPtr>::fromList(m_allProducts) - allDependencies;
         m_priority = UINT_MAX;
         m_seenProducts.clear();
         for (const ResolvedProductPtr &rootProduct : rootProducts)
@@ -266,6 +266,7 @@ void Executor::doBuild()
         m_productInstaller->removeInstallRoot();
 
     addExecutorJobs();
+    syncFileDependencies();
     prepareAllNodes();
     prepareProducts();
     setupRootNodes();
@@ -1095,7 +1096,7 @@ bool Executor::visit(RuleNode *ruleNode)
   */
 void Executor::prepareAllNodes()
 {
-    for (const ResolvedProductPtr &product : m_project->allProducts()) {
+    for (const ResolvedProductPtr &product : m_allProducts) {
         if (product->enabled) {
             QBS_CHECK(product->buildData);
             for (BuildGraphNode * const node : qAsConst(product->buildData->nodes))
@@ -1109,6 +1110,41 @@ void Executor::prepareAllNodes()
     }
 }
 
+void Executor::syncFileDependencies()
+{
+    Set<FileDependency *> &globalFileDepList = m_project->buildData->fileDependencies;
+    for (auto it = globalFileDepList.begin(); it != globalFileDepList.end(); ) {
+        FileDependency * const dep = *it;
+        if (FileInfo(dep->filePath()).exists()) {
+            ++it;
+            continue;
+        }
+        qCDebug(lcBuildGraph()) << "file dependency" << dep->filePath() << "no longer exists; "
+                                   "removing from lookup table";
+        m_project->buildData->removeFromLookupTable(dep);
+        bool isReferencedByArtifact = false;
+        for (const ResolvedProductConstPtr &product : m_allProducts) {
+            if (!product->buildData)
+                continue;
+            const auto artifactList = filterByType<Artifact>(product->buildData->nodes);
+            isReferencedByArtifact = std::any_of(artifactList.begin(), artifactList.end(),
+                    [dep](const Artifact *a) { return a->fileDependencies.contains(dep); });
+            // TODO: Would it be safe to mark the artifact as "not up to date" here and clear
+            //       its list of file dependencies, rather than doing the check again in
+            //       isUpToDate()?
+            if (isReferencedByArtifact)
+                break;
+        }
+        if (!isReferencedByArtifact) {
+            qCDebug(lcBuildGraph()) << "dependency is not referenced by any artifact, deleting";
+            it = globalFileDepList.erase(it);
+            delete dep;
+        } else {
+            ++it;
+        }
+    }
+}
+
 void Executor::prepareArtifact(Artifact *artifact)
 {
     artifact->inputsScanned = false;
@@ -1123,6 +1159,9 @@ void Executor::prepareArtifact(Artifact *artifact)
     }
 
     // Timestamps of file dependencies must be invalid for every build.
+    // TODO: These should be a subset of ProjectBuildData::fileDependencies, so clear the
+    // timestamps in syncFileDepencencies() instead.
+    // TODO: Verify this assumption in the sanity checks.
     for (FileDependency * const fileDependency : qAsConst(artifact->fileDependencies))
         fileDependency->clearTimestamp();
 }
@@ -1173,7 +1212,7 @@ void Executor::prepareReachableNodes_impl(BuildGraphNode *node)
 
 void Executor::prepareProducts()
 {
-    ProductPrioritySetter prioritySetter(m_project.get());
+    ProductPrioritySetter prioritySetter(m_allProducts);
     prioritySetter.apply();
     for (const ResolvedProductPtr &product : qAsConst(m_productsToBuild))
         product->setupBuildEnvironment(m_evalContext->engine(), m_project->environment);
diff --git a/src/lib/corelib/buildgraph/executor.h b/src/lib/corelib/buildgraph/executor.h
index 849920aee..9be8558bb 100644
--- a/src/lib/corelib/buildgraph/executor.h
+++ b/src/lib/corelib/buildgraph/executor.h
@@ -111,6 +111,7 @@ private:
 
     void doBuild();
     void prepareAllNodes();
+    void syncFileDependencies();
     void prepareArtifact(Artifact *artifact);
     void setupForBuildingSelectedFiles(const BuildGraphNode *node);
     void prepareReachableNodes();
@@ -162,6 +163,7 @@ private:
     ExecutorState m_state;
     TopLevelProjectPtr m_project;
     QList<ResolvedProductPtr> m_productsToBuild;
+    QList<ResolvedProductPtr> m_allProducts;
     NodeSet m_roots;
     Leaves m_leaves;
     QList<Artifact *> m_changedSourceArtifacts;
diff --git a/tests/auto/blackbox/testdata/moved-file-dependency/main.cpp b/tests/auto/blackbox/testdata/moved-file-dependency/main.cpp
new file mode 100644
index 000000000..3e89e9f26
--- /dev/null
+++ b/tests/auto/blackbox/testdata/moved-file-dependency/main.cpp
@@ -0,0 +1,3 @@
+#include <theheader.h>
+
+int main()  {}
diff --git a/tests/auto/blackbox/testdata/moved-file-dependency/moved-file-dependency.qbs b/tests/auto/blackbox/testdata/moved-file-dependency/moved-file-dependency.qbs
new file mode 100644
index 000000000..6252f98ed
--- /dev/null
+++ b/tests/auto/blackbox/testdata/moved-file-dependency/moved-file-dependency.qbs
@@ -0,0 +1,6 @@
+import qbs
+
+CppApplication {
+    cpp.includePaths: ["subdir1", "subdir2"]
+    files: ["main.cpp"]
+}
diff --git a/tests/auto/blackbox/testdata/moved-file-dependency/subdir1/theheader.h b/tests/auto/blackbox/testdata/moved-file-dependency/subdir1/theheader.h
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/tests/auto/blackbox/testdata/moved-file-dependency/subdir1/theheader.h
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 954c5cd6a..ddba191cb 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -5291,6 +5291,33 @@ void TestBlackbox::missingOverridePrefix()
              m_qbsStderr.constData());
 }
 
+void TestBlackbox::movedFileDependency()
+{
+    QDir::setCurrent(testDataDir + "/moved-file-dependency");
+    const QString subdir2 = QDir::currentPath() + "/subdir2";
+    QVERIFY(QDir::current().mkdir(subdir2));
+    const QString oldHeaderFilePath = QDir::currentPath() + "/subdir1/theheader.h";
+    const QString newHeaderFilePath = subdir2 + "/theheader.h";
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(m_qbsStdout.contains("compiling main.cpp"), m_qbsStdout.constData());
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(!m_qbsStdout.contains("compiling main.cpp"), m_qbsStdout.constData());
+
+    QFile f(oldHeaderFilePath);
+    QVERIFY2(f.rename(newHeaderFilePath), qPrintable(f.errorString()));
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(m_qbsStdout.contains("compiling main.cpp"), m_qbsStdout.constData());
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(!m_qbsStdout.contains("compiling main.cpp"), m_qbsStdout.constData());
+
+    f.setFileName(newHeaderFilePath);
+    QVERIFY2(f.rename(oldHeaderFilePath), qPrintable(f.errorString()));
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(m_qbsStdout.contains("compiling main.cpp"), m_qbsStdout.constData());
+    QCOMPARE(runQbs(), 0);
+    QVERIFY2(!m_qbsStdout.contains("compiling main.cpp"), m_qbsStdout.constData());
+}
+
 void TestBlackbox::badInterpreter()
 {
     if (!HostOsInfo::isAnyUnixHost())
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index f08e1ad25..d5d43afa7 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -142,6 +142,7 @@ private slots:
     void missingDependency();
     void missingProjectFile();
     void missingOverridePrefix();
+    void movedFileDependency();
     void multipleChanges();
     void nestedGroups();
     void nestedProperties();