Autotools pull in standard build system

Question

I have been updating my build tools to optionally us autotools (autoconfig/automake/libtool etc.).

As part of this change I have written a couple of M4 macros. This not being something I have done before, any input appreciated on style or if things can be done better.

AC_DEFUN(
    [AX_FUNC_THOR_BUILD],
    [
        AC_CHECK_PROGS([WGET], [wget], [:])
        if test "$WGET" = :; then
            AC_MSG_ERROR([This package needs wget.])
        fi

        AC_CHECK_PROGS([UNZIP], [unzip], [:])
        if test "$UNZIP" = :; then
            AC_MSG_ERROR([This package needs unzip.])
        fi

        AC_PROG_CXX

        git submodule init
        git submodule update
        pushd build/third
        ./setup "$CXX" || AC_MSG_ERROR([Failed to set up the test utilities], [1])
        popd
    ]
)

Should I pull the setup script into this macro or leave it separate?

#!/bin/bash
set -e
CXX=$1
#
# Change this as required to point at the root of ThorsAnvil code
ROOT=$(pwd)
THORSANVIL_ROOT=$(dirname ${ROOT})
if [[ -e ${THORSANVIL_ROOT}/lib/libgtest.a ]]; then
    exit 0
fi

echo "Un-archiving google test"
GTEST_ARCHIEVE=`ls gtest-*.zip 2>/dev/null | tail -n 1`
if [[ ${GTEST_ARCHIEVE} == "" ]]; then
    echo "Retrieving: gtest-1.7.0"
    wget http://googletest.googlecode.com/files/gtest-1.7.0.zip
    GTEST_ARCHIEVE=`ls gtest-*.zip 2>/dev/null | tail -n 1`
fi

GTEST_ARCHIEVE_DIR=${GTEST_ARCHIEVE%.zip}
if [[ ! -d ${GTEST_ARCHIEVE_DIR} ]]; then
    unzip ${GTEST_ARCHIEVE}
fi


echo "Building google test"
GTEST_DIR=${ROOT}/${GTEST_ARCHIEVE_DIR}
pushd ${GTEST_DIR}
${CXX} -I${GTEST_DIR}/include -I${GTEST_DIR} -c ${GTEST_DIR}/src/gtest-all.cc
${CXX} -I${GTEST_DIR}/include -I${GTEST_DIR} -c ${GTEST_DIR}/src/gtest_main.cc
ar -rv libgtest.a gtest-all.o gtest_main.o
popd


echo "Installing google test"
pushd ${THORSANVIL_ROOT}
mkdir -p bin include include3rd lib
rm -f lib/libgtest.*
cp ${GTEST_DIR}/libgtest.a lib/
rm -f include3rd/gtest
ln -s ${GTEST_DIR}/include/gtest include3rd/gtest
popd

Answer 1

The main M4 file

Should I pull the setup script into this macros or leave it separate?

I think it's good you separated. For the same reason that it's good to decompose a large function doing many things to smaller functions.

If it doesn't make sense to ever run the setup script independently, then an alternative is to put it in a function, if that's possible in M4.

This may be a matter of taste, I avoid pushd and popd in shell scripts for fear that I might forget to do popd, or that later I delete one of them by mistake. An alternative is a (...) subshell, for example:

    (
        cd build/third
        ./setup "$CXX" || AC_MSG_ERROR([Failed to set up the test utilities], [1])
    )

The subshell has its own environment, its own $PWD, so directory changes will not affect the encapsulating shell process. Forgetting one of the parens in (...) is impossible as that would be a blatant syntax error.

The setup script

It would be better to give the setup script a .sh extension to indicate it's a shell script.

---

Some variables containing filenames are not carefully quoted, for example:

THORSANVIL_ROOT=$(dirname ${ROOT})

This will break down if the path contains spaces.

Maybe on your computer you have the good sense to avoid spaces in paths of your dev stuff, but others cloning your project might not, and annoy you with dumb questions when something doesn't work. It's better to quote such variables. Paste your script on shellcheck.net and review all similar cases.

---

if [[ -e ${THORSANVIL_ROOT}/lib/libgtest.a ]]; then
    exit 0
fi

Instead of -e, which gives true for directories too, I'd use -f that more explicitly requires regular files. (It will match symlinks pointing to regular files too.) Sure, it's unlikely that a .a file would be a directory, but if that ever happens, it will really suck.

---

GTEST_ARCHIEVE=`ls gtest-*.zip 2>/dev/null | tail -n 1`
if [[ ${GTEST_ARCHIEVE} == "" ]]; then
    echo "Retrieving: gtest-1.7.0"
    wget http://googletest.googlecode.com/files/gtest-1.7.0.zip
    GTEST_ARCHIEVE=`ls gtest-*.zip 2>/dev/null | tail -n 1`
fi

Always prefer the modern $(...) subshells instead of the old and troublesome ``...`` style.

Also, since you're duplicating that ls ... command, it would be better to put that in a helper function. Even though it's just one line, it's not exactly trivial, and copy-pasted code very often comes back to haunt you.

Btw I guess you meant to name that variable GTEST_ARCHIVE instead of GTEST_ARCHIEVE.

As in the other script, I would wrap the pushd/popd in (...) instead.