Re: Patch 3/3 for topic/libposix

[Gary V. Vaughan]

On 12 Nov 2010, at 08:15, Bruce Korb wrote:
> Hi Gary, et al.,

Hi Bruce,

If you can configure your email program to quote patches inline
without mangling them, it makes giving review feedback much
easier (otherwise I have to save the patch, open it in an editor,
copy the content of the patch, and then 'paste-as-quotation' back
into my mail reply).

> Since the module
> file is now created by capturing the output of posix-modules,
> I also removed the validation from the libposix/bootstrap
> script.

Agreed.

>  I also augmented it to follow through with building
> the distro with a "--distro" option.  In fact:
> 
>   cd libposix ; ./bootstrap --all
> 
> will just do it all.

This is entirely unidiomatic.  I'd much rather keep this already well
established and expected process outside of the bootstrap script:

   ./bootstrap
   ./configure --what --ever --options I need
   make all check
   sudo make install

> The reason for recreating configure.ac, by the way, is that
> I just found it a lot easier to capture the git-version-gen
> output and insert the value with a here-doc instead of
> messing with an unmodified ".in" file and using sed.

But this is already the accepted way of using git-version-gen,
as used by key packages like coreutils, m4 and others, so if
it is not working for you, we should fix it properly rather
than kludge around it.

Also, I'm not at all convinced of the advantage of putting the
contents of `configure.ac' inside `bootstrap' at this juncture;
it just means that I have to remember that (unlike just about
every other GNU flavour project) I have to edit `bootstrap'
when I want to change the `configure.ac' file.

Further review comments interspersed below:

> From f23b520342d11559b21db9911c1dd3ef7c6c41e2 Mon Sep 17 00:00:00 2001
> From: Bruce Korb <address@hidden>
> Date: Thu, 11 Nov 2010 08:48:17 -0800
> Subject: [PATCH 3/3] Rewrite bootstrap to create the libposix module file and 
> the
>  configure.ac files on the fly.  Also, enable it to construct
>  the distribution tarball in one go.

Don't forget a properly formatted ChangeLog entry too.

> ---
>  .gitignore            |    1 +
>  libposix/.gitignore   |    4 +
>  libposix/bootstrap    |  183 +++++++++++++++++++++++++----
>  libposix/configure.ac |   35 ------
>  modules/libposix      |  318 
> -------------------------------------------------
>  5 files changed, 167 insertions(+), 374 deletions(-)
>  delete mode 100644 libposix/configure.ac
>  delete mode 100644 modules/libposix
> 
> diff --git a/.gitignore b/.gitignore
> index b95fb40..a9a9402 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -6,3 +6,4 @@
>  allsnippets.tmp
>  amsnippet.tmp
>  testdir*
> +*.diff

The above is a separate patch for master IMHO.

> diff --git a/libposix/.gitignore b/libposix/.gitignore
> index b078d37..fe3cbd6 100644
> --- a/libposix/.gitignore
> +++ b/libposix/.gitignore
> @@ -46,3 +46,7 @@ stamp-h1
>  /tests/test-*
>  unused-parameter.h
>  warn-on-use.h
> +/_*

Really? That's terribly permissive, and I don't see where
the existing code generates file droppings with a leading
underscore (I didn't look terribly hard though).  If this
is necessary, then naming the specific files is better.

> +/configure.ac
> +/tests
> +/tmp
> diff --git a/libposix/bootstrap b/libposix/bootstrap
> index 987bd31..94f1eed 100755
> --- a/libposix/bootstrap
> +++ b/libposix/bootstrap
> @@ -1,30 +1,171 @@
>  #! /bin/sh
> +# -*- Mode: Shell-script -*-
>  
> -PATH=..:$PATH
> +die() {

No cuddling in GNU code!

> +    echo "mk-libposix-module.sh error:  $*" >&2

The following is more idiomatic:

: ${SED=sed}

EXIT_SUCCESS=0
EXIT_FAILURE=1

basename='s|^.*/||'

progpath="$0"
progname=`echo "$progpath" |$SED "$basename"`

func_error ()
{
    echo "$progname: $*" >&2
}

func_fatal_error ()
{
    func_error ${1+"$@"}
    exit $EXIT_FAILURE
}

> +    trap '' EXIT

Named signals are not portable in general, and you can't set
a trap on exit 0 inside a function as you do in init() below
(see autoconf.info(Shellology) for details). Arguably, this
script should only be executed by maintainers with a modern
set of tools, but I'd really like for people who want to help
us debug on obscure platforms to be able to rebootstrap on
their test machine when it's so easy to support.

It's more idiomatic to trap only abnormal exits (1 2 13 15)
outside of any function calls and then to explicitly call the
clean up function just before normal script exit (avoiding the
need for "trap '' 0" in several places):

trap 'x=$?; func_cleanup "$file_droppings"; exit $x' 1 2 13 15

> +    test ${#clean_list} -gt 1 && \

XSI extensions are not guaranteed, and backslash is not required
with a trailing && (although other large GNU shell scripts
tend to follow the style of trailing \ with && at the start
of the following line so that you can easily spot continuations
by scanning either the left or right end of lines):

      test -n "$file_droppings" \
        && ...

or if you are worried about spurious whitespace:

      tmp_file_droppings=$file_droppings
      test -n "$tmp_file_droppings" \
        && ...

> +        echo '*NOT*:  rm -rf' ${clean_list}
> +    kill -TERM $progpid

kill -15 $progpid

or if you prefer:

SIGTERM=15

kill -$SIGTERM $progpid

Although I'm not sure what that buys you over the simple trap and
func_cleanup idiom, and would rather let the script fail with an
informative exit status.

> +    exit 1
> +}
>  
> -# Bootstrap for autotools.
> -gnulib-tool --import --lib=libposix --makefile-name=gnulib.mk \
> -  --macro-prefix=LIBPOSIX --libtool --no-changelog --symlink \
> -  --with-tests --with-c++-tests --with-longrunning-tests \
> -  git-version-gen libposix
> +do_or_die() {
> +    "$@" || die "FAILED: $*"
> +}
>  
> +clean_list=''
> +cleanup() {
> +    trap '' EXIT
> +    test ${#clean_list} -gt 1 && \
> +        rm -rf ${clean_list}
> +    return 0

return is not portable, and in light of my other review suggestions
not required:

func_cleanup ()
{
    test -n "$*" && rm -rf "$@"
}

> +}
>  
> -# No need to maintain a Makefile.am just to include gnulib.mk.
> -mv tests/gnulib.mk tests/Makefile.am
> +init() {

Bleh, that's just a function for the sake of it.  Keep top-level
execution outside of functions to avoid weirdness in plenty of
shells.

> +    progpid=$$

I don't think this is necessary, as the script can kill itself
more reliably with 'exit $?' than 'kill -15 $$' anyway, and without
corrupting the exit status and hiding from the user what the real
reason for exiting was.

> +    progdir=`dirname $0`
> +    prognam=`basename $0`

Neither dirname nor basename are portable, use sed instead.

> +    glibdir=`cd ${progdir} >/dev/null && cd .. && pwd`
>  
> +    cd ${progdir}
> +    PATH=$glibdir:$PATH

No need for PATH *and* cd here AFAICT, one or the other is fine.

> -# Sanity check the module list for synchronisation issues.
> -{
> -    sed_noblanks='/^$/d'
> -    posix-modules |sed -e "$sed_noblanks" -e 's|$| posix-modules|'
> -    gnulib-tool --extract-dependencies libposix \
> -        |sed -e "$sed_noblanks" -e 's|$| libposix|'
> -} | awk '_[$1] {delete _[$1]; next}
> -               {_[$1]=$2}
> -         END   {for (k in _)
> -                  printf ("bootstrap: warning: `%s'\'' only appears in 
> %s\n", k, _[k])}' \
> -  | sort

Agreed!

> +    case "X$1" in
> +    X--all )
> +        set -- --clean --distro
> +        ;;
> +    esac

Eww. That is entirely new and peculiar to just this script,
and violates the principle of least surprise.  Lets trust the
developer to know when he wants to rebuild and let him choose
his own arguments, and trust him to be (or become) familiar with
the standard mechanisms established in every other bootstrap
using GNU package.

> +    case "X$1" in
> +    X--clean* )
> +        git clean -f -d -x .
> +        shift
> +        test $# -eq 0 && exit 0
> +        ;;
> +    esac

Likewise.  And certainly we shouldn't be running 'git clean'
behind the developer's back, deleting any temporary files she
deliberately created (painstaking and time-consuming test
logs for one).

> -# Run autotools.
> -autoreconf --force --install --verbose --symlink
> +    trap die EXIT
> +    case "X$1" in
> +    X--dist* )
> +        do_distro=true
> +        ;;
> +
> +    * ) do_distro=false
> +        ;;
> +    esac
> +}

Likewise.

> +mk_module() {
> +    do_or_die mkdir tmp

Using 'set -e' is more idiomatic than passing every external command
to a sentinel function.

> +    clean_list=tmp
> +    do_or_die mkdir tmp/modules

Likewise.

> +    (   echo alloca
> +        posix-modules

No need to fork here, braces work equally well.

> +    ) | sort -u > tmp/posix-list
> +
> +    posix_list=$(grep -v '^$' tmp/posix-list)

No need to fork here either (nor am I convinced about the
portability of <<- for heredocs):

{
  cat <<EOF
Description:
...
Depends-on:
EOF
  {
    echo alloca
    posix-modules
  } | sort -u | $SED -e '/^$/d'
cat <<EOF

configure.ac:
...
EOF
} > tmp/modules/libposix

> +run_glt() {
> +    v=$(${glibdir}/build-aux/git-version-gen ./.tarball-version | \
> +        sed 's/-dirty/-modified/')
> +
> +    # AC_USE_SYSTEM_EXTENSIONS
> +    # this should be AC_REQUIRED by gnulib modules that need it,
> +    # but either a couple of modules have forgotten it, or else
> +    # AC_REQUIRE is emitting macro expansions out of order
> +    #
> +    # AC_CONFIG_MACRO_DIR
> +    # we can't use AC_CONFIG_AUX_DIR here, because the heuristics
> +    # for finding install-sh in the generated configure script
> +    # consider this directory to be a subproject of gnulib proper,
> +    # and will only look for install-sh in . and .. :(
> +    # AC_CONFIG_AUX_DIR([build-aux])
> +    #
> +    cat > configure.ac <<- _EOF_
> +     AC_INIT([GNU libposix],[${v}],address@hidden)
> +     AS_BOX([Configuring AC_PACKAGE_TARNAME AC_PACKAGE_VERSION])
> +     AC_USE_SYSTEM_EXTENSIONS
> +     AC_CONFIG_MACRO_DIR([m4])
> +     AC_CONFIG_HEADER([config.h])
> +     AC_CONFIG_FILES([Makefile lib/Makefile tests/Makefile])
> +     AM_INIT_AUTOMAKE([foreign])
> +     LT_INIT
> +     AC_SUBST([LTV_CURRENT], 0)
> +     AC_SUBST([LTV_REVISION], 0)
> +     AC_SUBST([LTV_AGE], 0)
> +     AC_PROG_CC
> +     LIBPOSIX_EARLY
> +     AM_PROG_CC_C_O
> +     LIBPOSIX_INIT
> +     AC_OUTPUT
> +     _EOF_

I really do dislike putting configure.ac inside bootstrap for the
reasons I stated above.

> +    opts='
> +     --local-dir=tmp
> +     --import
> +     --lib=libposix
> +     --makefile-name=gnulib.mk
> +     --macro-prefix=LIBPOSIX
> +     --libtool
> +     --no-changelog
> +     --symlink
> +     --with-tests
> +     --with-c++-tests
> +     --with-longrunning-tests'
> +
> +    do_or_die gnulib-tool ${opts} libposix

No need for an extra opts variable, or the spurious do_or_die, or
indeed for encapsulating this string of commands into a one-time
thunk.  Just list the commands that are needed at the top-level.

> +    # No need to maintain a Makefile.am just to include gnulib.mk.
> +    mv tests/gnulib.mk tests/Makefile.am
> +}
> +
> +run_reconf() {
> +    # Run autotools.
> +    do_or_die autoreconf --force --install --verbose --symlink
> +}

set -e
autoreconf --force --install --verbose --symlink

is much easier to understand, and several lines shorter than:

do_or_die ()
{
   ...
}

run_reconf ()
{
  ...
}

run_reconf

> +mk_distro() {
> +    mkdir _build _install
> +    dd=$PWD/_install
> +    cd _build
> +    do_or_die ../configure --prefix=/usr/local
> +    do_or_die make
> +    do_or_die make dist
> +    do_or_die make install DESTDIR=${dd}
> +    do_or_die make check
> +}

This is not bootstrap's job.  If you want it, then create a
separate helper script for yourself.

> +PS4='>${FUNCNAME:-bs}> ' ; set -x

Definitely not portable, if bootstrap is working properly then
I'd rather it do so reasonably quietly.  If it's not working,
I'll happily rerun it myself with '/bin/sh -x ./bootstrap'.

> +init ${1+"$@"}

Clearer to inline the contents of init here and not worry about
passing command line arguments through to the function correctly.

> +mk_module
> +run_glt
> +run_reconf

Clearer, faster and shorter to inline these thunks here too.

> +${do_distro} && mk_distro

Not bootstrap's job.

> +cleanup

Agreed (although I'm surprised to see this here considering all
the "trap '' EXIT" dancing earlier...).

> diff --git a/libposix/configure.ac b/libposix/configure.ac
> deleted file mode 100644
> index c68fa48..0000000
> --- a/libposix/configure.ac
> +++ /dev/null
> [[snip]]

I'd rather keep configure.ac for the reasons stated above.

> diff --git a/modules/libposix b/modules/libposix
> deleted file mode 100644
> index f43874d..0000000
> --- a/modules/libposix
> +++ /dev/null
> [[snip]]

Agreed.

Cheers,
-- 
Gary V. Vaughan (address@hidden)

PGP.sig
Description: This is a digitally signed message part