I narrowed the cc list back to scm-migration-dev, because the webrev.sh
changes went back to onnv-gate under separate cover.
Thanks, Roland, for looking. At a high level:
1. Doesn't matter whether nightly.sh cleanups are deferred or not; any
merge with these changes is sure to be straightforward. (And I'm
guessing that, since your question implies that you haven't done much
of the cleanup work yet, these changes will go back first.)
2. I did a blanket replacement of exit status checks to use arithmetic
comparison in nightly.sh, leaving only the cases where the test is part
of a compound logical statement.
3. I cleaned up only the functions that I'm already touching significantly
for echo vs printf. I suspect it was largely superfluous, because I
think the script effectively controls the input, but good style is good
style, and incremental cleanup is useful.
I got some feedback from Rich that I had (obviously, and I'm somewhat
ashamed) failed to fix the actual problem of bug 481. (I cleaned up the
callees, but not the callers.) So there's another webrev forthcoming on
these changes, after I do some sanity testing.
--Mark
On Fri, 20 Jun 2008, Roland Mainz wrote:
> Date: Fri, 20 Jun 2008 01:29:37 +0200
> From: Roland Mainz <roland.mainz at nrubsig.org>
> To: Mark J. Nelson <Mark.J.Nelson at Sun.COM>
> Cc: Minh.Nguyen at Sun.COM, William.Fiveash at Sun.COM,
> Michael Kupfer <Mike.Kupfer at Sun.COM>, Joh Levon <john.levon at
> sun.com>,
> Peter.Cudhea at Sun.COM, on-gatekeeper-bugs at sun.com,
> Daniel Duvall <Danek.Duvall at Sun.COM>,
> James Carlson <James.D.Carlson at Sun.COM>, William.Kucharski at Sun.COM,
> William Sommerfeld <William.Sommerfeld at Sun.COM>, Ryan.Scott at Sun.COM,
> scm-migration-dev at opensolaris.org
> Subject: Re: [scm-migration-dev] code review request for nightly.sh
> andwebrev.sh changes
>
> "Mark J. Nelson" wrote:
>>
>> I reckon the interest list for the webrev bug is as good a place as any to
>> seek code review. I'm not an HTML guy, so I did a pretty generic
>> seek-and-replace for ;<\([^ ]+\) \(.*\)/>;<\1 \2></\1>; in the webrev
>> source.
>>
>> The two different webrevs are
>>
>> http://cr.opensolaris.org/~mjnelson/webrev.481/
>> http://cr.opensolaris.org/~mjnelson/webrev.481-firefox3.0/
>>
>
> 1. I assume it may be better to defer any nightly.sh cleanups until your
> putback is done, right ?
> 2. Quick look at
> http://cr.opensolaris.org/~mjnelson/webrev.481/scm-fixes.patch (patch
> code is quoted with "> "):
>>
>> - [ $? -eq 0 ] || build_ok=n
>> + if [ $? -ne 0 ]; then
>
> Please use ...
> -- snip --
> if (( $? != 0 )) ; then
> -- snip --
> (see
> http://www.opensolaris.org/os/project/shell/shellstyle/#compare_exit_code_using_math)
>
>> + echo "cpio failed for $DEST" |
>
> Please use
> -- snip --
> printf "cpio failed for %s\n" "$DEST"
> -- snip --
> .. to prevent that "echo" interprets any special characters in $DEST
>
>> + tee -a $mail_msg_file >> $LOGFILE
>> + build_ok=n
>> + fi
>> ;;
>> mercurial)
>> - copy_source_mercurial $DEST $srcroot
>> - [ $? -eq 0 ] || build_ok=n
>> + copy_source_mercurial $DEST $srcroot || build_ok=n
>> ;;
>> *)
>> build_ok=n
>> @@ -358,7 +365,11 @@
>>
>> if [[ -n "$open_top" ]]; then
>> hg locate "$open_top" | cpio -pd "$dest" >>$LOGFILE 2>&1
>> - [ $? -eq 0 ] || return 1
>> + if [ $? -ne 0 ]; then
>
> Please use ...
> -- snip --
> if (( $? != 0 )) ; then
> -- snip --
> (see
> http://www.opensolaris.org/os/project/shell/shellstyle/#compare_exit_code_using_math)
>
>
>> + echo "can't copy $srcroot: closed tree not" \
>
> Please use
> -- snip --
> printf "can't copy %s: closed tree not present\n" "$srcroot"
> -- snip --
>
> ... and please try to avoid breaking string literals at the 80 char
> limit - this doesn't work properly in shellscripts. $ echo "foo foo" #
> is not equal to $ echo "foo" "foo" #
>
>> + "present." | tee -a $mail_msg_file >> $LOGFILE
>> + return 1
>> + fi
>> fi
>>
>> if [[ -n "$closed_top" ]]; then
>> @@ -366,13 +377,23 @@
>> if [[ "$closed_top" = usr/closed ]]; then
>> (cd usr/closed; hg locate |
>> cpio -pd "$dest/usr/closed") >>$LOGFILE 2>&1
>> + if [ $? -ne 0 ]; then
>
> Please use ...
> -- snip --
> if (( $? != 0 )) ; then
> -- snip --
> (see
> http://www.opensolaris.org/os/project/shell/shellstyle/#compare_exit_code_using_math)
>
>> + echo "cpio failed for $dest/usr/closed" |
>
> Please use
> -- snip --
> printf "cpio failed for %s/usr/closed\n" "$dest"
> -- snip --
>
>> + tee -a $mail_msg_file >> $LOGFILE
>> + return 1
>> + fi
>> else
>> # copy subtree of usr/closed
>> closed_top=${closed_top#usr/closed/}
>> (cd usr/closed; hg locate "$closed_top" |
>> cpio -pd "$dest/usr/closed") >>$LOGFILE 2>&1
>> + if [ $? -ne 0 ]; then
>
> Please use ...
> -- snip --
> if (( $? != 0 )) ; then
> -- snip --
> (see
> http://www.opensolaris.org/os/project/shell/shellstyle/#compare_exit_code_using_math)
>
>
>> + echo "cpio failed for " \
>> + "$dest/usr/closed/$closed_top" |
>
>
> Please use
> -- snip --
> printf "cpio failed for %s\n" "$dest/usr/closed/$closed_top"
> -- snip --
>
>> + tee -a $mail_msg_file >> $LOGFILE
>> + return 1
>> + fi
>>
>
> ----
>
> Bye,
> Roland
>
> --
> __ . . __
> (o.\ \/ /.o) roland.mainz at nrubsig.org
> \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer
> /O /==\ O\ TEL <currently fluctuating>
> (;O/ \/ \O;)
> _______________________________________________
> scm-migration-dev mailing list
> scm-migration-dev at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/scm-migration-dev
>
