[PATCH util-modular 6/6] release.sh: run ./configure within the release.sh

[Emil Velikov]

On 13 November 2016 at 22:24, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> On Fri, Nov 11, 2016 at 03:45:29PM +0000, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> At the moment one has to run autogen.sh/configure in their checkout
>> prior to using release.sh
>>
>> We can "spare" that by folding it into the script. As a side effect this
>> gives us a number of nice benefits:
>>  - We can check/error out if build artefacts are present.
>> There's a noticeable number of projects which use srcdir _before_
>> builddir in their C*FLAGS.
>>
>>  - Can catch local hacks and workarounds for "broken" configure scripts.
>> By definition one should be able to run the following without ever
>> having problems.
>>   $ ./autogen.sh --prefix=`pwd`/foo && make && make install
>>
>> At the same time, there's a number of projects which would a) attempt to
>> install things outside of `pwd`/foo and/or b) otherwise require elevated
>> privileges during make install.
>>
>> Some of those are 'managed' by using custom configure options and
>> feeding those to AM_DISTCHECK_CONFIGURE_FLAGS.
>>
>> Let's avoid this by generating a unique build dir (./build.XXXXXXXXXX)
>> and doing the dist/build/check in there.
>
> if you want a truly clean repo: why not create the temporary build dir, then
> clone the repository into it and start with a completely fresh checkout.
> This way you can be sure that there's no detritus around that may mess with
> the build.
>
Doing a separate clone is quite wasteful imho. Something similar that
I've been thinking of is git worktree.
It provides an effectively clean env./checkout-like tree.

>>
>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>> ---
>> XXX:
>>  - For the future we might add support for custom build dirs (say
>> /ramdrive/foo) but for the moment keep things as-it.
>>
>>  - Should we cleanup the tmp/build dir ourselves, how about the
>> autotool generated files ?
>>
>>  - If people prefer we can guard this, or the original behaviour, behind
>> a --dist vs --distcheck like option.
Do you have a preference/suggestion on these ?

>> ---
>>  release.sh | 45 +++++++++++++++++++++++++--------------------
>>  1 file changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/release.sh b/release.sh
>> index 1b3703d..19e3ef9 100755
>> --- a/release.sh
>> +++ b/release.sh
>> @@ -345,31 +345,27 @@ process_module() {
>>       return 1
>>      fi
>>
>> +    if [ -e configure ]; then
>> +        echo "Error: the git repository contains configure"
>> +        echo "Did you forget to run git clean -fXd (and git clean -fxd) ?"
>
> shouldn't we suggest distclean here instead of git clean? because git clean
> can remove things that you may want to hang on to for sentimental reasons
> (e.g. previous tarballs, etc.)
>
Due to various reasons (bugs/other) distclean might not purge
everything. It would be great those that never happens, but for now
I'm leaning with the "more secure" option.

>> +        return 1
>> +    fi
>>
>> -    # Change directory to be in the git build directory (could be out-of-source)
>> -    # More than one can be found when distcheck has run and failed
>> -    configNum=`find . -name config.status -type f | wc -l | sed 's:^ *::'`
>> +    # Create tmpdir for the build
>> +    build_dir=`mktemp -d -p . build.XXXXXXXXXX`
>>      if [ $? -ne 0 ]; then
>> -     echo "Error: failed to locate config.status."
>> -     echo "Has the module been configured?"
>> -     return 1
>> -    fi
>> -    if [ x"$configNum" = x0 ]; then
>> -     echo "Error: failed to locate config.status, has the module been configured?"
>> -     return 1
>> -    fi
>> -    if [ x"$configNum" != x1 ]; then
>> -     echo "Error: more than one config.status file was found,"
>> -     echo "       clean-up previously failed attempts at distcheck"
>> -     return 1
>> +        echo "Error: could not create a temporary directory for the build."
>> +        echo "Do you have coreutils' mktemp ?"
>> +        return 1
>>      fi
>> -    status_file=`find . -name config.status -type f`
>> +
>> +    echo "Info: generating configure."
>> +    autoreconf --force --install >/dev/null
>>      if [ $? -ne 0 ]; then
>> -     echo "Error: failed to locate config.status."
>> -     echo "Has the module been configured?"
>> -     return 1
>> +        echo "Error: failed generate configure."
>
> failed *to* generate configure
>
>> +        return 1
>>      fi
>> -    build_dir=`dirname $status_file`
>> +
>>      cd $build_dir
>>      if [ $? -ne 0 ]; then
>>       echo "Error: failed to cd to $MODULE_RPATH/$build_dir."
>> @@ -377,6 +373,15 @@ process_module() {
>>       return 1
>>      fi
>>
>> +    # Using ../ here feels a bit nasty, yet $top_src is an absolute path. Thus
>> +    # it will get propagated in the generated sources, which we do not want.
>> +    ../configure >/dev/null
>> +    if [ $? -ne 0 ]; then
>> +        echo "Error: failed to configure module."
>> +        cd $top_src
>
> I'd really like to see more pushd/popd, but that's unrelated to this patch.
> Looks good, but I think you should look at the effort to do a full local
> clone, it may only be a few lines and it should remove quite a bit of the
> error checking.
>
Yes I'm thinking/working towards having things in a cleaner state. At
the same time this patch would cause a noticeable change in workflow,
so I've intentionally opted out of making things too intrusive.

Any objections against git worktree and/or moving towards it as a
follow-up change ?
Or you'd thinking this patch should be "it's all or nothing" kind of change ?

Thanks
Emil