From 2b0d34f0ed5b9b11668199796396d825a46b9bf5 Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Sun, 2 Dec 2018 16:53:05 -0500 Subject: [PATCH] More fixes for converting missing targets -> sources. I attempted to fix this in commit c06d1fba4013, 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 | 5 +- deps.py | 20 +-- redo-ood.py | 16 ++- redo-sources.py | 4 +- redo-targets.py | 2 +- state.py | 39 +++++- t/351-deps-forget/.gitignore | 5 +- t/351-deps-forget/all.do | 238 +++++++++++++++++++++++++++++------ t/351-deps-forget/bork.do | 11 ++ t/351-deps-forget/bork.do.in | 2 - t/351-deps-forget/clean.do | 3 +- t/351-deps-forget/sub.do | 3 +- 12 files changed, 282 insertions(+), 66 deletions(-) create mode 100644 t/351-deps-forget/bork.do delete mode 100644 t/351-deps-forget/bork.do.in diff --git a/builder.py b/builder.py index 46109c3..5c4bd69 100644 --- a/builder.py +++ b/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) diff --git a/deps.py b/deps.py index e803870..5229996 100644 --- a/deps.py +++ b/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) - f.is_generated = False - #f.update_stamp() + debug('%s converted target -> source %r\n' % (depth, f.id)) + f.is_generated = f.failed_runid = 0 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 diff --git a/redo-ood.py b/redo-ood.py index be0016f..81c9c0b 100755 --- a/redo-ood.py +++ b/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 deps.isdirty(f, depth='', max_changed=vars.RUNID, + if f.is_target(): + 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) diff --git a/redo-sources.py b/redo-sources.py index 2e19684..790b9c3 100755 --- a/redo-sources.py +++ b/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('//'): - continue # special name, ignore - if not f.is_generated and f.read_stamp() != state.STAMP_MISSING: + if f.is_source(): print state.relpath(os.path.join(vars.BASE, f.name), cwd) diff --git a/redo-targets.py b/redo-targets.py index 95ef5c9..f0db6e9 100755 --- a/redo-targets.py +++ b/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) diff --git a/state.py b/state.py index d95c041..e2db20c 100644 --- a/state.py +++ b/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 ' diff --git a/t/351-deps-forget/.gitignore b/t/351-deps-forget/.gitignore index d51b82d..692127e 100644 --- a/t/351-deps-forget/.gitignore +++ b/t/351-deps-forget/.gitignore @@ -1,6 +1,7 @@ bork -bork.do bork.log sub sub.log -targets.out +sub.warn +silent.out +want diff --git a/t/351-deps-forget/all.do b/t/351-deps-forget/all.do index a799cec..b72d44c 100644 --- a/t/351-deps-forget/all.do +++ b/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 -redo bork -[ "$(cat bork.log)" = "x" ] || exit 2 -redo bork -[ "$(cat bork.log)" = "xx" ] || exit 3 +# Returns true if bork is marked as a redo-source. +is_source() { + redo-sources | qgrep '^bork$' +} -redo-ifchange sub -[ "$(cat bork.log)" = "xx" ] || exit 10 -[ "$(cat sub.log)" = "y" ] || exit 11 -. ../skip-if-minimal-do.sh -redo-sources | qgrep '^bork$' && exit 12 -redo-targets | tee targets.out | qgrep '^bork$' || exit 13 +# Returns true if bork is marked as a redo-target. +is_target() { + redo-targets | qgrep '^bork$' +} -# Might as well test redo-ood while we're here -../flush-cache -redo bork -redo-targets | qgrep '^bork$' || exit 15 -redo-targets | qgrep '^sub$' || exit 16 -redo-ood | qgrep '^sub$' || exit 17 +# Returns true if bork is marked as an out-of-date redo-target. +is_ood() { + redo-ood | qgrep '^bork$' +} -redo-ifchange sub -[ "$(cat bork.log)" = "xxx" ] || exit 18 -[ "$(cat sub.log)" = "yy" ] || exit 19 -rm -f bork -../flush-cache -redo-ifchange sub # rebuilds, and sub.do drops dependency on bork -[ "$(cat bork.log)" = "xxx" ] || exit 20 -[ "$(cat sub.log)" = "yyy" ] || exit 21 -redo-sources | qgrep '^bork$' && exit 22 # nonexistent, so not a source -redo-targets | qgrep '^bork$' && exit 23 # deleted; not a target anymore +# The table for our table-driven test. +# Column meanings are: +# pre: the state of the 'bork' file at test start +# src = 'bork' is a redo-source +# nil = 'bork' is a redo-target that produced nil (ie. a virtual target) +# add = 'bork' is a redo-target that produced a file +# 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 -../flush-cache -redo-ifchange sub # doesn't depend on bork anymore, so doesn't rebuild -[ "$(cat bork.log)" = "xxx" ] || exit 30 -[ "$(cat sub.log)" = "yyy" ] || exit 31 + src del err skip ran no 0 0 0 # content deleted + src del nil ran ran no 0 1 0 + src del add ran ran no 0 1 0 -# bork should now be considered static, so no warning or need to rebuild. -# It should now be considered a source, not a target. -redo sub # force rebuild; sub.do now declares dependency on bork -[ "$(cat bork.log)" = "xxx" ] || exit 40 -[ "$(cat sub.log)" = "yyyy" ] || exit 41 -redo-sources | qgrep '^bork$' || exit 42 -redo-targets | qgrep '^bork$' && exit 43 + src add err skip skip no 1 0 0 # source updated + src add nil skip skip no 1 0 0 + src add add skip skip no 1 0 0 + + # File was initially a target that produced nil + nil nop err skip ran no 0 0 0 # content forgotten + 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 >$1.log +redo-always + +read want >$3; exit 0 ;; + *) exit 80 ;; +esac diff --git a/t/351-deps-forget/bork.do.in b/t/351-deps-forget/bork.do.in deleted file mode 100644 index 29d4871..0000000 --- a/t/351-deps-forget/bork.do.in +++ /dev/null @@ -1,2 +0,0 @@ -echo bork -printf x >>$1.log diff --git a/t/351-deps-forget/clean.do b/t/351-deps-forget/clean.do index 5caef9d..8625eb4 100644 --- a/t/351-deps-forget/clean.do +++ b/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 diff --git a/t/351-deps-forget/sub.do b/t/351-deps-forget/sub.do index 5747903..1ae26f3 100644 --- a/t/351-deps-forget/sub.do +++ b/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