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é <berra...@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.
>
