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