This is the mail archive of the cygwin-apps@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

gbs cleanup patch, 2nd try


Here is my second attempt at a gbs cleanup patch:

2004-10-29 Andrew E. Schulman <andrex@alumni.utexas.net>

 * 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 2>&1 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 2>&1 ; \
  true )
}

strip normally writes nothing at all to stdout or stderr, so the obvious 
reason to have '2>&1 ; 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 about why we removed something
that's no longer there?  Let me know.

> 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 added a comment about this, and also a test for exit status 1 (which is a 
successful result).

> > 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...  

Yes.

> Did you test for failures too (i.e.,
> that the script correctly stops if a command fails, etc, etc)?

Yes, I have, though not in as many cases.  Some examples are above.  But 
it's pretty hard to go wrong with that; -e ensures that as soon as a command 
fails, we halt.

> - redirections from xargs were removed (perhaps most stderr messages were
>   removed by the "-r" flag, but maybe not)

Right.  As I said above, I think these were only there to divert error 
messages due to empty argument lists in xargs.  But if there were some other 
error messages, why should we send them to stdout?  If it's because of
a general policy that "all error messages should go to stdout", then we 
shouldn't be doing this in individual commands; we should do it once and for 
all at the top of the script, with an "exec 2>&1".  But note that this isn't 
consistently done throughout, and anyway I don't think it's a good idea.

Whew!
Andrew.

Attachment: Changelog.txt
Description: Text document

--- generic-build-script.orig	2004-10-14 18:55:26.000000000 -0400
+++ generic-build-script	2004-10-30 00:57:48.000000000 -0400
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/sh -e
 #
 # Generic package build script
 #
@@ -125,23 +125,23 @@
 }
 
 mkdirs() {
-  (cd ${topdir} && \
-  rm -fr ${objdir} ${instdir} ${srcinstdir} && \
-  mkdir -p ${objdir} && \
-  mkdir -p ${instdir} && \
+  (cd ${topdir}
+  rm -fr ${objdir} ${instdir} ${srcinstdir}
+  mkdir -p ${objdir}
+  mkdir -p ${instdir}
   mkdir -p ${srcinstdir} )
 }
 prep() {
-  (cd ${topdir} && \
-  unpack ${src_orig_pkg} && \
-  cd ${topdir} && \
-  if [ -f ${src_patch} ] ; then \
-    patch -Z -p0 < ${src_patch} ;\
-  fi && \
+  (cd ${topdir}
+  unpack ${src_orig_pkg}
+  cd ${topdir}
+  if [ -f ${src_patch} ] ; then
+    patch -Z -p0 < ${src_patch}
+  fi
   mkdirs )
 }
 conf() {
-  (cd ${objdir} && \
+  (cd ${objdir}
   CFLAGS="${MY_CFLAGS}" LDFLAGS="${MY_LDFLAGS}" \
   ${srcdir}/configure \
   --srcdir=${srcdir} --prefix="${prefix}" \
@@ -152,115 +152,114 @@
   --datadir='${prefix}/share' )
 }
 reconf() {
-  (cd ${topdir} && \
-  rm -fr ${objdir} && \
-  mkdir -p ${objdir} && \
+  (cd ${topdir}
+  rm -fr ${objdir}
+  mkdir -p ${objdir}
   conf )
 }
 build() {
-  (cd ${objdir} && \
+  (cd ${objdir}
   CFLAGS="${MY_CFLAGS}" make )
 }
 check() {
-  (cd ${objdir} && \
+  (cd ${objdir}
   make ${test_rule} | tee ${checkfile} 2>&1 )
 }
 clean() {
-  (cd ${objdir} && \
+  (cd ${objdir}
   make clean )
 }
 install() {
-  (cd ${objdir} && \
-  rm -fr ${instdir}/* && \
-  make install DESTDIR=${instdir} && \
-  for f in ${prefix}/share/info/dir ${prefix}/info/dir ; do \
-    if [ -f ${instdir}${f} ] ; then \
-      rm -f ${instdir}${f} ; \
-    fi ;\
-  done &&\
-  for d in ${prefix}/share/doc/${BASEPKG} ${prefix}/share/doc/Cygwin ; do \
-    if [ ! -d ${instdir}${d} ] ; then \
-      mkdir -p ${instdir}${d} ;\
-    fi ;\
-  done &&\
-  if [ -d ${instdir}${prefix}/share/info ] ; then \
-    find ${instdir}${prefix}/share/info -name "*.info" | xargs -r gzip -q ; \
-  fi && \
-  if [ -d ${instdir}${prefix}/share/man ] ; then \
+  (cd ${objdir}
+  rm -fr ${instdir}/*
+  make install DESTDIR=${instdir}
+  for f in ${prefix}/share/info/dir ${prefix}/info/dir ; do
+    if [ -f ${instdir}${f} ] ; then
+      rm -f ${instdir}${f}
+    fi
+  done
+  for d in ${prefix}/share/doc/${BASEPKG} ${prefix}/share/doc/Cygwin ; do
+    if [ ! -d ${instdir}${d} ] ; then
+      mkdir -p ${instdir}${d}
+    fi
+  done
+  if [ -d ${instdir}${prefix}/share/info ] ; then
+    find ${instdir}${prefix}/share/info -name "*.info" | xargs -r gzip -q
+  fi
+  if [ -d ${instdir}${prefix}/share/man ] ; then
     find ${instdir}${prefix}/share/man -name "*.1" -o -name "*.3" -o \
       -name "*.3x" -o -name "*.3pm" -o -name "*.5" -o -name "*.6" -o \
-      -name "*.7" -o -name "*.8" | xargs gzip -q ; \
-  fi && \
-  templist="" && \
-  for fp in ${install_docs} ; do \
-    for f in ${srcdir}/$fp ; do \
-      if [ -f $f ] ; then \
-        templist="$templist $f"; \
-      fi ; \
-    done ; \
-  done && \
-  if [ ! "x$templist" = "x" ]; then \
+      -name "*.7" -o -name "*.8" | xargs -r gzip -q
+  fi
+  templist=""
+  for fp in ${install_docs} ; do
+    for f in ${srcdir}/$fp ; do
+      if [ -f $f ] ; then
+        templist="$templist $f"
+      fi
+    done
+  done
+  if [ ! "x$templist" = "x" ]; then
     /usr/bin/install -m 644 $templist \
-         ${instdir}${prefix}/share/doc/${BASEPKG} ; \
-  fi && \
-  if [ -f ${srcdir}/CYGWIN-PATCHES/${PKG}.README ]; then \
+         ${instdir}${prefix}/share/doc/${BASEPKG}
+  fi
+  if [ -f ${srcdir}/CYGWIN-PATCHES/${PKG}.README ]; then
     /usr/bin/install -m 644 ${srcdir}/CYGWIN-PATCHES/${PKG}.README \
-      ${instdir}${prefix}/share/doc/Cygwin/${BASEPKG}.README ; \
-  elif [ -f ${srcdir}/CYGWIN-PATCHES/README ] ; then \
-    /usr/bin/install -m 644 ${srcdir}/CYGWIN-PATCHES/README \
-      ${instdir}${prefix}/share/doc/Cygwin/${BASEPKG}.README ; \
-  fi && \
-  if [ -f ${srcdir}/CYGWIN-PATCHES/${PKG}.sh ] ; then \
-    if [ ! -d ${instdir}${sysconfdir}/postinstall ]; then \
-      mkdir -p ${instdir}${sysconfdir}/postinstall ; \
-    fi && \
+      ${instdir}${prefix}/share/doc/Cygwin/${BASEPKG}.README
+  elif [ -f ${srcdir}/CYGWIN-PATCHES/README ] ; then
+    /usr/bin/install -m 644 ${srcdir}/CYGWIN-PATCHES/README
+      ${instdir}${prefix}/share/doc/Cygwin/${BASEPKG}.README
+  fi
+  if [ -f ${srcdir}/CYGWIN-PATCHES/${PKG}.sh ] ; then
+    if [ ! -d ${instdir}${sysconfdir}/postinstall ]; then
+      mkdir -p ${instdir}${sysconfdir}/postinstall
+    fi
     /usr/bin/install -m 755 ${srcdir}/CYGWIN-PATCHES/${PKG}.sh \
-      ${instdir}${sysconfdir}/postinstall/${PKG}.sh ; \
-  elif [ -f ${srcdir}/CYGWIN-PATCHES/postinstall.sh ] ; then \
-    if [ ! -d ${instdir}${sysconfdir}/postinstall ]; then \
-      mkdir -p ${instdir}${sysconfdir}/postinstall ; \
-    fi && \
+      ${instdir}${sysconfdir}/postinstall/${PKG}.sh
+  elif [ -f ${srcdir}/CYGWIN-PATCHES/postinstall.sh ] ; then
+    if [ ! -d ${instdir}${sysconfdir}/postinstall ]; then
+      mkdir -p ${instdir}${sysconfdir}/postinstall
+    fi
     /usr/bin/install -m 755 ${srcdir}/CYGWIN-PATCHES/postinstall.sh \
-      ${instdir}${sysconfdir}/postinstall/${PKG}.sh ; \
-  fi && \
-  if [ -f ${srcdir}/CYGWIN-PATCHES/preremove.sh ] ; then \
-    if [ ! -d ${instdir}${sysconfdir}/preremove ]; then \
-      mkdir -p ${instdir}${sysconfdir}/preremove ; \
-    fi && \
+      ${instdir}${sysconfdir}/postinstall/${PKG}.sh
+  fi
+  if [ -f ${srcdir}/CYGWIN-PATCHES/preremove.sh ] ; then
+    if [ ! -d ${instdir}${sysconfdir}/preremove ]; then
+      mkdir -p ${instdir}${sysconfdir}/preremove
+    fi
     /usr/bin/install -m 755 ${srcdir}/CYGWIN-PATCHES/preremove.sh \
-      ${instdir}${sysconfdir}/preremove/${PKG}.sh ; \
+      ${instdir}${sysconfdir}/preremove/${PKG}.sh
   fi )
 }
 strip() {
-  (cd ${instdir} && \
-  find . -name "*.dll" -or -name "*.exe" | xargs strip 2>&1 ; \
-  true )
+  (cd ${instdir}
+  find . -name "*.dll" -or -name "*.exe" | xargs -r strip )
 }
 list() {
-  (cd ${instdir} && \
-  find . -name "*" ! -type d | sed 's%^\.%  %' ; \
-  true )
+  (cd ${instdir}
+  find . -name "*" ! -type d | sed 's%^\.%  %' )
 }
 depend() {
-  (cd ${instdir} && \
-  find ${instdir} -name "*.exe" -o -name "*.dll" | xargs cygcheck | \
-  sed -e '/\.exe/d' -e 's,\\,/,g' | sort -bu | xargs -n1 cygpath -u \
-  | xargs cygcheck -f | sed 's%^%  %' | sort -u ; \
-  true )
+  (cd ${instdir}
+  find ${instdir} -name "*.exe" -o -name "*.dll" | xargs -r cygcheck \
+  | sed -e '/\.exe/d' -e 's,\\,/,g' | sort -bu | xargs -r -n1 cygpath -u \
+  | xargs -r cygcheck -f | sed 's%^%  %' | sort -u )
 }
 pkg() {
-  (cd ${instdir} && \
+  (cd ${instdir}
   tar cvjf ${bin_pkg} * )
 }
 mkpatch() {
-  (cd ${srcdir} && \
-  find . -name "autom4te.cache" | xargs rm -rf ; \
-  unpack ${src_orig_pkg} && \
-  mv ${BASEPKG} ../${BASEPKG}-orig && \
-  cd ${topdir} && \
+  (cd ${srcdir}
+  find . -name "autom4te.cache" | xargs -r rm -rf
+  unpack ${src_orig_pkg}
+  mv ${BASEPKG} ../${BASEPKG}-orig
+  cd ${topdir}
+  # allow diff to exit normally with status 1;
+  # this just means some changes were found
   diff -urN -x '.build' -x '.inst' -x '.sinst' \
     ${BASEPKG}-orig ${BASEPKG} > \
-    ${srcinstdir}/${src_patch_name} ; \
+    ${srcinstdir}/${src_patch_name} || [ $? -eq 1 ]
   rm -rf ${BASEPKG}-orig )
 }
 # Note: maintainer-only functionality
@@ -268,91 +267,89 @@
   cp --backup=numbered ${srcinstdir}/${src_patch_name} ${topdir}
 }
 spkg() {
-  (mkpatch && \
-  if [ "${SIG}" -eq 1 ] ; then \
-    name=${srcinstdir}/${src_patch_name} text="PATCH" sigfile ; \
-  fi && \
-  cp ${src_orig_pkg} ${srcinstdir}/${src_orig_pkg_name} && \
-  if [ -e ${src_orig_pkg}.sig ] ; then \
-    cp ${src_orig_pkg}.sig ${srcinstdir}/ ; \
-  fi && \
-  cp $0 ${srcinstdir}/`basename $0` && \
-  name=$0 text="SCRIPT" sigfile && \
-  if [ "${SIG}" -eq 1 ] ; then \
-    cp $0.sig ${srcinstdir}/ ; \
-  fi && \
-  cd ${srcinstdir} && \
+  (mkpatch
+  if [ "${SIG}" -eq 1 ] ; then
+    name=${srcinstdir}/${src_patch_name} text="PATCH" sigfile
+  fi
+  cp ${src_orig_pkg} ${srcinstdir}/${src_orig_pkg_name}
+  if [ -e ${src_orig_pkg}.sig ] ; then
+    cp ${src_orig_pkg}.sig ${srcinstdir}/
+  fi
+  cp $0 ${srcinstdir}/`basename $0`
+  name=$0 text="SCRIPT" sigfile
+  if [ "${SIG}" -eq 1 ] ; then
+    cp $0.sig ${srcinstdir}/
+  fi
+  cd ${srcinstdir}
   tar cvjf ${src_pkg} * )
 }
 finish() {
   rm -rf ${srcdir}
 }
 sigfile() {
-  if [ \( "${SIG}" -eq 1 \) -a \( -e $name \) -a \( \( ! -e $name.sig \) -o \( $name -nt $name.sig \) \) ]; then \
-    if [ -x /usr/bin/gpg ]; then \
-      echo "$text signature need to be updated"; \
-      rm -f $name.sig; \
-      /usr/bin/gpg --detach-sign $name; \
-    else \
-      echo "You need the gnupg package installed in order to make signatures."; \
-    fi; \
+  if [ \( "${SIG}" -eq 1 \) -a \( -e $name \) -a \( \( ! -e $name.sig \) -o \( $name -nt $name.sig \) \) ]; then
+    if [ -x /usr/bin/gpg ]; then
+      echo "$text signature need to be updated"
+      rm -f $name.sig
+      /usr/bin/gpg --detach-sign $name
+    else
+      echo "You need the gnupg package installed in order to make signatures."
+    fi
   fi
 }
 checksig() {
-  if [ -x /usr/bin/gpg ]; then \
-    if [ -e ${src_orig_pkg}.sig ]; then \
-      echo "ORIGINAL PACKAGE signature follows:"; \
-      /usr/bin/gpg --verify ${src_orig_pkg}.sig ${src_orig_pkg}; \
-    else \
-      echo "ORIGINAL PACKAGE signature missing."; \
-    fi; \
-    if [ -e $0.sig ]; then \
-      echo "SCRIPT signature follows:"; \
-      /usr/bin/gpg --verify $0.sig $0; \
-    else \
-      echo "SCRIPT signature missing."; \
-    fi; \
-    if [ -e ${src_patch}.sig ]; then \
-      echo "PATCH signature follows:"; \
-      /usr/bin/gpg --verify ${src_patch}.sig ${src_patch}; \
-    else \
-      echo "PATCH signature missing."; \
-    fi; \
+  if [ -x /usr/bin/gpg ]; then
+    if [ -e ${src_orig_pkg}.sig ]; then
+      echo "ORIGINAL PACKAGE signature follows:"
+      /usr/bin/gpg --verify ${src_orig_pkg}.sig ${src_orig_pkg}
+    else
+      echo "ORIGINAL PACKAGE signature missing."
+    fi
+    if [ -e $0.sig ]; then
+      echo "SCRIPT signature follows:"
+      /usr/bin/gpg --verify $0.sig $0
+    else
+      echo "SCRIPT signature missing."
+    fi
+    if [ -e ${src_patch}.sig ]; then
+      echo "PATCH signature follows:"
+      /usr/bin/gpg --verify ${src_patch}.sig ${src_patch}
+    else
+      echo "PATCH signature missing."
+    fi
   else
-    echo "You need the gnupg package installed in order to check signatures." ; \
+    echo "You need the gnupg package installed in order to check signatures."
   fi
 }
 while test -n "$1" ; do
   case $1 in
-    prep)		prep ; STATUS=$? ;;
-    mkdirs)		mkdirs ; STATUS=$? ;;
-    conf)		conf ; STATUS=$? ;;
-    configure)		conf ; STATUS=$? ;;
-    reconf)		reconf ; STATUS=$? ;;
-    build)		build ; STATUS=$? ;;
-    make)		build ; STATUS=$? ;;
-    check)		check ; STATUS=$? ;;
-    test)		check ; STATUS=$? ;;
-    clean)		clean ; STATUS=$? ;;
-    install)		install ; STATUS=$? ;;
-    list)		list ; STATUS=$? ;;
-    depend)		depend ; STATUS=$? ;;
-    strip)		strip ; STATUS=$? ;;
-    package)		pkg ; STATUS=$? ;;
-    pkg)		pkg ; STATUS=$? ;;
-    mkpatch)		mkpatch ; STATUS=$? ;;
-    acceptpatch)	acceptpatch ; STATUS=$? ;;
-    src-package)	spkg ; STATUS=$? ;;
-    spkg)		spkg ; STATUS=$? ;;
-    finish)		finish ; STATUS=$? ;;
-    checksig)		checksig ; STATUS=$? ;;
-    first)		mkdirs && spkg && finish ; STATUS=$? ;;
-    all)		checksig && prep && conf && build && install && \
-			strip && pkg && spkg && finish ; \
-			STATUS=$? ;;
+    prep)		prep ;;
+    mkdirs)		mkdirs ;;
+    conf)		conf ;;
+    configure)		conf ;;
+    reconf)		reconf ;;
+    build)		build ;;
+    make)		build ;;
+    check)		check ;;
+    test)		check ;;
+    clean)		clean ;;
+    install)		install ;;
+    list)		list ;;
+    depend)		depend ;;
+    strip)		strip ;;
+    package)		pkg ;;
+    pkg)		pkg ;;
+    mkpatch)		mkpatch ;;
+    acceptpatch)	acceptpatch ;;
+    src-package)	spkg ;;
+    spkg)		spkg ;;
+    finish)		finish ;;
+    checksig)		checksig ;;
+    first)		mkdirs ; spkg ; finish ;;
+    all)		checksig ; prep ; conf ; build ; install ;
+			strip ; pkg ; spkg ; finish ;;
     *) echo "Error: bad arguments" ; exit 1 ;;
   esac
-  ( exit ${STATUS} ) || exit ${STATUS}
   shift
 done
 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]