Both redo and minimal/do were doing slightly weird things with
symlinked directories, especially when combined with "..". For
example, if x is a link to ., then x/x/x/x/../y should resolve to
"../y", which is quite non-obvious.
Added some tests to make sure this stays fixed.
This can happen when $PWD contains a symlink somewhere in the path. In
that case, "cd ..; cat x" could mean something different from "cat ../x".
Notably, this error occurs when running "./do test" if your build
directory is through a symlink. For example, on freebsd your home
directory is /home/$USER, but /home is a symlink to /usr/home, which
triggers this problem.
Not adding tests in this commit, because when I added some tests, I
found even more symlink-related bugs, but those ones are much more
unlikely to occur. The additional fixes+tests are in a later commit.
Running commands in "||" context (like "x || return") disables "set -e"
behaviour in that context, even several levels deep in the call
hierarchy. The exact behaviour varies between shells, but this caused
a test failure with at least zsh 5.3.1 on debian.
Although I expect this is rather rare, some people may want to build in
a read-write subdir of a read-only tree. Other than some confusing
error reporting, this works fine in redo after the recent changes to
temp file handling, but let's add a test to make sure it stays that
way. The test found a bug in minimal/do, so let's fix that.
Reported-by: Jeff Stearns <jeff.stearns@gmail.com>
Previously, we'd try to put the stdout temp file in the same dir as the
target, if that dir exists. Otherwise we'd walk up the directory tree
looking for a good place. But this would go wrong if the directory we
chose got *deleted* during the run of the .do file.
Instead, we switch to an entirely new design: we use mkstemp() to
generate a temp file in the standard temp file location (probably
/tmp), then open it and immediately delete it, so the .do file can't
cause any unexpected behaviour. After the .do file exits, we use our
still-open fd to the stdout file to read the content back out.
In the old implementation, we also put the $3 in the "adjusted"
location that depended whether the target dir already existed, just for
consistency. But that was never necessary: we didn't create the $3
file, and if the .do script wants to write to $3, it should create the
target dir first anyway. So change it to *always* use a $3 temp
filename in the target dir, which is much simpler and so has fewer edge
cases.
Add t/202-del/deltest4 with some tests for all these edge cases.
Reported-by: Jeff Stearns <jeff.stearns@gmail.com>
Basically all just missing quotes around shell strings that use $PWD.
Most paths inside a project, since redo uses relative paths, only need
to worry when project-internal directories or filenames have spaces in
them.
Reported-by: Jeff Stearns <jeff.stearns@gmail.com>
If we tried to build target a/b/c/d and a/b/c didn't exist yet, we
would correctly name the temp file something like a__b/c__d.tmp. But
if a/b didn't exist yet, we named the temp file a/b__c/d.tmp, which
didn't work. Instead, name it a/b__c__d.tmp as expected.
With the new "continue" feature on by default, it turned out that
ctrl-c during a build, or a .do file returning an error, would mark a
target as "built" even though it hadn't been. This would prevent
retrying it when you started minimal/do again. Use a temp file
instead.
It's a little tricky: to prevent accidental recursion, we want to
create a file *before* building, but clean up that file when starting
the next session. And we rename that file to the actual .did file
*after* building successfully.
If redo-ifchange is a symlink to minimal/do, and we run it from
*outside* an existing session, it shouldn't default to 'all'.
redo-sources, redo-targets, and redo-ood aren't too useful with
minimal/do, but they should exist in case someone tries to use them, so
just symlink them to /bin/true, like we do with redo-ifcreate.
It would be incorrect to print ../$1.do (we only use $1.do in the
*current* directly, not prefix dirs; we only use default*.do in
those). However, when we found ../default.do, we forgot to print it,
because of a silly logic error.
Adding redo-whichdo wasn't so hard, except that it exposed a bunch of
mistakes in the way minimal/do was choosing .do files. Most of the
wrong names it tried didn't matter (since they're very unlikely to
exist) except that they produce the wrong redo-whichdo output.
Debugging that took so much effort that I added unit tests for some
functions to make it easier to find problems in the future.
Note: this change apparently makes minimal/do about 50% slower than
before, because the _find_dofiles algorithm got less optimized (but
more readable). It's still not very slow, and anyway, since it's
minimal/do, I figured readable/correct code probably outweighed a bit
of speed. (Although if anyone can come up with an algorithm that's
clear *and* works better, I won't turn it down :) I feel like I must
be doing it the hard way...)
After playing with "continuable" mode for a while, it seems to me that
what most people want will be incremental builds by default (albeit
without any dependency checking to rebuild a target if sources change).
This lets you run minimal/do several times in a row to incrementally
build up your project while you're playing around. If you want to
start over, use -c.
As a result, in test.do we now recommend using 'minimal/do -c test'
since minimal/do now strays even further from proper redo semantics
otherwise.
While we're here, update the usage message to describe what all the new
options mean.
-x, -v, and -d are the same as redo.
-c means "continuable", which disables the feature that deletes (and
forgets) all targets at the start of each run. This is a little risky,
since minimal/do still doesn't understand dependencies, but it allows
you to run minimal/do several times in succession, so that
minimal/do -c a
minimal/do -c b
is the same as
minimal/do a b
We previously assumed that redo and redo-ifchange are the same in
minimal/do's design, because it rebuilds all targets on every run, and
so there's no reason to ever build the same target more than once.
Unfortunately that's incorrect: if you run 'redo x' from two points in
a single run (or even twice in the same .do file), we expect x to be
built twice. If you wanted redo to decide whether to build it the
second time, you should have used redo-ifchange.
t/102-empty/touchtest was trying to test for this. However, a
second bug in minimal/do made the test pass anyway. minimal/do would
*always* rebuild any target x that produced no output, not caring
whether it had tried to build before, whether you used redo or
redo-ifchange. And while we tested that redo would redo a file that
had been deleted, we didn't ensure that it would redo a file that was
*not* deleted, nor that redo-ifchange would *not* redo that file.
Fix both bugs in minimal/do, and make t/102-empty/touchtest cover the
missing cases.
We grew a test for these at some point, but minimal/do didn't actually
pass it.
sh syntax oddity: if you say
x=$1 y=$2
then it works fine when $1 and $2 contain spaces. But if you say
export x=$1 y=$2
(or "local" instead of "export") then $1 and $2 will be split on IFS,
and it won't do what you think. I guess this is because 'export' and
'local' are like commands, and command arguments are split if not
quoted.
...or files that contain bytes but not a trailing newline. It's okay if we
don't get any data, but we definitely have to *not* let "set -e" abort us.
Now that we fixed set -e in the previous patch, it revealed this problem.
If you run something like
blah_function || return 1
then everything even *inside* blah_function is *not* subject to the "set -e"
that would otherwise be in effect. That's true even for ". subfile" inside
blah_function - which is exactly how minimal/do runs .do files.
Instead, rewrite it as
blah_function
[ "$?" = "0" ] || return 1
And add a bit to the unit tests to ensure that "set -e" behaviour is enabled
in .do files as we expect, and crash loudly otherwise.
(This weird behaviour may only happen in some shells and not others.)
Also, we had a "helpful" alias of redo() defined at the bottom of the file.
Combined with the way we use '.' to source the .do files, this would make it
not start a new shell just to run a recursive 'redo' command. It almost
works, but this stupid "set -e" bug could cause a nested .do file to not
honour "set -e" if someone ran "redo foo || exit 1" from inside a .do
script. The performance optimization is clearly not worth it here, so
rename it to _redo(); that causes it to actually re-exec the redo program
(which is a symlink to minimal/do).
If you use "redo --old-args", it will switch back to the old
(apenwarr-style) arguments for now, to give you time to update your .do
scripts. This option will go away eventually.
Note: minimal/do doesn't understand the --old-args option. If you're using
minimal/do in your project, keep using the old one until you update your use
of $1/$2, and then update to the new one.
apenwarr-style default.o.do:
$1 foo
$2 .o
$3 whatever.tmp
djb-style default.o.do:
$1 foo.o
$2 foo
$3 whatever.tmp
apenwarr-style foo.o.do:
$1 foo.o
$2 ""
$3 whatever.tmp
djb-style foo.o.do:
$1 foo.o
$2 foo.o (I think?)
$3 whatever.tmp
The reason we'd crash is that we tried to pre-create a file called
$target.redo.tmp, which wouldn't work because the directory containing
$target didn't exist.
We now try to generate a smarter filename by using the innermost directory
of target that *does* exist. It's a little messy, but the idea is to make
sure we won't have to rename() across a filesystem boundary if, for example,
there's a mounted filesystem in the middle of the hierarchy somewhere.
Since we use ". filename.do" to run the .do files instead of just
"filename.do", shell local variables end up being inherited by the
subprogram. Change all the local variables to be all lowercase, to avoid
conflicting with any typical environment variables someone might use.
The particular variable that triggered this was PREFIX (reported by "ulrik"
on the mailing list) and that fixes this, at least.
Arguably we shouldn't be using ".", but using it avoids unnecessary forks,
which is kind of nice.
If you ". ./filename" and ./filename contains no commands, apparently
ash/dash will give the exit code of the command *before* the ., rather than
defaulting to zero as it supposedly should. This should work around it in
our .do files at least.
Reported by Tim Allen.
It should just build nothing. Because sometimes you want to do something
like:
redo-ifchange $(find -name '*.c')
And the find doesn't return any results. This is consistent with what real
redo does.
Added a test to confirm that it works.
Sometimes clean.do implementations want to delete our .do_built file and
directory. If that happens, deal with it quietly, as long as they don't
request any *additional* redo-type operations.
Apparently emacs sets TERM=dumb in its tty simulator, so even though
isatty() returns true, we shouldn't use colour codes. (emacs is therefore
lame. But we knew that.)
If all.do runs and creates no output, we shouldn't create a file called
'all', but we should remember that 'all' has been run successfully. We do
this by creating 'all.did' during the build.
Since minimal/do always just wipes everything out every time it runs, we can
safely remove the .did files after minimal/do terminates, so this doesn't
clutter things too much in normal use.
This fixes some edge cases, particularly that 'minimal/do clean' no longer
leaves stupid files named "clean" lying around, and the redo-sh directory
can now be rebuilt correctly since we rebuild it as long as redo-sh.did
doesn't exist. (We don't want to "rm -rf redo-sh" because it makes me
nervous.)
...only when running under minimal/do, of course.
The tests in question mostly fail because they're testing particular
dependency-related behaviour, and minimal/do doesn't support dependencies,
so naturally it doesn't work.
Just allow that sub-redo to return an error code. Also, parent redos should
return error code 1, not the same code as the child. That makes it easier
to figure out which file generated the "special" error code.
It actually decreases readability of the .do files - by not making it
explicit when you're going into a subdir.
Plus it adds ambiguity: what if there's a dirname.do *and* a dirname/all?
We could resolve the ambiguity if we wanted, but that adds more code, while
taking out this special case makes *less* code and improves readability.
I think it's the right way to go.
This greatly reduces the number of fork+exec calls, so in particular,
t/curse/all.do now runs much faster:
/bin/sh (bash): was 5.9s, now 2.2s
/bin/dash: was 3.2s, now 1.1s
Obviously improving the speed of minimal/do doesn't really matter, except
that it makes a good benchmark to compare the "real" redo against. So far
it's losing badly: 5.4s.
