Executor: Do not assert on condition that can actually happen.v1.5.1

We asserted on the condition that a node's product must be in the list
of products to build, meaning that it needs to belong to the set of
user-selected products or one of their dependencies. However, there are
at least two ways in which one can trigger this condition to be false:
1) The user provides a faulty project in which a "Depends" item is
missing, but the respective artifact from the other product is still
found (e.g. a generated header file via a project-global include
path).
2) The project is actually okay, but our C++ scanner erroneously adds
dependencies from a different product, e.g. because it does not know
about #ifdefs.

Instead of the assertion, we now simply mark the respective node as
built and continue. Ideally, we'd log a warning, but we cannot do that
because of point 2) above.

Change-Id: I3549d732dea5cde84d1019132580a8e051c9db11
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

7 files changed, 81 insertions, 3 deletions

diff --git a/src/lib/corelib/buildgraph/executor.cpp b/src/lib/corelib/buildgraph/executor.cpp
index be5a93302..2706d0323 100644
--- a/src/lib/corelib/buildgraph/executor.cpp
+++ b/src/lib/corelib/buildgraph/executor.cpp
@@ -408,6 +408,9 @@ void Executor::buildArtifact(Artifact *artifact)
 
     QBS_CHECK(artifact->buildState == BuildGraphNode::Buildable);
 
+    if (artifact->artifactType != Artifact::SourceFile && !checkNodeProduct(artifact))
+        return;
+
     // skip artifacts without transformer
     if (artifact->artifactType != Artifact::Generated) {
         // For source artifacts, that were not reachable when initializing the build, we must
@@ -430,6 +433,9 @@ void Executor::buildArtifact(Artifact *artifact)
 
 void Executor::executeRuleNode(RuleNode *ruleNode)
 {
+    if (!checkNodeProduct(ruleNode))
+        return;
+
     QBS_CHECK(!m_evalContext->isActive());
     ArtifactSet changedInputArtifacts;
     if (ruleNode->rule()->isDynamic()) {
@@ -972,6 +978,20 @@ void Executor::checkForUnbuiltProducts()
     }
 }
 
+bool Executor::checkNodeProduct(BuildGraphNode *node)
+{
+    if (m_productsToBuild.contains(node->product))
+        return true;
+
+    // TODO: Turn this into a warning once we have a reliable C++ scanner.
+    m_logger.qbsTrace() << "Ignoring node " << node->toString() << ", which belongs to a "
+                            "product that is not part of the list of products to build. "
+                            "Possible reasons are an erroneous project design or a false "
+                            "positive from a dependency scanner.";
+    finishNode(node);
+    return false;
+}
+
 void Executor::finish()
 {
     QBS_ASSERT(m_state != ExecutorIdle, /* ignore */);
@@ -1007,8 +1027,6 @@ void Executor::checkForCancellation()
 bool Executor::visit(Artifact *artifact)
 {
     QBS_CHECK(artifact->buildState != BuildGraphNode::Untouched);
-    QBS_CHECK(artifact->artifactType == Artifact::SourceFile
-              || m_productsToBuild.contains(artifact->product));
     buildArtifact(artifact);
     return false;
 }
@@ -1016,7 +1034,6 @@ bool Executor::visit(Artifact *artifact)
 bool Executor::visit(RuleNode *ruleNode)
 {
     QBS_CHECK(ruleNode->buildState != BuildGraphNode::Untouched);
-    QBS_CHECK(m_productsToBuild.contains(ruleNode->product));
     executeRuleNode(ruleNode);
     return false;
 }
diff --git a/src/lib/corelib/buildgraph/executor.h b/src/lib/corelib/buildgraph/executor.h
index d626cca3d..2018357b6 100644
--- a/src/lib/corelib/buildgraph/executor.h
+++ b/src/lib/corelib/buildgraph/executor.h
@@ -131,6 +131,7 @@ private:
     void finishTransformer(const TransformerPtr &transformer);
     void possiblyInstallArtifact(const Artifact *artifact);
     void checkForUnbuiltProducts();
+    bool checkNodeProduct(BuildGraphNode *node);
 
     bool mustExecuteTransformer(const TransformerPtr &transformer) const;
     bool isUpToDate(Artifact *artifact) const;
diff --git a/tests/auto/blackbox/testdata/missing-dependency/main.cpp b/tests/auto/blackbox/testdata/missing-dependency/main.cpp
new file mode 100644
index 000000000..ee2bdf30e
--- /dev/null
+++ b/tests/auto/blackbox/testdata/missing-dependency/main.cpp
@@ -0,0 +1,5 @@
+#include <theHeader.h>
+
+int main()
+{
+}
diff --git a/tests/auto/blackbox/testdata/missing-dependency/missing-dependency.qbs b/tests/auto/blackbox/testdata/missing-dependency/missing-dependency.qbs
new file mode 100644
index 000000000..e3a37d415
--- /dev/null
+++ b/tests/auto/blackbox/testdata/missing-dependency/missing-dependency.qbs
@@ -0,0 +1,39 @@
+import qbs
+import qbs.TextFile
+
+Project {
+    Product {
+        name: "theDep"
+        type: ["genheader"]
+
+        // TODO: Remove in 1.6
+        Group {
+            files: ["theHeader.h.in"]
+            fileTags: ["header.in"]
+        }
+
+        Rule {
+            inputs: ["header.in"]
+            Artifact {
+                filePath: project.buildDirectory + "/theHeader.h"
+                fileTags: product.type
+            }
+            prepare: {
+                var cmd = new JavaScriptCommand();
+                cmd.silent = true;
+                cmd.sourceCode = function() {
+                    var f = new TextFile(output.filePath, TextFile.WriteOnly);
+                    f.close();
+                }
+                return [cmd];
+            }
+        }
+    }
+    CppApplication {
+        name: "theApp"
+        cpp.includePaths: [project.buildDirectory]
+        files: ["main.cpp"]
+    }
+}
+
+
diff --git a/tests/auto/blackbox/testdata/missing-dependency/theHeader.h.in b/tests/auto/blackbox/testdata/missing-dependency/theHeader.h.in
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/tests/auto/blackbox/testdata/missing-dependency/theHeader.h.in
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index e1f05b534..a46d02fe5 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -3880,6 +3880,21 @@ void TestBlackbox::lrelease()
     QVERIFY(!regularFileExists(relativeProductBuildDir("lrelease-test") + "/hu.qm"));
 }
 
+void TestBlackbox::missingDependency()
+{
+    QDir::setCurrent(testDataDir + "/missing-dependency");
+    QbsRunParameters params;
+    params.expectFailure = true;
+    params.arguments << "-p" << "theApp";
+    QVERIFY(runQbs(params) != 0);
+    QVERIFY2(!m_qbsStderr.contains("ASSERT"), m_qbsStderr.constData());
+    QCOMPARE(runQbs(QbsRunParameters(QStringList() << "-p" << "theDep")), 0);
+    params.expectFailure = false;
+    params.arguments << "-vv";
+    QCOMPARE(runQbs(params), 0);
+    QVERIFY(m_qbsStderr.contains("false positive"));
+}
+
 void TestBlackbox::badInterpreter()
 {
     if (!HostOsInfo::isAnyUnixHost())
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index be4c3a8fc..22a435c07 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -160,6 +160,7 @@ private slots:
     void listPropertyOrder();
     void loadableModule();
     void lrelease();
+    void missingDependency();
     void missingProfile();
     void mixedBuildVariants();
     void multipleChanges();