Re: [PATCH-for-4.16 1/2] configure: modify default of building rombios

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/11/2021 14:52, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH-for-4.16 1/2] configure: modify default of 
> building rombios"):
>> The tools/configure script will default to build rombios if qemu
>> traditional is enabled. If rombios is being built, ipxe will be built
>> per default, too.
>>
>> This results in rombios and ipxe no longer being built per default when
>> disabling qemu traditional.
>>
>> Fix that be rearranging the dependencies:
>>
>> - build ipxe per default
>> - build rombios per default if either ipxe or qemu traditional are
>>   being built
>>
>> This modification prepares not building qemu traditional per default
>> without affecting build of rombios and ipxe.
> Thanks.  We had a conversation on irc about this.  In summary:
>
> Reviewed-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>
>
>
> Discussion:
>
> Both before and after this patch --with-system-ipxe --disable-ipxe (or
> --with-system-ipxe on arm) would set IPXE_PATH but not cause ipxe to
> be actually built.  I think that is correct.
>
> We discussed the linkage between rombios and ipxe.  Our understanding
> is that thbe intent was to support two configurations:
>   qemu-trad + {hvmloader, rombios, ipxe}
>   qemu-upstraem + {hvmloader, seabios, pxe rom provided by qemu}
>
> Before this patch, --disable-rombios --enable-ipxe would be an error.
> After this patch, it is tolerated (and indeed, it might occur with
> only one of those options due to defaulting).  But we think that there
> is no actual functional link between these pieces: ie, nothing will
> actually go wrong.  You just might not have ipxe support since you
> wouldn't be using trad at all.
>
> We think that someone who explicitly manipulates the
> --en/disable-rombios and --en/disable-ipxe options so as to construct
> such a situation ought to know what they are doing and should get what
> the asked for.
>
> Most people will controlling this via --en/disable-qemu-trad, which
> will do a plausible. thing.
>
>
> I think *this* patch is a bugfix, although to rather obscure configure
> behaviour.  I am inclined to release ack both this and the followup,
> for the reasons I gave previously.
>
> Comments (especially, anything to contradict what I wrote above) ASAP
> please.

I think the patch will fix the problem from CI.  And from that point of
view it is an improvement.

However,  I don't think the help text is correct any more.  Specifically:

+       [Use system supplied IPXE PATH instead of building and installing
+        our own version, it takes precedence over --{en,dis}able-ipxe
and is
+        bound by the presence of rombios, --without-system-ipxe is an
error]),[

and the binding to RomBIOS is no longer true AFAICT.  At a minimum, I
think the wording there needs tweaking.

If tweaking is going on, then "per default" in the commit message feels
a little awkward, and would more normally be "by default".

Otherwise, LGTM.

~Andrew