More fixes for converting missing targets -> sources.

I attempted to fix this in commit c06d1fba40, but it was apparently
incomplete and not all cases were covered by tests.

Let's add a much more thorough test by going through every possible
combination and making sure redo-{sources,targets,ood} all work as
expected, that the "you modified it" warning does or does not show up
when expected, and that dependencies are rebuilt the number of times we
expect.

builder.py

@@ -204,8 +204,6 @@ class BuildJob:
         self.basename = basename
         self.ext = ext
         self.argv = argv
-        sf.is_generated = True
-        sf.save()
         dof = state.File(name=os.path.join(dodir, dofile))
         dof.set_static()
         dof.save()
@@ -313,6 +311,9 @@ class BuildJob:
             err('...you should write status messages to stderr, not stdout.\n')
             rv = 207
         if rv==0:
+            # FIXME: race condition here between updating stamp/is_generated
+            # and actually renaming the files into place.  There needs to
+            # be some kind of two-stage commit, I guess.
             if st2:
                 try:
                     os.rename(self.tmpname2, t)

deps.py

@@ -8,7 +8,8 @@ DIRTY = 1
 def isdirty(f, depth, max_changed,
             already_checked,
             is_checked=state.File.is_checked,
-            set_checked=state.File.set_checked_save):
+            set_checked=state.File.set_checked_save,
+            log_override=state.warn_override):
     if f.id in already_checked:
         raise state.CyclicDependencyError()
     # make a copy of the list, so upon returning, our parent's copy
@@ -16,7 +17,7 @@ def isdirty(f, depth, max_changed,
     already_checked = list(already_checked) + [f.id]
 
     if vars.DEBUG >= 1:
-        debug('%s?%s\n' % (depth, f.nicename()))
+        debug('%s?%s %r,%r\n' % (depth, f.nicename(), f.is_generated, f.is_override))
 
     if f.failed_runid:
         debug('%s-- DIRTY (failed last time)\n' % depth)
@@ -25,7 +26,7 @@ def isdirty(f, depth, max_changed,
         debug('%s-- DIRTY (never built)\n' % depth)
         return DIRTY
     if f.changed_runid > max_changed:
-        debug('%s-- DIRTY (built)\n' % depth)
+        debug('%s-- DIRTY (built %d > %d; %d)\n' % (depth, f.changed_runid, max_changed, vars.RUNID))
         return DIRTY  # has been built more recently than parent
     if is_checked(f):
         if vars.DEBUG >= 1:
@@ -46,10 +47,11 @@ def isdirty(f, depth, max_changed,
                 # it a target again, but if someone creates it by hand,
                 # it'll be a source.  This should reduce false alarms when
                 # files change from targets to sources as a project evolves.
-                debug('%s   converted target -> source\n' % depth)
+                debug('%s   converted target -> source %r\n' % (depth, f.id))
-                f.is_generated = False
+                f.is_generated = f.failed_runid = 0
-                #f.update_stamp()
                 f.save()
+                f.refresh()
+                assert not f.is_generated
         else:
             debug('%s-- DIRTY (mtime)\n' % depth)
         if f.csum:
@@ -69,7 +71,9 @@ def isdirty(f, depth, max_changed,
                           max_changed = max(f.changed_runid,
                                             f.checked_runid),
                           already_checked=already_checked,
-                          is_checked=is_checked, set_checked=set_checked)
+                          is_checked=is_checked,
+                          set_checked=set_checked,
+                          log_override=log_override)
             if sub:
                 debug('%s-- DIRTY (sub)\n' % depth)
                 dirty = sub
@@ -111,6 +115,6 @@ def isdirty(f, depth, max_changed,
 
     # if we get here, it's because the target is clean
     if f.is_override:
-        state.warn_override(f.name)
+        log_override(f.name)
     set_checked(f)
     return CLEAN

redo-ood.py

@@ -14,17 +14,27 @@ if len(sys.argv[1:]) != 0:
 
 cache = {}
 
+
 def is_checked(f):
     return cache.get(f.id, 0)
 
+
 def set_checked(f):
     cache[f.id] = 1
 
 
+def log_override(name):
+    pass
+
+
 cwd = os.getcwd()
 for f in state.files():
-    if f.is_generated and f.read_stamp() != state.STAMP_MISSING:
+    if f.is_target():
-        if deps.isdirty(f, depth='', max_changed=vars.RUNID,
+        if deps.isdirty(f,
+                        depth='',
+                        max_changed=vars.RUNID,
                         already_checked=[],
-                        is_checked=is_checked, set_checked=set_checked):
+                        is_checked=is_checked,
+                        set_checked=set_checked,
+                        log_override=log_override):
             print state.relpath(os.path.join(vars.BASE, f.name), cwd)

redo-sources.py

@@ -13,7 +13,5 @@ if len(sys.argv[1:]) != 0:
 
 cwd = os.getcwd()
 for f in state.files():
-    if f.name.startswith('//'):
+    if f.is_source():
-        continue  # special name, ignore
-    if not f.is_generated and f.read_stamp() != state.STAMP_MISSING:
         print state.relpath(os.path.join(vars.BASE, f.name), cwd)

redo-targets.py

@@ -13,5 +13,5 @@ if len(sys.argv[1:]) != 0:
 
 cwd = os.getcwd()
 for f in state.files():
-    if f.is_generated and f.read_stamp() != state.STAMP_MISSING:
+    if f.is_target():
         print state.relpath(os.path.join(vars.BASE, f.name), cwd)

state.py

@@ -299,7 +299,18 @@ class File(object):
         debug2('FAILED: %r\n' % self.name)
         self.update_stamp()
         self.failed_runid = vars.RUNID
-        self.is_generated = True
+        if self.stamp != STAMP_MISSING:
+            # if we failed and the target file still exists,
+            # then we're generated.
+            self.is_generated = True
+        else:
+            # if the target file now does *not* exist, then go back to
+            # treating this as a source file.  Since it doesn't exist,
+            # if someone tries to rebuild it immediately, it'll go
+            # back to being a target.  But if the file is manually
+            # created before that, we don't need a "manual override"
+            # warning.
+            self.is_generated = False
 
     def set_static(self):
         self.update_stamp(must_exist=True)
@@ -321,6 +332,30 @@ class File(object):
             self.stamp = newstamp
             self.set_changed()
 
+    def is_source(self):
+        if self.name.startswith('//'):
+            return False  # special name, ignore
+        newstamp = self.read_stamp()
+        if (self.is_generated and
+            (not self.is_failed() or newstamp != STAMP_MISSING) and
+            not self.is_override and
+            self.stamp == newstamp):
+            # target is as we left it
+            return False
+        if ((not self.is_generated or self.stamp != newstamp) and
+            newstamp == STAMP_MISSING):
+            # target has gone missing after the last build.
+            # It's not usefully a source *or* a target.
+            return False
+        return True
+
+    def is_target(self):
+        if not self.is_generated:
+            return False
+        if self.is_source():
+            return False
+        return True
+
     def is_checked(self):
         return self.checked_runid and self.checked_runid >= vars.RUNID
 
@@ -331,6 +366,8 @@ class File(object):
         return self.failed_runid and self.failed_runid >= vars.RUNID
 
     def deps(self):
+        if self.is_override or not self.is_generated:
+            return
         q = ('select Deps.mode, Deps.source, %s '
              '  from Files '
              '    join Deps on Files.rowid = Deps.source '

t/351-deps-forget/.gitignore

@@ -1,6 +1,7 @@
 bork
-bork.do
 bork.log
 sub
 sub.log
-targets.out
+sub.warn
+silent.out
+want

t/351-deps-forget/all.do

@@ -1,55 +1,211 @@
 exec >&2
-rm -f bork.do bork bork.log sub sub.log targets.out
+rm -f want silent.out bork bork.log sub sub.log sub.warn
 
+# Run a command without displaying its output.
+# We are intentionally generating some redo errors, and
+# we don't want the log to look scary.
+# In case we need the output to debug a failed test,
+# we leave the most recent command output in silent.out.
+silent() {
+	"$@" >silent.out 2>&1
+}
+
+# like "grep -q", but portable.
 qgrep() {
-	# like "grep -q", but portable
 	grep "$@" >/dev/null
 }
 
-cp bork.do.in bork.do
+# Returns true if bork is marked as a redo-source.
-redo bork
+is_source() {
-[ "$(cat bork.log)" = "x" ] || exit 2
+	redo-sources | qgrep '^bork$'
-redo bork
+}
-[ "$(cat bork.log)" = "xx" ] || exit 3
 
-redo-ifchange sub
+# Returns true if bork is marked as a redo-target.
-[ "$(cat bork.log)" = "xx" ] || exit 10
+is_target() {
-[ "$(cat sub.log)" = "y" ] || exit 11
+	redo-targets | qgrep '^bork$'
-. ../skip-if-minimal-do.sh
+}
-redo-sources | qgrep '^bork$' && exit 12
-redo-targets | tee targets.out | qgrep '^bork$' || exit 13
 
-# Might as well test redo-ood while we're here
+# Returns true if bork is marked as an out-of-date redo-target.
-../flush-cache
+is_ood() {
-redo bork
+	redo-ood | qgrep '^bork$'
-redo-targets | qgrep '^bork$' || exit 15
+}
-redo-targets | qgrep '^sub$'  || exit 16
-redo-ood     | qgrep '^sub$'  || exit 17
 
-redo-ifchange sub
-[ "$(cat bork.log)" = "xxx" ] || exit 18
-[ "$(cat sub.log)" = "yy" ] || exit 19
 
-rm -f bork
+# The table for our table-driven test.
-../flush-cache
+# Column meanings are:
-redo-ifchange sub  # rebuilds, and sub.do drops dependency on bork
+#   pre: the state of the 'bork' file at test start
-[ "$(cat bork.log)" = "xxx" ] || exit 20
+#     src = 'bork' is a redo-source
-[ "$(cat sub.log)" = "yyy" ] || exit 21
+#     nil = 'bork' is a redo-target that produced nil (ie. a virtual target)
-redo-sources | qgrep '^bork$' && exit 22  # nonexistent, so not a source
+#     add = 'bork' is a redo-target that produced a file
-redo-targets | qgrep '^bork$' && exit 23  # deleted; not a target anymore
+#   update: the override to perform after 'pre'
+#     nop = do nothing
+#     del = delete 'bork', if it exists
+#     add = create/override a new 'bork'
+#   post: the behaviour requested from bork.do after 'pre' and 'update' finish
+#     err = bork.do exits with an error
+#     nil = bork.do produces nil (ie. a virtual target)
+#     add = bork.do produces a file
+#   subran: 'ran' if sub.do is expected to pass, else 'skip'
+#   ran: 'ran' if bork.do is expected to run at all, else 'skip'
+#   warn: 'warn' if 'redo bork' is expected to warn about overrides, else 'no'
+#   src/targ/ood: 1 if bork should show up in source/target/ood output, else 0
+truth="
+	# File was initially a source file
+	src nop err   skip skip   no   1 0 0
+	src nop nil   skip skip   no   1 0 0
+	src nop add   skip skip   no   1 0 0
 
-echo static >bork
+	src del err   skip ran    no   0 0 0   # content deleted
-../flush-cache
+	src del nil   ran  ran    no   0 1 0
-redo-ifchange sub  # doesn't depend on bork anymore, so doesn't rebuild
+	src del add   ran  ran    no   0 1 0
-[ "$(cat bork.log)" = "xxx" ] || exit 30
-[ "$(cat sub.log)" = "yyy" ] || exit 31
 
-# bork should now be considered static, so no warning or need to rebuild.
+	src add err   skip skip   no   1 0 0   # source updated
-# It should now be considered a source, not a target.
+	src add nil   skip skip   no   1 0 0
-redo sub  # force rebuild; sub.do now declares dependency on bork
+	src add add   skip skip   no   1 0 0
-[ "$(cat bork.log)" = "xxx" ] || exit 40
+
-[ "$(cat sub.log)" = "yyyy" ] || exit 41
+	# File was initially a target that produced nil
-redo-sources | qgrep '^bork$' || exit 42
+	nil nop err   skip ran    no   0 0 0   # content forgotten
-redo-targets | qgrep '^bork$' && exit 43
+	nil nop nil   ran  ran    no   0 1 0
+	nil nop add   ran  ran    no   0 1 0
+
+	nil del err   skip ran    no   0 0 0   # content nonexistent
+	nil del nil   ran  ran    no   0 1 0
+	nil del add   ran  ran    no   0 1 0
+
+	nil add err   skip skip   warn 1 0 0   # content overridden
+	nil add nil   skip skip   warn 1 0 0
+	nil add add   skip skip   warn 1 0 0
+
+	# File was initially a target that produced output
+	add nop err   skip ran    no   0 1 1   # update failed
+	add nop nil   ran  ran    no   0 1 0
+	add nop add   ran  ran    no   0 1 0
+
+	add del err   skip ran    no   0 0 0   # content nonexistent
+	add del nil   ran  ran    no   0 1 0
+	add del add   ran  ran    no   0 1 0
+
+	add add err   skip skip   warn 1 0 0   # content overridden
+	add add nil   skip skip   warn 1 0 0
+	add add add   skip skip   warn 1 0 0
+"
+
+echo "$truth" |
+while read pre update post   subran ran   warn   src targ ood XX; do
+	[ "$pre" != "" -a "$pre" != "#" ] || continue
+
+	# add some helpful vertical whitespace between rows when
+	# using 'redo -x'
+	:
+	:
+	:
+	:
+	echo "test: $pre $update $post"
+	rm -f bork
+
+	# Step 1 does the requested 'pre' operation.
+	: STEP 1
+	../flush-cache
+	case $pre in
+	  src)
+		# This is a little convoluted because we need to convince
+		# redo to forget 'bork' may have previously been known as a
+		# target.  To make it work, we have to let redo see the file
+		# at least once as "should be existing, but doesn't."  That
+		# will mark is as no longer a target.  Then we can create the
+		# file from outside redo.
+		rm -f bork
+		echo err >want
+		# Now redo will ack the nonexistent file, but *not* create
+		# it, because bork.do will exit with an error.
+		silent redo-ifchange bork || true
+		# Make sure redo is really sure the file is not a target
+		! is_target || exit 13
+		# Manually create the source file and ensure redo knows it's
+		# a source, and hasn't magically turned back into a target.
+		echo src >>bork
+		is_source || exit 11
+		! is_target || exit 12
+		;;
+	  nil)
+		echo nil >want
+		redo bork
+		! is_source || exit 11
+		is_target || exit 12
+		;;
+	  add)
+		echo add >want
+		redo bork
+		! is_source || exit 11
+		is_target || exit 12
+		;;
+	  *) exit 90 ;;
+	esac
+
+	# Step 2 does the requested 'update' operation.
+	: STEP 2
+	skip=
+	case $update in
+	  nop) ;;
+	  del) rm -f bork; skip=1 ;;
+	  add) echo override >>bork ;;
+	  *) exit 91 ;;
+	esac
+
+	../flush-cache
+	if [ -z "$skip" ]; then
+		silent redo sub
+	fi
+
+	# Step 3 does the requested 'post' operation.
+	: STEP 3
+	../flush-cache
+	: >bork.log
+	: >sub.log
+	echo "$post" >want
+	redo-ifchange sub >sub.warn 2>&1 || true
+
+	read blog <bork.log || true
+	case $ran in
+	  skip) want_blog='' ;;
+	  ran)  want_blog='x' ;;
+	esac
+	[ "$blog" = "$want_blog" ] || exit 21
+
+	read slog <sub.log || true
+	case $subran in
+	  skip) want_slog='' ;;
+	  ran)  want_slog='y' ;;
+	esac
+	[ "$slog" = "$want_slog" ] || exit 22
+
+	if [ "$src" = 1 ]; then
+		is_source || exit 31
+	else
+		! is_source || exit 32
+	fi
+
+	if [ "$targ" = 1 ]; then
+		is_target || exit 33
+	else
+		! is_target || exit 34
+	fi
+
+	if [ "$ood" = 1 ]; then
+		is_ood || exit 35
+	else
+		! is_ood || exit 36
+	fi
+
+	# FIXME: I'd like to not depend on the specific wording of warning
+	# messages here.  However, the whole point of the warning message
+	# is that it doesn't affect behaviour (or else it would be an error,
+	# not a warning).
+	if [ "$warn" = "warn" ]; then
+		qgrep "you modified it" sub.warn || exit 51
+	else
+		! qgrep "you modified it" sub.warn || exit 52
+	fi
+done
 
 exit 0

t/351-deps-forget/bork.do

@@ -0,0 +1,11 @@
+printf x >>$1.log
+redo-always
+
+read want <want
+
+case $want in
+  err) exit 10 ;;
+  nil) exit 0 ;;
+  add) printf x >>$3; exit 0 ;;
+  *) exit 80 ;;
+esac

t/351-deps-forget/bork.do.in

@@ -1,2 +0,0 @@
-echo bork
-printf x >>$1.log

t/351-deps-forget/clean.do

@@ -1,2 +1 @@
-rm -f *~ .*~ bork bork.do bork.log sub sub.log targets.out
+rm -f *~ .*~ want bork bork.log sub sub.log sub.warn silent.out
-

t/351-deps-forget/sub.do

@@ -1,3 +1,4 @@
-[ -e bork ] && redo-ifchange bork
+redo-ifchange bork
 echo sub
+echo sub >&2
 printf y >>$1.log