On Fri, 15 Oct 2004, Schulman.Andrew wrote:
> As we discussed a day or two ago, here is a patch that cleans up the
> following aspects of the generic build script (today's CVS version):
Great, thanks, Andrew. Would you mind attaching the patch next time,
though, as mailers screw up spacing and line wrapping... Also, it's
missing a ChangeLog (what you wrote below doesn't quite qualify, although
it could be massaged into one). Some more comments below.
> - Invokes the shell by /bin/sh -e.
Great. Have you tested whether the -e flag gets propagated inside the
functions?
> - 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?
> - Removes all of the $STATUS junk at the end. There's no more need for
> this, since with -e the script will fail when any subcommand fails.
Ok, this is fine.
> - Replaces all instances of 'if [ ! -d xxx ] ; then mkdir -p xxx ; fi'
> with just 'mkdir -p xxx'. The second form is equivalent but simpler.
Ah, I didn't realize that '-p' also means "no error if existing". Good
catch! :-)
> - Adds -r to every invocation of xargs. In every case this is desirable
> and will prevent errors in null cases.
Yup, this is something I was thinking of doing for a while. Thanks.
Later we should also probably change all "find | xargs" combos to "find
-print0 | xargs -0".
> With /bin/sh -e, one has to be careful about trapping unwanted non-zero
> exit statuses. The only place I found where this was an issue was in
> mkpatch(), because diff has an exit status of 1 if it finds differences.
> Because of this I added '|| true' after the diff command. One could be
> fussier here and check for an exit status of 1 (which is okay) or 2
> (which is an error), but I didn't bother. I removed all other instances
> of trailing 'true's from the script.
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?
A comment about the exit status 1 vs. 2 for diff would also be helpful in
the source, in case we decide to do this in the future...
> I've tested the revised gbs by building three different packages, and
> tested each operation at least once. I had no problems.
I assume all of them succeeded... Did you test for failures too (i.e.,
that the script correctly stops if a command fails, etc, etc)?
A few other minor items:
- there were some space changes (blank lines deleted) that shouldn't have
been (the blank line after "unpack()" is a particular one I'm thinking
of)
- redirections from xargs were removed (perhaps most stderr messages were
removed by the "-r" flag, but maybe not)
- you changed the "cd $dir && find ." to "find $dir" in some places, which
I'd be more comfortable changing as a separate step, if at all
- in list(), you changed the files that are found (i.e., ".*" would have
been ignored before, and won't be now), and added "sort". Frankly, I'd
prefer not to include extra improvements like that in a patch this
large. I'll be happy to commit them separately later.
- in the main arg processing loop, you've combined cases. While it makes
the code shorter somewhat, I don't see it as a readability improvement,
and don't think that this patch should cover it -- a separate patch
would be better.
Thanks again for the effort you've invested in this, and for putting up
with my nitpicking.
Igor
--
http://cs.nyu.edu/~pechtcha/
|\ _,,,---,,_ [EMAIL PROTECTED]
ZZZzz /,`.-'`' -. ;-;;,_ [EMAIL PROTECTED]
|,4- ) )-,_. ,\ ( `'-' Igor Pechtchanski, Ph.D.
'---''(_/--' `-'\_) fL a.k.a JaguaR-R-R-r-r-r-.-.-. Meow!
"Happiness lies in being privileged to work hard for long hours in doing
whatever you think is worth doing." -- Dr. Jubal Harshaw
