Fix "build unrelated targets on error" mode

With the /k option given, jom tried to build all targets, even
targets that depended on failed targets. Thus it behaved the same as
/i (ignore exit codes), which is wrong.

Task-number: QTCREATORBUG-17131
Change-Id: I1dcbabbf423042de44222a600f7a1b8be1203f45
Reviewed-by: Oliver Wolff <oliver.wolff@qt.io>

6 files changed, 90 insertions, 13 deletions

diff --git a/src/jomlib/dependencygraph.cpp b/src/jomlib/dependencygraph.cpp
index 382a13b..bf8cfbc 100644
--- a/src/jomlib/dependencygraph.cpp
+++ b/src/jomlib/dependencygraph.cpp
@@ -82,6 +82,24 @@ void DependencyGraph::build(DescriptionBlock* target)
     //qDebug() << "\nFINISHED";
 }
 
+void DependencyGraph::markParentsRecursivlyUnbuildable(DescriptionBlock *target)
+{
+    markParentsRecursivlyUnbuildable(m_nodeContainer.value(target));
+}
+
+bool DependencyGraph::isUnbuildable(DescriptionBlock *target) const
+{
+    return m_nodeContainer.value(target)->state == Node::Unbuildable;
+}
+
+void DependencyGraph::markParentsRecursivlyUnbuildable(Node *node)
+{
+    foreach (Node *parent, node->parents) {
+        parent->state = Node::Unbuildable;
+        markParentsRecursivlyUnbuildable(parent);
+    }
+}
+
 bool DependencyGraph::isTargetUpToDate(DescriptionBlock* target)
 {
     FastFileInfo fi(target->targetName());
@@ -313,7 +331,8 @@ DescriptionBlock *DependencyGraph::findAvailableTarget(bool ignoreTimeStamps)
     // return the first leaf that is not currently executed
     foreach (Node *leaf, m_leaves) {
         if (leaf->state != Node::ExecutingState) {
-            leaf->state = Node::ExecutingState;
+            if (leaf->state != Node::Unbuildable)
+                leaf->state = Node::ExecutingState;
             displayNodeBuildInfo(leaf, ignoreTimeStamps ? isTargetUpToDate(leaf->target) : false);
             return leaf->target;
         }
diff --git a/src/jomlib/dependencygraph.h b/src/jomlib/dependencygraph.h
index 89bc782..2ed92fc 100644
--- a/src/jomlib/dependencygraph.h
+++ b/src/jomlib/dependencygraph.h
@@ -40,6 +40,8 @@ public:
     ~DependencyGraph();
 
     void build(DescriptionBlock* target);
+    void markParentsRecursivlyUnbuildable(DescriptionBlock *target);
+    bool isUnbuildable(DescriptionBlock *target) const;
     bool isEmpty() const;
     void removeLeaf(DescriptionBlock* target);
     DescriptionBlock *findAvailableTarget(bool ignoreTimeStamps);
@@ -52,7 +54,7 @@ private:
 
     struct Node
     {
-        enum State {UnknownState, ExecutingState};
+        enum State {UnknownState, ExecutingState, Unbuildable};
 
         State state;
         DescriptionBlock* target;
@@ -68,6 +70,7 @@ private:
     void internalDump(Node* node, QString& indent);
     void internalDotDump(Node* node, const QString& parent);
     void displayNodeBuildInfo(Node* node, bool isUpToDate);
+    static void markParentsRecursivlyUnbuildable(Node *node);
 
 private:
     Node* m_root;
diff --git a/src/jomlib/targetexecutor.cpp b/src/jomlib/targetexecutor.cpp
index 0f20d62..9345939 100644
--- a/src/jomlib/targetexecutor.cpp
+++ b/src/jomlib/targetexecutor.cpp
@@ -203,18 +203,35 @@ void TargetExecutor::findNextTarget()
 {
     forever {
         m_nextTarget = m_depgraph->findAvailableTarget(m_makefile->options()->buildAllTargets);
-        if (m_nextTarget && m_nextTarget->m_commands.isEmpty()) {
-            // Short cut for targets without commands.
-            m_depgraph->removeLeaf(m_nextTarget);
-        } else {
-            return;
+        if (m_nextTarget) {
+            if (m_nextTarget->m_commands.isEmpty()) {
+                // Short cut for targets without commands.
+                m_depgraph->removeLeaf(m_nextTarget);
+                continue;
+            } else if (m_makefile->options()->buildUnrelatedTargetsOnError
+                       && m_depgraph->isUnbuildable(m_nextTarget)) {
+                fprintf(stderr, "jom: Target '%s' cannot be built due to failed dependencies.\n",
+                        qPrintable(m_nextTarget->targetName()));
+                m_depgraph->removeLeaf(m_nextTarget);
+                continue;
+            }
         }
+        return;
     }
 }
 
 void TargetExecutor::onChildFinished(CommandExecutor* executor, bool commandFailed)
 {
     Q_CHECK_PTR(executor->target());
+    if (commandFailed) {
+        m_allCommandsSuccessfullyExecuted = false;
+        if (m_makefile->options()->buildUnrelatedTargetsOnError) {
+            // Recursively mark all parents of this node as unbuildable due to unsatisfied
+            // dependencies. This must happen before removing the node from the build graph.
+            m_depgraph->markParentsRecursivlyUnbuildable(executor->target());
+            fputs("jom: Option /K specified. Continuing.\n", stderr);
+        }
+    }
     FastFileInfo::clearCacheForFile(executor->target()->targetName());
     m_depgraph->removeLeaf(executor->target());
     if (m_jobAcquisitionCount > 0) {
@@ -235,9 +252,6 @@ void TargetExecutor::onChildFinished(CommandExecutor* executor, bool commandFail
             m_availableProcesses.first()->setBufferedOutput(false);
     }
 
-    if (commandFailed)
-        m_allCommandsSuccessfullyExecuted = false;
-
     bool abortMakeProcess = commandFailed && !m_makefile->options()->buildUnrelatedTargetsOnError;
     if (abortMakeProcess) {
         m_bAborted = true;
diff --git a/tests/makefiles/blackbox/buildUnrelatedTargetsOnError/test.mk b/tests/makefiles/blackbox/buildUnrelatedTargetsOnError/test.mk
new file mode 100644
index 0000000..9441397
--- /dev/null
+++ b/tests/makefiles/blackbox/buildUnrelatedTargetsOnError/test.mk
@@ -0,0 +1,13 @@
+# test the /k option
+# When running "jom /f test.mk /k" the target "dependsOnFailingTarget" must not be built.
+
+first: workingTarget dependsOnFailingTarget
+
+failingTarget:
+    cmd /c exit 7
+
+dependsOnFailingTarget: failingTarget
+    @echo We should not see this.
+
+workingTarget:
+    @echo Yay! This always works!
diff --git a/tests/tests.cpp b/tests/tests.cpp
index a43fa6f..668baeb 100644
--- a/tests/tests.cpp
+++ b/tests/tests.cpp
@@ -38,6 +38,7 @@
 #include <options.h>
 #include <exception.h>
 
+#include <algorithm>
 #include <limits>
 
 using namespace NMakeFile;
@@ -928,7 +929,8 @@ void Tests::windowsPathsInTargetName()
 /**
  * Note: this function clears the environment of m_jomProcess after every start.
  */
-bool Tests::runJom(const QStringList &args, const QString &workingDirectory)
+bool Tests::runJom(const QStringList &args, const QString &workingDirectory,
+                   QProcess::ProcessChannelMode channelMode)
 {
 #ifdef _DEBUG
     const QLatin1String jomBinaryName("jomd.exe");
@@ -947,7 +949,7 @@ bool Tests::runJom(const QStringList &args, const QString &workingDirectory)
         oldWorkingDirectory = QDir::currentPath();
         QDir::setCurrent(workingDirectory);
     }
-    m_jomProcess->setProcessChannelMode(QProcess::MergedChannels);
+    m_jomProcess->setProcessChannelMode(channelMode);
     m_jomProcess->start(jomBinary, args);
     bool success = true;
     if (!m_jomProcess->waitForStarted()) {
@@ -1020,6 +1022,29 @@ void Tests::touchFile(const QString &fileName)
     file.resize(s);
 }
 
+QList<QByteArray> Tests::splitOutput(const QByteArray &output)
+{
+    QList<QByteArray> result = output.split('\n');
+    std::for_each(result.begin(), result.end(), [] (QByteArray &a) { a = a.trimmed(); });
+    return result;
+}
+
+void Tests::buildUnrelatedTargetsOnError()
+{
+    QVERIFY(runJom(QStringList() << "/f" << "test.mk" << "/nologo" << "/k",
+                   "blackbox/buildUnrelatedTargetsOnError", QProcess::SeparateChannels));
+    QCOMPARE(m_jomProcess->exitCode(), 1);
+    const QByteArray out = m_jomProcess->readAllStandardOutput();
+    QVERIFY(out.contains("Yay! This always works!"));
+    QVERIFY(!out.contains("We should not see this."));
+    const QList<QByteArray> err = splitOutput(m_jomProcess->readAllStandardError());
+    QVERIFY(std::find_if(err.begin(), err.end(), [] (const QByteArray &line)
+                { return line.endsWith("[failingTarget] Error 7"); }) != err.end());
+    QVERIFY(err.contains("jom: Option /K specified. Continuing."));
+    QVERIFY(err.contains("jom: Target 'dependsOnFailingTarget' "
+                         "cannot be built due to failed dependencies."));
+}
+
 void Tests::caseInsensitiveDependents()
 {
     QVERIFY(runJom(QStringList() << "/f" << "test.mk" << "/nologo", "blackbox/caseInsensitiveDependents"));
diff --git a/tests/tests.h b/tests/tests.h
index 282486a..fe33f20 100644
--- a/tests/tests.h
+++ b/tests/tests.h
@@ -67,6 +67,7 @@ private slots:
     void windowsPathsInTargetName();
 
     // black-box tests
+    void buildUnrelatedTargetsOnError();
     void caseInsensitiveDependents();
     void environmentVariables_data();
     void environmentVariables();
@@ -84,10 +85,12 @@ private slots:
 
 private:
     bool openMakefile(const QString& fileName);
-    bool runJom(const QStringList &args, const QString &workingDirectory = QString());
+    bool runJom(const QStringList &args, const QString &workingDirectory = QString(),
+                QProcess::ProcessChannelMode channelMode = QProcess::MergedChannels);
     bool fileContentsEqual(const QString& fileName1, const QString& fileName2);
     QStringList readJomStdOutput();
     void touchFile(const QString &fileName);
+    static QList<QByteArray> splitOutput(const QByteArray &output);
 
 private:
     QString m_oldCurrentPath;