Re: [PATCH] configure: add support for psuedo-"in source tree" builds

[Philippe Mathieu-Daudé]

On 8/20/20 7:42 PM, Eric Blake wrote:
> On 8/20/20 11:55 AM, Daniel P. Berrangé wrote:
>> Meson requires the build dir to be separate from the source tree. Many
>> people are used to just running "./configure && make" though and the
>> meson conversion breaks that.
>>
>> This introduces some backcompat support to make it appear as if an
>> "in source tree" build is being done, but with the the results in the
>> "build/" directory. This allows "./configure && make" to work as it
>> did historically, albeit with the output binaries staying under build/.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---

[*]

> 
> In addition to reviews you already have,
> 
> 
>> I've not tested it beyond that. Note it blows away the "build/"
>> dir each time ./configure is run so it is pristine each time.
> 
> I definitely like the idea of only blowing away what we created - but if
> we created build, then recreating it for each new configure run is nice.
> 
>>
>> We could optionally symlink binaries from build/ into $PWD
>> if poeople think that is important, eg by changing GNUmakefile
>> to have:
>>
>> recurse: all
>>     for bin in `find build -maxdepth 1 -type f -executable | grep -v
>> -E '(ninjatool|config.status)'`; \
> 
> Using -maxdepth gets rid of the need to pre-create empty directories for
> nested binaries, but also loses out on binaries such as
> x86_64-softmmu/qemu-system-x86_64.  Oh, it looks like meson creates
> qemu-system-x86_64 as a binary in the top level, then a symlink in its
> old location.  Populating symlinks to ALL old locations is thus trickier
> than what you are proposing here, so it is fine to save that for a
> followup patch (let's get the bare minimum in first, so that at least
> ./configure && make works, before we worry about back-compat symlinks).
> 
>>
>> This goes on top of Paolo's most recent meson port v175 posting,
>> or whatever number it is upto now :-)
> 
> Nice comment for reviewers, but doesn't quite need to be preserved in git.

Already below '---' in [*] ;)

> 
>>
>>   .gitignore |  2 ++
>>   configure  | 40 ++++++++++++++++++++++++++++++++--------
>>   2 files changed, 34 insertions(+), 8 deletions(-)
>>
>> diff --git a/.gitignore b/.gitignore
>> index 92311284ef..4ccb9ed975 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -1,3 +1,5 @@
>> +/GNUmakefile
>> +/build/
>>   /.doctrees
>>   /config-devices.*
>>   /config-all-devices.*
>> diff --git a/configure b/configure
>> index cc5f58f31a..a5c88ad1ac 100755
>> --- a/configure
>> +++ b/configure
>> @@ -11,6 +11,38 @@ unset CLICOLOR_FORCE GREP_OPTIONS
>>   # Don't allow CCACHE, if present, to use cached results of compile
>> tests!
>>   export CCACHE_RECACHE=yes
>>   +source_path=$(cd "$(dirname -- "$0")"; pwd)
> 
> This behaves wrong if CDPATH is set in the environment.  We should
> really include CDPATH in our environment sanitization at the top of the
> file.
> 
>> +
>> +if printf %s\\n "$source_path" "$PWD" | grep -q "[[:space:]:]";
>> +then
>> +  error_exit "main directory cannot contain spaces nor colons"
>> +fi
>> +
>> +if test "$PWD" == "$source_path"
> 
> bashism; s/==/=/ or you will break configure on dash systems
> 
>> +then
>> +    echo "Using './build' as the directory for build output"
> 
> Do we want a way for a user to type './configure builddir=build/' and
> 'make builddir=build/' so they can specify different builddir overrides
> per invocation (of course, where builddir defaults to 'build/' if not
> specified)?  But hardcoding to _just_ ./build/ for getting this patch in
> quickly is fine.
> 
>> +    rm -rf build
>> +    mkdir -p build
>> +    cat > GNUmakefile <<EOF
> 
> If you use 'EOF' or \EOF here, then...
> 
>> +
>> +ifeq (\$(MAKECMDGOALS),)
> 
> you wouldn't have to escape all these $.  Looking through the file...
> 
>> +recurse: all
>> +endif
>> +
>> +.NOTPARALLEL: %
>> +%: force
>> +    @echo 'changing dir to build for \$(MAKE) "\$(MAKECMDGOALS)"...'
>> +    @\$(MAKE) -C build -f Makefile \$(MAKECMDGOALS)
>> +    if test "\$(MAKECMDGOALS)" = "distclean" ; then rm -rf build ; fi
>> +force: ;
>> +.PHONY: force
>> +GNUmakefile: ;
>> +
>> +EOF
> 
> ...I didn't see any use of $ that was not supposed to be literally in
> the generated GNUmakefile.
> 
>> +    cd build
>> +    exec $source_path/configure "$@"
>> +fi
>> +
>>   # Temporary directory used for files created while
>>   # configure runs. Since it is in the build directory
>>   # we can safely blow away any previous version of it
> 
> Now that we are guaranteeing configure is run in a build directory, this
> part of configure might have some cleanups possible.  But that can be a
> separate patch.
> 
>> @@ -297,14 +329,6 @@ ld_has() {
>>       $ld --help 2>/dev/null | grep ".$1" >/dev/null 2>&1
>>   }
>>   -# make source path absolute
>> -source_path=$(cd "$(dirname -- "$0")"; pwd)
>> -
>> -if printf %s\\n "$source_path" "$PWD" | grep -q "[[:space:]:]";
>> -then
>> -  error_exit "main directory cannot contain spaces nor colons"
>> -fi
>> -
>>   # default parameters
>>   cpu=""
>>   iasl="iasl"
>>
> 
> Looking forward to v2.
>