From 711b05766f277a6acb4eb22038920543c879087e Mon Sep 17 00:00:00 2001
From: Avery Pennarun <apenwarr@gmail.com>
Date: Tue, 30 Oct 2018 23:03:46 -0400
Subject: [PATCH] Print a better message when detecting pre-existing cyclic
 dependencies.

We already printed an error at build time, but added the broken
dependency anyway.  If the .do script decided to succeed despite
redo-ifchange aborting, the target would be successfully created
and we'd end up with an infinite loop when running isdirty() later.

The result was still "correct", because python helpfully aborted
the infinite loop after the recursion got too deep.  But let's
explicitly detect it and print a better error message.

(Thanks to Nils Dagsson Moskopp's redo-testcases repo for exposing this
problem.  If you put a #!/bin/sh header on your .do script means you
need to run 'set -e' yourself if you want .do scripts to abort after an
error, which you almost always do, and those testcases don't, which
exposed this bug if you ran the tests twice.)
---
 builder.py       | 6 +++++-
 deps.py          | 8 ++++++++
 redo-ifchange.py | 3 ++-
 redo-ood.py      | 1 +
 state.py         | 2 +-
 5 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builder.py b/builder.py
index 6240435..a7d4294 100644
--- a/builder.py
+++ b/builder.py
@@ -86,7 +86,11 @@ class BuildJob:
     def start(self):
         assert(self.lock.owned)
         try:
-            dirty = self.shouldbuildfunc(self.t)
+            try:
+                dirty = self.shouldbuildfunc(self.t)
+            except state.CyclicDependencyError:
+                err('cyclic dependency while checking %s\n' % _nice(self.t))
+                raise ImmediateReturn(208)
             if not dirty:
                 # target doesn't need to be built; skip the whole task
                 return self._after2(0)
diff --git a/deps.py b/deps.py
index 173cd72..dd658c5 100644
--- a/deps.py
+++ b/deps.py
@@ -6,8 +6,15 @@ CLEAN = 0
 DIRTY = 1
 
 def isdirty(f, depth, max_changed,
+            already_checked,
             is_checked=state.File.is_checked,
             set_checked=state.File.set_checked_save):
+    if f.id in already_checked:
+        raise state.CyclicDependencyError()
+    # make a copy of the list, so upon returning, our parent's copy
+    # is unaffected
+    already_checked = list(already_checked) + [f.id]
+
     if vars.DEBUG >= 1:
         debug('%s?%s\n' % (depth, f.nicename()))
 
@@ -50,6 +57,7 @@ def isdirty(f, depth, max_changed,
             sub = isdirty(f2, depth = depth + '  ',
                           max_changed = max(f.changed_runid,
                                             f.checked_runid),
+                          already_checked=already_checked,
                           is_checked=is_checked, set_checked=set_checked)
             if sub:
                 debug('%s-- DIRTY (sub)\n' % depth)
diff --git a/redo-ifchange.py b/redo-ifchange.py
index c1345c6..ee6dd83 100755
--- a/redo-ifchange.py
+++ b/redo-ifchange.py
@@ -12,7 +12,8 @@ def should_build(t):
     f = state.File(name=t)
     if f.is_failed():
         raise builder.ImmediateReturn(32)
-    dirty = deps.isdirty(f, depth = '', max_changed = vars.RUNID)
+    dirty = deps.isdirty(f, depth = '', max_changed = vars.RUNID,
+                         already_checked=[])
     return dirty==[f] and deps.DIRTY or dirty
 
 
diff --git a/redo-ood.py b/redo-ood.py
index 433d7ea..250aad1 100755
--- a/redo-ood.py
+++ b/redo-ood.py
@@ -24,5 +24,6 @@ def set_checked(f):
 for f in state.files():
     if f.is_generated and f.read_stamp() != state.STAMP_MISSING:
         if deps.isdirty(f, depth='', max_changed=vars.RUNID,
+                        already_checked=[],
                         is_checked=is_checked, set_checked=set_checked):
             print f.nicename()
diff --git a/state.py b/state.py
index 7f24728..dd490db 100644
--- a/state.py
+++ b/state.py
@@ -365,7 +365,7 @@ class Lock:
         assert(not self.owned)
         if str(self.fid) in vars.get_locks():
             # Lock already held by parent: cyclic dependence
-            raise CyclicDependencyError
+            raise CyclicDependencyError()
         fcntl.lockf(self.lockfile, fcntl.LOCK_EX, 0, 0)
         self.owned = True