Re: [PATCH v2 2/2] configure: Check bzip2 is available

[Thomas Huth <thuth@redhat.com>]

On 08/11/2019 18.43, Laszlo Ersek wrote:
> On 11/08/19 12:48, Daniel P. Berrangé wrote:
>> On Fri, Nov 08, 2019 at 12:45:31PM +0100, Philippe Mathieu-Daudé wrote:
>>> The bzip2 tool is not included in default installations.
>>> On freshly installed systems, ./configure succeeds but 'make'
>>> might fail later:
>>>
>>>     BUNZIP2 pc-bios/edk2-i386-secure-code.fd.bz2
>>>   /bin/sh: bzip2: command not found
>>>   make: *** [Makefile:305: pc-bios/edk2-i386-secure-code.fd] Error 127
>>>   make: *** Deleting file 'pc-bios/edk2-i386-secure-code.fd'
>>>   make: *** Waiting for unfinished jobs....
>>>
>>> Add a check in ./configure to warn the user if bzip2 is missing.
>>>
>>> Fixes: 536d2173b2b
>>> Reported-by: Thomas Huth <thuth@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> v2: use better English (Daniel)
>>> (Not taking Daniel Reviewed-by because logic changed)
>>> ---
>>>  configure | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>
>>> diff --git a/configure b/configure
>>> index 9b322284c3..2b419a8039 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -2147,6 +2147,7 @@ case " $target_list " in
>>>    ;;
>>>  esac
>>>  
>>> +# Some firmware binaries are compressed with bzip2
>>
>> Squash into previous patch
>>
>>>  for target in $target_list; do
>>>    case "$target" in
>>>      arm-softmmu | aarch64-softmmu | i386-softmmu | x86_64-softmmu)
>>> @@ -2154,6 +2155,9 @@ for target in $target_list; do
>>>        ;;
>>>    esac
>>>  done
>>> +if test "$edk2_blobs" = "yes" && ! has bzip2; then
>>> +  error_exit "The bzip2 program is required for building QEMU"
>>> +fi
>>>  
>>>  feature_not_found() {
>>>    feature=$1
>>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> I don't feel too strongly about this, but how about a different improvement:
> 
> - keep the comment in this patch, but move it right before the "has
> bzip2" check
> - instead of "some firmware binaries", clearly state "edk2 blobs".
> 
> Something like:
> 
> +# Edk2 blobs are compressed with bzip2.
> +if test "$edk2_blobs" = "yes" && ! has bzip2; then
> +  error_exit "The bzip2 program is required for building QEMU"
> +fi
> 
> So... now we have three variants: the one posted by Phil (v2), the one
> recommended by Dan (as an update to v2), and the one I suggest above (as
> a different update to v2). For simplicity, I'm fine with any one of the
> three variants going in.

I'll add the two patches to my "qtest + misc" PULL request that I'm
planning to send out tomorrow - and use Laszlo's suggestion for moving
the comment. I hope that's fine for everybody, if not please complain.

 Thomas