From 8a0effa5b3d216eea23d0c87de5b93bc3b82e8dd Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Sun, 24 Feb 2019 22:36:47 -0500 Subject: [PATCH 1/7] minimal/do: remove dependency on 'seq' command. It was not available in older versions of FreeBSD and MacOS. Reported-by: Wayne Scott --- minimal/do | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/minimal/do b/minimal/do index 9c25919..bd78473 100755 --- a/minimal/do +++ b/minimal/do @@ -4,7 +4,7 @@ # For the full version, visit http://github.com/apenwarr/redo # # The author disclaims copyright to this source file and hereby places it in -# the public domain. (2010 12 14; updated 2018 12 13) +# the public domain. (2010 12 14; updated 2019 02 24) # USAGE=" usage: do [-d] [-x] [-v] [-c] @@ -28,16 +28,23 @@ if [ -n "$TERM" -a "$TERM" != "dumb" ] && tty <&2 >/dev/null 2>&1; then plain="$(printf '\033[m')" fi +# The 'seq' command is not available on all platforms. +_seq() { + local x=0 max="$1" + while [ "$x" -lt "$max" ]; do + x=$((x + 1)) + echo "$x" + done +} + # Split $1 into a dir part ($_dirsplit_dir) and base filename ($_dirsplit_base) -_dirsplit() -{ +_dirsplit() { _dirsplit_base=${1##*/} _dirsplit_dir=${1%$_dirsplit_base} } # Like /usr/bin/dirname, but avoids a fork and uses _dirsplit semantics. -qdirname() -( +qdirname() ( _dirsplit "$1" dir=${_dirsplit_dir%/} echo "${dir:-.}" @@ -214,7 +221,7 @@ _realpath() path="${relto%/}/$path" fi ( - for d in $(seq 100); do + for d in $(_seq 100); do #echo "Trying: $PWD--$path" >&2 if cd -P "$path" 2>/dev/null; then # success @@ -269,7 +276,7 @@ _find_dofiles() dofile=$_dirsplit_base [ -n "$dodir" ] && dodir=${dodir%/}/ [ -e "$dodir$dofile" ] && return 0 - for i in $(seq 100); do + for i in $(_seq 100); do [ -n "$dodir" ] && dodir=${dodir%/}/ #echo "_find_dofiles: '$dodir' '$dofile'" >&2 _find_dofiles_pwd "$dodir" "$dofile" && return 0 From 8100aa4973137513f892a53e2838382445100600 Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Sat, 2 Mar 2019 02:13:18 -0500 Subject: [PATCH 2/7] Certain redo post-build failures would still mark a target as built. If we failed because: - target dir doesn't exist - failed to copy from stdout - failed to rename $3 We would correctly return error 209, but the target would still be marked as having been built, so redo-ifchange would not try to build it next time, beause we forgot to call sf.set_failed() in those cases. minimal/do worked correctly. Added a test to catch this in the future. --- redo/builder.py | 3 ++- t/122-defaults-parent/.gitignore | 1 + t/122-defaults-parent/all.do | 22 ++++++++++++++++++++++ t/122-defaults-parent/clean.do | 2 ++ t/122-defaults-parent/inner/default.do | 2 ++ t/122-defaults-parent/x/file | 0 6 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 t/122-defaults-parent/.gitignore create mode 100644 t/122-defaults-parent/all.do create mode 100644 t/122-defaults-parent/clean.do create mode 100644 t/122-defaults-parent/inner/default.do create mode 100644 t/122-defaults-parent/x/file diff --git a/redo/builder.py b/redo/builder.py index b226110..0fb24a8 100644 --- a/redo/builder.py +++ b/redo/builder.py @@ -419,7 +419,8 @@ class _BuildJob(object): sf.csum = None sf.update_stamp() sf.set_changed() - else: + # rv might have changed up above + if rv: helpers.unlink(self.tmpname) sf = self.sf sf.set_failed() diff --git a/t/122-defaults-parent/.gitignore b/t/122-defaults-parent/.gitignore new file mode 100644 index 0000000..50e1322 --- /dev/null +++ b/t/122-defaults-parent/.gitignore @@ -0,0 +1 @@ +/*.log diff --git a/t/122-defaults-parent/all.do b/t/122-defaults-parent/all.do new file mode 100644 index 0000000..838ea31 --- /dev/null +++ b/t/122-defaults-parent/all.do @@ -0,0 +1,22 @@ +rm -f x/shouldfail + +log=$PWD/$1.log +rm -f "$log" + +expect_fail() { + local rv=$1 + shift + if ("$@") >>$log 2>&1; then + cat "$log" >&2 + echo "unexpected success:" "$@" >&2 + return $rv + else + return 0 + fi +} + +cd inner +expect_fail 11 redo ../x/shouldfail # should fail +expect_fail 12 redo-ifchange ../x/shouldfail # should fail again + +exit 0 diff --git a/t/122-defaults-parent/clean.do b/t/122-defaults-parent/clean.do new file mode 100644 index 0000000..b877c59 --- /dev/null +++ b/t/122-defaults-parent/clean.do @@ -0,0 +1,2 @@ +rm -f *~ .*~ */*~ */.*~ *.tmp */*.tmp x/shouldfail *.log */*.log + diff --git a/t/122-defaults-parent/inner/default.do b/t/122-defaults-parent/inner/default.do new file mode 100644 index 0000000..f8283d1 --- /dev/null +++ b/t/122-defaults-parent/inner/default.do @@ -0,0 +1,2 @@ +echo "inner/default.do: PWD=$PWD '$1' '$2' '$3'" >&2 +echo SUSPICIOUS diff --git a/t/122-defaults-parent/x/file b/t/122-defaults-parent/x/file new file mode 100644 index 0000000..e69de29 From 90989d1ffb365af693aab5def848e5b0002e3ace Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Sat, 2 Mar 2019 02:38:02 -0500 Subject: [PATCH 3/7] Overridden files were accidentally getting reclassified as static. This is relatively harmless, since we treat them *almost* identically, except that we print a warning for overridden files to remind you that something fishy is going on. Add a test for the actual warning message to ensure it is printed. (I don't like tests for specific warning messages, but it was necessary in this case.) --- redo/builder.py | 3 ++- t/350-deps/all.do | 2 +- t/350-deps/clean.do | 2 +- t/350-deps/override/.gitignore | 4 ++++ t/350-deps/override/a.do | 1 + t/350-deps/override/all.do | 34 ++++++++++++++++++++++++++++++++++ t/350-deps/override/b.do | 2 ++ t/350-deps/override/clean.do | 1 + 8 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 t/350-deps/override/.gitignore create mode 100644 t/350-deps/override/a.do create mode 100644 t/350-deps/override/all.do create mode 100644 t/350-deps/override/b.do create mode 100644 t/350-deps/override/clean.do diff --git a/redo/builder.py b/redo/builder.py index 0fb24a8..f7a9ac9 100644 --- a/redo/builder.py +++ b/redo/builder.py @@ -170,7 +170,8 @@ class _BuildJob(object): # to produce hello.c, but we don't want that to happen if # hello.c was created by the end user. debug2("-- static (%r)\n" % t) - sf.set_static() + if not sf.is_override: + sf.set_static() sf.save() return self._finalize(0) sf.zap_deps1() diff --git a/t/350-deps/all.do b/t/350-deps/all.do index f607605..77bc3e6 100644 --- a/t/350-deps/all.do +++ b/t/350-deps/all.do @@ -1,2 +1,2 @@ redo test1 test2 ifchange-fail overwrite gentest doublestatic \ - basic/test + basic/test override/all diff --git a/t/350-deps/clean.do b/t/350-deps/clean.do index 9405134..896b70f 100644 --- a/t/350-deps/clean.do +++ b/t/350-deps/clean.do @@ -1,3 +1,3 @@ -redo basic/clean +redo basic/clean override/clean rm -f *~ .*~ *.count t1a overwrite overwrite[123] \ genfile2 genfile.log static.log diff --git a/t/350-deps/override/.gitignore b/t/350-deps/override/.gitignore new file mode 100644 index 0000000..b94ccbc --- /dev/null +++ b/t/350-deps/override/.gitignore @@ -0,0 +1,4 @@ +a +b +*.log +stamp diff --git a/t/350-deps/override/a.do b/t/350-deps/override/a.do new file mode 100644 index 0000000..1ed9074 --- /dev/null +++ b/t/350-deps/override/a.do @@ -0,0 +1 @@ +echo hello-a >$3 diff --git a/t/350-deps/override/all.do b/t/350-deps/override/all.do new file mode 100644 index 0000000..57f86d8 --- /dev/null +++ b/t/350-deps/override/all.do @@ -0,0 +1,34 @@ +exec >&2 +rm -f a b *.log stamp + +echo 1 >stamp +redo b +[ "$(cat b)" = "hello-a-1-b" ] || exit 11 + +../../flush-cache +echo 2 >stamp +redo-ifchange b +[ "$(cat b)" = "hello-a-1-b" ] || exit 21 # a unchanged; b not redone + +. ../../skip-if-minimal-do.sh + +# Unfortunately the test below depends on the specific wording of the +# "override" warning message, of the form: +# redo: a - you modified it; skipping +# That's because this is specifically a test that the warning message +# gets generated. I added that test because (of course) when we didn't +# test it, the warning message accidentally got broken. Oops. If you +# rephrase the message, you'll have to also change the test. + +../../flush-cache +echo 3 >stamp +echo over-a >a +redo-ifchange b >$1.log 2>&1 +[ "$(cat b)" = "over-a-3-b" ] || exit 31 # a overwritten, b redone +grep "a - " "$1.log" >/dev/null || exit 32 # expected a warning msg + +../../flush-cache +echo 4 >stamp +redo-ifchange b >$1.log 2>&1 +[ "$(cat b)" = "over-a-3-b" ] || exit 41 # a not changed, b not redone +grep "a - " "$1.log" >/dev/null || exit 42 # still expect a warning msg diff --git a/t/350-deps/override/b.do b/t/350-deps/override/b.do new file mode 100644 index 0000000..7bfd42e --- /dev/null +++ b/t/350-deps/override/b.do @@ -0,0 +1,2 @@ +redo-ifchange a +printf '%s-%s-b\n' "$(cat a)" "$(cat stamp)" >$3 diff --git a/t/350-deps/override/clean.do b/t/350-deps/override/clean.do new file mode 100644 index 0000000..5ceb95c --- /dev/null +++ b/t/350-deps/override/clean.do @@ -0,0 +1 @@ +rm -f *~ .*~ stamp *.log a b From e5a27f04e8052a76cf4e8cd2a02eb549ab899052 Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Sat, 2 Mar 2019 02:53:02 -0500 Subject: [PATCH 4/7] If redo searched all the way up to /default.do, it would run ./default.do instead. This only happened if the containing project was buggy, ie. you tried to build a target that has no .do file available anywhere. However, it resulted in a confusing outcome for that case, where we'd run the wrong default.do file with the wrong parameters. Extended an existing test to catch this mistake. --- redo/builder.py | 2 ++ redo/paths.py | 2 +- t/122-defaults-parent/all.do | 16 +++++++++++++--- t/122-defaults-parent/inner2/default.do | 2 ++ 4 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 t/122-defaults-parent/inner2/default.do diff --git a/redo/builder.py b/redo/builder.py index f7a9ac9..237c8db 100644 --- a/redo/builder.py +++ b/redo/builder.py @@ -183,6 +183,8 @@ class _BuildJob(object): return self._finalize(0) else: err('no rule to redo %r\n' % t) + sf.set_failed() + sf.save() return self._finalize(1) # There is no good place for us to pre-create a temp file for # stdout. The target dir might not exist yet, or it might currently diff --git a/redo/paths.py b/redo/paths.py index a966af2..17c852d 100644 --- a/redo/paths.py +++ b/redo/paths.py @@ -32,7 +32,7 @@ def possible_do_files(t): # since t is an absolute path, dirbits[0] is always '', so we don't # need to count all the way down to i=0. for i in range(len(dirbits), 0, -1): - basedir = '/'.join(dirbits[:i]) + basedir = '/'.join(dirbits[:i]) or '/' subdir = '/'.join(dirbits[i:]) for dofile, basename, ext in _default_do_files(filename): yield (basedir, dofile, diff --git a/t/122-defaults-parent/all.do b/t/122-defaults-parent/all.do index 838ea31..917d634 100644 --- a/t/122-defaults-parent/all.do +++ b/t/122-defaults-parent/all.do @@ -1,7 +1,6 @@ rm -f x/shouldfail log=$PWD/$1.log -rm -f "$log" expect_fail() { local rv=$1 @@ -15,8 +14,19 @@ expect_fail() { fi } +# These should all fail because there is no matching .do file. +# In previous versions of redo, it would accidentally try to use +# $PWD/default.do even for ../path/file, which is incorrect. That +# could cause it to return success accidentally. + +rm -f "$log" cd inner -expect_fail 11 redo ../x/shouldfail # should fail -expect_fail 12 redo-ifchange ../x/shouldfail # should fail again +expect_fail 11 redo ../x/shouldfail +expect_fail 12 redo-ifchange ../x/shouldfail + +rm -f "$log" +cd ../inner2 +expect_fail 21 redo ../x/shouldfail2 +expect_fail 22 redo-ifchange ../x/shouldfail2 exit 0 diff --git a/t/122-defaults-parent/inner2/default.do b/t/122-defaults-parent/inner2/default.do new file mode 100644 index 0000000..ee989e6 --- /dev/null +++ b/t/122-defaults-parent/inner2/default.do @@ -0,0 +1,2 @@ +echo "inner/default.do: PWD=$PWD '$1' '$2' '$3'" >&2 +# output file is left empty From 83bc49512f47cc6b54ca1795bd43b6b021794939 Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Sat, 2 Mar 2019 03:09:42 -0500 Subject: [PATCH 5/7] Explicitly reject target/source filenames with newlines in them. This avoids an ugly assertion failure when we try to log a message containing an inner newline. --- redo/builder.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/redo/builder.py b/redo/builder.py index 237c8db..1a4acd8 100644 --- a/redo/builder.py +++ b/redo/builder.py @@ -478,6 +478,11 @@ def run(targets, shouldbuildfunc): else: selflock = myfile = me = None + for t in targets: + if '\n' in t: + err('%r: filenames containing newlines are not allowed.\n' % t) + return 204 + def cheat(): if not selflock: return 0 From 8a97b0cb2ccedc74c8fc6565a1cb6ef03706169a Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Sat, 2 Mar 2019 03:18:17 -0500 Subject: [PATCH 6/7] redo-ifchange regression: if REDO_LOG is not set, assume it's 1. At some point this got broken during a refactoring. The result was that redo-ifchange, run from the command line (as opposed to inside a .do script) would fail to start the log prettifier. --- redo/env.py | 2 +- t/100-args/all.do | 2 +- t/100-args/clean.do | 2 +- t/100-args/noargs/all.do | 1 + t/100-args/noargs/run.do | 3 +++ 5 files changed, 7 insertions(+), 3 deletions(-) create mode 100644 t/100-args/noargs/all.do create mode 100644 t/100-args/noargs/run.do diff --git a/redo/env.py b/redo/env.py index 104f532..ca416d4 100644 --- a/redo/env.py +++ b/redo/env.py @@ -35,7 +35,7 @@ class Env(object): self.VERBOSE = _get_bool('REDO_VERBOSE', '') self.XTRACE = _get_bool('REDO_XTRACE', '') self.KEEP_GOING = _get_bool('REDO_KEEP_GOING', '') - self.LOG = _get_int('REDO_LOG', '') + self.LOG = _get_int('REDO_LOG', '1') self.LOG_INODE = _get('REDO_LOG_INODE', '') self.COLOR = _get_int('REDO_COLOR', '') self.PRETTY = _get_int('REDO_PRETTY', '') diff --git a/t/100-args/all.do b/t/100-args/all.do index bcb13f5..ac34ba3 100644 --- a/t/100-args/all.do +++ b/t/100-args/all.do @@ -1,2 +1,2 @@ -redo test.args test2.args passfailtest +redo test.args test2.args passfailtest noargs/run . ../skip-if-minimal-do.sh diff --git a/t/100-args/clean.do b/t/100-args/clean.do index 655f41d..c9914e6 100644 --- a/t/100-args/clean.do +++ b/t/100-args/clean.do @@ -1 +1 @@ -rm -f passfail *~ .*~ +rm -f passfail *~ .*~ */*~ */.*~ noargs/all diff --git a/t/100-args/noargs/all.do b/t/100-args/noargs/all.do new file mode 100644 index 0000000..fa6c74c --- /dev/null +++ b/t/100-args/noargs/all.do @@ -0,0 +1 @@ +echo RAN >$3 diff --git a/t/100-args/noargs/run.do b/t/100-args/noargs/run.do new file mode 100644 index 0000000..f365ebf --- /dev/null +++ b/t/100-args/noargs/run.do @@ -0,0 +1,3 @@ +rm -f all +redo-ifchange # should not default to 'all' since not running from top level +[ ! -e all ] || exit 11 From 63230a1ae338b6a88be0e9c2a6e43b6f10d456dd Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Sat, 2 Mar 2019 03:45:35 -0500 Subject: [PATCH 7/7] builder.py: atomically replace the log for a given target. Previously we were truncating the log if it existed. This would cause redo-log to produce invalid output if you had the following (admittedly rare) sequence in a single session: - start building X - redo-log starts showing the log for X - finish building X - redo-log has not finished showing the log for X yet - start building X again for some reason - redo-log sees a truncated logfile. Now, redo-log can finish reading the original file (which no longer has a filename since it was overwritten) while the new file is being created. --- bin/all.do | 2 +- redo/builder.py | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/bin/all.do b/bin/all.do index 4d41446..e96e00d 100644 --- a/bin/all.do +++ b/bin/all.do @@ -1,3 +1,3 @@ exec >&2 -redo-ifchange ../redo/version/all ../redo/py list redo-sh +redo-ifchange ../redo/version/all ../redo/py ../redo/sh list xargs redo-ifchange