[OSSTEST,0/2] ts-xen-build: explicitly enable/disable configure features

Message

Ian Jackson Oct. 27, 2021, 5:02 p.m. UTC

The existing code depends on precisely whether the non-default option
appearing in the configure script is indeed the opposite of the option
we want to pass.

Right now it seems to be working but this seems fragile.  Do it
differently.

I have verified that with current Xen, on arm64:

   + egrep -q -- '--disable-xend|--enable-xend' tools/configure
   + egrep -q -- '--disable-ovmf|--enable-ovmf' tools/configure
   + enable_opts=' --enable-ovmf'
   + egrep -q -- '--disable-qemu-traditional|--enable-qemu-traditional' tools/configure
   + enable_opts=' --enable-ovmf --disable-qemu-traditional'
   + ./configure --sysconfdir=/etc --enable-ovmf --disable-qemu-traditional

and on amd64:

   + egrep -q -- '--disable-xend|--enable-xend' tools/configure
   + egrep -q -- '--disable-ovmf|--enable-ovmf' tools/configure
   + enable_opts=' --enable-ovmf'
   + egrep -q -- '--disable-qemu-traditional|--enable-qemu-traditional' tools/configure
   + enable_opts=' --enable-ovmf --enable-qemu-traditional'
   + ./configure --sysconfdir=/etc --enable-ovmf --enable-qemu-traditional

Juergen, I would appreciate a review from you.  I think I would like
this in osstest production before changing the qemu trad build default
in Xen.

Ian Jackson (2):
  ts-xen-build: Refactor enable/disable configure options
  ts-xen-build: Pass --enable if --disable found in usage, and v.v.

 ts-xen-build | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

Comments

Jürgen Groß Oct. 28, 2021, 11:52 a.m. UTC | #1

On 27.10.21 19:02, Ian Jackson wrote:
> The existing code depends on precisely whether the non-default option
> appearing in the configure script is indeed the opposite of the option
> we want to pass.
> 
> Right now it seems to be working but this seems fragile.  Do it
> differently.
> 
> I have verified that with current Xen, on arm64:
> 
>     + egrep -q -- '--disable-xend|--enable-xend' tools/configure
>     + egrep -q -- '--disable-ovmf|--enable-ovmf' tools/configure
>     + enable_opts=' --enable-ovmf'
>     + egrep -q -- '--disable-qemu-traditional|--enable-qemu-traditional' tools/configure
>     + enable_opts=' --enable-ovmf --disable-qemu-traditional'
>     + ./configure --sysconfdir=/etc --enable-ovmf --disable-qemu-traditional
> 
> and on amd64:
> 
>     + egrep -q -- '--disable-xend|--enable-xend' tools/configure
>     + egrep -q -- '--disable-ovmf|--enable-ovmf' tools/configure
>     + enable_opts=' --enable-ovmf'
>     + egrep -q -- '--disable-qemu-traditional|--enable-qemu-traditional' tools/configure
>     + enable_opts=' --enable-ovmf --enable-qemu-traditional'
>     + ./configure --sysconfdir=/etc --enable-ovmf --enable-qemu-traditional
> 
> Juergen, I would appreciate a review from you.  I think I would like
> this in osstest production before changing the qemu trad build default
> in Xen.

Far from being a Perl expert I agree this is a sensible approach and it
should do the right thing.

It will still depend on no unsupported option being mentioned in any
comment, e.g. "# option --enable-foo is no longer supported" will result
in a wrong positive when testing for feature "foo". In the end this will
break the build, so it should be easy to detect in case this happens
some time in the future.

As there is no way to print out all supported options, this could only
be solved by adding "--disable-option-checking", which has other
disadvantages.

You can add my:

Reviewed-by: Juergen Gross <[e-mail]>


Juergen

Ian Jackson Oct. 28, 2021, 12:29 p.m. UTC | #2

Juergen Gross writes ("Re: [OSSTEST PATCH 0/2] ts-xen-build: explicitly enable/disable configure features"):
> Far from being a Perl expert I agree this is a sensible approach and it
> should do the right thing.
> 
> It will still depend on no unsupported option being mentioned in any
> comment, e.g. "# option --enable-foo is no longer supported" will result
> in a wrong positive when testing for feature "foo". In the end this will
> break the build, so it should be easy to detect in case this happens
> some time in the future.
> 
> As there is no way to print out all supported options, this could only
> be solved by adding "--disable-option-checking", which has other
> disadvantages.
> 
> You can add my:
> 
> Reviewed-by: Juergen Gross <[e-mail]>

Thank you for your detailed review and analysis.

I have pushed this to osstest pretest.

Ian.