Here is my second attempt at a gbs cleanup patch:
2004-10-29 Andrew E. Schulman [EMAIL PROTECTED]
* generic-build-script: cleanups.
- Remove superfluous ;'s and line-ending \'s
- Invoke /bin/sh with -e
- Remove 's joining commands; obviated by -e
- Remove STATUS checks at end; obviated by -e
- Add -r to all invocations of xargs;
prevents errors in null cases
- Remove 21 redirections after xargs;
obviated by xargs -r
- Allow diff to exit normally with status 1;
just means changes were found
- Remove unnecessary/undesirable trailing 'true's
The patch and changelog are attached. In order to simplify the discussion
this time, I've left out the improvements to mkdir, find, and other minor
changes that I had included in the previous patch. I'll save those for
another time.
- Invokes the shell by /bin/sh -e.
Great. Have you tested whether the -e flag gets propagated inside the
functions?
Yes, I have now:
$ /bin/sh -e ; echo finished /bin/sh -e, status=$?
$ echo $-
eims
$ # the -e flag propagates into subshells:
$ tst1() { echo $- ; }
$ tst1
eims
$ # false result inside a function causes subshell and
$ # main shell to immediately exit:
$ tst2() { false ; echo continuing execution ; }
$ tst2
finished /bin/sh -e, status=1
- Removes the many superfluous 's, ;'s, and \'s between commands and
at end of lines.
Umm, actually that does change the semantics of the calls slightly (though
perhaps for the better). In particular, code like
if [ -f ${src_patch} ] ; then
patch -Z -p0 ${src_patch}
fi
will, I believe, now fail if the 'patch' command fails, and it wouldn't
have before. Just something to note. Have you tested that the script
works properly in these cases?
No, in this case the semantics are the same. The trailing ; after the
'patch' in the old code has no effect; 'patch' is still the last command in
the 'if'...'fi' block, so its exit status becomes the exit status of 'if'.
I did check this, both in general and by running the gbs with a mangled
src_patch, which caused the script (both old and new versions) to stop at
the patch command.
However, in cases where two commands are separated by ; in the old code,
it's true that the semantics have changed: if the first command fails, in
the old code the second command would be executed, in the new code it won't.
This is desirable; see below.
Hmm, you should've really added '|| true' everywhere there was a '; \' in
the original code to make this patch a simple transformation. In
particular, the find | xargs combo sometimes returned false for no
reason, so we used to ignore its exit status (was it the lack of -r?).
Ditto for install -- neither the man nor the info page say anything
about the exit status, and I can't look at the code right now. I don't
mean to nitpick, but whenever you chose to omit '|| true', you could have
probably added a comment saying why the original code was wrong. Would
you mind doing that?
Okay, there are several issues here.
(1) The man pages for find and xargs, and some experimentation, show that
the only reason find | xargs will return false is if the command that xargs
runs returns false. An obvious reason for that is if the command expects
some arguments, but xargs doesn't give it any; we've removed that
then we don't want to suppress the failure; see (3) below. find
still returns true, by the way, even if it prints an empty list (or more
exactly, never evaluates a true expression).
(2) At least one of the trailing 'true's that I removed was clearly intended
to suppress an error due to an empty argument list:
strip() {
(cd ${instdir} \
find . -name *.dll -or -name *.exe | xargs strip 21 ; \
true )
}
strip normally writes nothing at all to stdout or stderr, so the obvious
reason to have '21 ; true' here is in case xargs gives 'strip' an empty
argument list. This is therefore better rendered as
strip() {
(cd ${instdir}
find . -name *.dll -or -name *.exe | xargs -r strip )
}
See below for more about removing the redirection. Of course there might be
other sorts of errors, such as an unwriteable executable. This leads to...
(3) If there are other sorts of errors, we shouldn't be suppressing them
with trailing 'true's. In this sense the semantics of the patched code are,
as you say, better. For example, if install returns false, maybe we don't
understand yet why, but something has gone wrong and we should halt, unless
we can determine that there's some good reason (as with diff) to ignore the
error. If strip returns false, maybe the executable it's trying to work on
has wrong permissions, or isn't a proper executable. So again we should
halt.
whenever you chose to omit '|| true', you could have
probably added a comment saying why the original code was wrong. Would
you mind doing that?
No problem. But where should I add it? In the new code? Does it make
sense to have a comment in the new code
