Fix corner case in "up to date" check.

Currently, the timestamps of target artifacts (and only these) are
always retrieved from disk before checking whether artifacts are up to
date. This can lead to problems, for instance in the following scenario:
    - The transformer creating the target artifact has another output
artifact, i.e. the target artifact has a non-target sibling.
    - The sibling is created in a different command that runs after the
one creating the target artifact.
    - While the command creating the target artifact is in progress, a
different, unrelated command running at the same time fails.
As a result, the executor will wait until the in-flight commands have
finished and then cancel the build, not running any commands still
queued for execution. This means that the target artifact will have a
different on-disk timestamps than its sibling.
On the next build, this newer timestamp will be retrieved. Since the
current code assumes that all sibling artifacts have the same timestamp,
it only checks one random output artifact. If that happens to be the
target artifact, the up-to-date check will report that the transformer
does not need to be re-run, even though one artifact is not up to date.
This patch fixes the two closely related subtle bugs that can lead to
this behavior:
    1) Get rid of the "convenvience functionality" that always checks
the timestamp of target artifacts as a service to users that do not know
that they are not supposed to manually touch files in the build
directory. This behavior has been obsolete since the introduction of the
"--check-timestamps" option.
    2) If the "--check-timestamps" option has been given, our invariant
about all output artifacts of a transformer having the same timestamp is
not guaranteed to hold, as they come from an outside source. Therefore,
in this case we must check the timestamps of all output artifacts, not
just one.

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

2 files changed, 20 insertions, 14 deletions

diff --git a/src/lib/buildgraph/executor.cpp b/src/lib/buildgraph/executor.cpp
index 1b18c2aa6..fe3f2f907 100644
--- a/src/lib/buildgraph/executor.cpp
+++ b/src/lib/buildgraph/executor.cpp
@@ -234,13 +234,8 @@ void Executor::doBuild()
     // find the root nodes
     m_roots.clear();
     foreach (const ResolvedProductPtr &product, m_productsToBuild) {
-        foreach (Artifact *targetArtifact, product->buildData->targetArtifacts) {
+        foreach (Artifact *targetArtifact, product->buildData->targetArtifacts)
             m_roots += targetArtifact;
-
-            // The user expects that he can delete target artifacts and they get rebuilt.
-            // To achieve this we must retrieve their timestamps.
-            targetArtifact->setTimestamp(FileInfo(targetArtifact->filePath()).lastModified());
-        }
     }
 
     prepareReachableArtifacts(initialBuildState);
@@ -362,14 +357,24 @@ bool Executor::isUpToDate(Artifact *artifact) const
 
 bool Executor::mustExecuteTransformer(const TransformerPtr &transformer) const
 {
-    foreach (Artifact *artifact, transformer->outputs)
-        if (artifact->alwaysUpdated)
-            return !isUpToDate(artifact);
+    bool hasAlwaysUpdatedArtifacts = false;
+    foreach (Artifact *artifact, transformer->outputs) {
+        if (!artifact->alwaysUpdated)
+            continue;
+        hasAlwaysUpdatedArtifacts = true;
+        const bool upToDate = isUpToDate(artifact);
 
-    // All outputs of the transformer have alwaysUpdated == false.
-    // We need at least on output that is always updated.
-    QBS_CHECK(false);
-    return true;
+        // The invariant is that all output artifacts of a transformer have the same
+        // "virtual" timestamp. However, if the user requested that on-disk timestamps be evaluated,
+        // they can differ and the oldest output file of the transformer decides.
+        if (!upToDate || !m_buildOptions.forceTimestampCheck())
+            return !upToDate;
+    }
+
+    // We need at least one output that is always updated.
+    QBS_CHECK(hasAlwaysUpdatedArtifacts);
+
+    return false;
 }
 
 void Executor::buildArtifact(Artifact *artifact)
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index 481838c70..9b1b31520 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -252,7 +252,8 @@ void TestBlackbox::build_project()
     QVERIFY2(QFile::exists(productFileName), qPrintable(productFileName));
     QVERIFY(QFile::exists(buildGraphPath));
     QVERIFY2(QFile::remove(productFileName), qPrintable(productFileName));
-    QCOMPARE(runQbs(), 0);
+    waitForNewTimestamp();
+    QCOMPARE(runQbs(QbsRunParameters("--check-timestamps")), 0);
     QVERIFY2(QFile::exists(productFileName), qPrintable(productFileName));
     QVERIFY(QFile::exists(buildGraphPath));
 }