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

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

[Ian Jackson]

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(-)

-- 
2.20.1

[OSSTEST PATCH 1/2] ts-xen-build: Refactor enable/disable configure options

[Ian Jackson]

Replace the repeated pattern with a function to generate that code.
No significant functional change.

Signed-off-by: Ian Jackson <iwj@xenproject.org>
CC: Juergen Gross <jgross@suse.com>
---
 ts-xen-build | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/ts-xen-build b/ts-xen-build
index ccb2aba23..d6f6bfacb 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -142,9 +142,21 @@ END
 }
 
 sub build () {
-    my $xend_opt= $r{enable_xend} =~ m/true/ ? "--enable-xend" : "--disable-xend";
-    my $ovmf_opt= $r{enable_ovmf} =~ m/true/ ? "--enable-ovmf" : "--disable-ovmf";
-    my $qemutrad_opt = $r{arch} =~ m/amd64|i386/ ? "--enable-qemu-traditional" : "--disable-qemu-traditional";
+    my $enable_opts = ''; # shell script to set "enable_opts" shell var
+    my $enable_disable = sub {
+	my ($subdir, $feat, $enable) = @_;
+	my $opt = "--".($enable ? 'enable' : 'disable')."-$feat";
+	$enable_opts .= <<END;
+                if grep -q -- $opt \\
+                               ${subdir}configure ; then
+		    enable_opts="\$enable_opts $opt"
+                fi
+END
+    };
+    
+    $enable_disable->("tools/", "xend", $r{enable_xend} =~ m/true/);
+    $enable_disable->("tools/", "ovmf", $r{enable_ovmf} =~ m/true/);
+    $enable_disable->("tools/", "qemu-traditional", $r{arch} =~ m/amd64|i386/);
 
     my $configure_prefix = $r{cmdprefix_configure} // '';
     my $configure_suffix = $r{cmdsuffix_configure} // '';
@@ -152,17 +164,9 @@ sub build () {
 
     buildcmd_stamped_logged(600, 'xen', 'configure', <<END,<<END,<<END);
             if test -f configure; then
-                if grep -q -- $xend_opt tools/configure ; then
-		    xend=$xend_opt
-                fi
-                if grep -q -- $ovmf_opt tools/configure ; then
-                    ovmf=$ovmf_opt
-                fi
-                if grep -q -- $qemutrad_opt tools/configure ; then
-                    qemutrad=$qemutrad_opt
-                fi
+$enable_opts
 END
-               $configure_prefix ./configure --sysconfdir=/etc \$xend \$ovmf \$qemutrad $configure_suffix @configure_args
+               $configure_prefix ./configure --sysconfdir=/etc \$enable_opts $configure_suffix @configure_args
 END
             fi
 END
-- 
2.20.1

[OSSTEST PATCH 2/2] ts-xen-build: Pass --enable if --disable found in usage, and v.v.

[Ian Jackson]

The existing code works in practice if the usage message always lists
the non-default, since the unlisted-in-usage options that would be
supported, but are elided, are in any case the default.

But configure might *compute* its defaults.  In which case it will
list only one of them in the usage message.  If the computed default
is not the same as the usual default (the one implied by listing the
opposite in the usage message) we would wrongly not pass the option.

So grep for both enable and disable.

Signed-off-by: Ian Jackson <iwj@xenproject.org>
CC: Juergen Gross <jgross@suse.com>
---
 ts-xen-build | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/ts-xen-build b/ts-xen-build
index d6f6bfacb..c294a51ea 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -142,12 +142,20 @@ END
 }
 
 sub build () {
+    # We want to explicitly enable and disable some things.  But not
+    # all versions of Xen support all configuration options.  We
+    # detect presence of an option by grepping configure.  That finds
+    # them in the usage message.  The usage message has only one of
+    # the two, depending on the usual default.  (Presence of --enable
+    # in the usage output means --disable is supported, and vice
+    # versa.)  So we search for both enable and disable, and if either
+    # is found, we use the one we want.
     my $enable_opts = ''; # shell script to set "enable_opts" shell var
     my $enable_disable = sub {
 	my ($subdir, $feat, $enable) = @_;
 	my $opt = "--".($enable ? 'enable' : 'disable')."-$feat";
 	$enable_opts .= <<END;
-                if grep -q -- $opt \\
+                if egrep -q -- '--disable-$feat|--enable-$feat' \\
                                ${subdir}configure ; then
 		    enable_opts="\$enable_opts $opt"
                 fi
-- 
2.20.1

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2014 bytes --]

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 <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Ian Jackson]

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 <jgross@suse.com>

Thank you for your detailed review and analysis.

I have pushed this to osstest pretest.

Ian.