Re: [PATCH 2/3] configure: Remove options that can be handled via meson_options.txt instead

[Thomas Huth]

On 30/08/2021 17.06, Eric Blake wrote:
> On Sun, Aug 29, 2021 at 07:32:09PM +0200, Thomas Huth wrote:
> > These trivial options can now be handled via the new generic code
> > that parses meson_options.txt
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   configure         | 316 +---------------------------------------------
> >   meson_options.txt |   2 +-
> >   2 files changed, 5 insertions(+), 313 deletions(-)
>
> Picking on one example...
>
> > diff --git a/configure b/configure
> > index b3e6d51916..cb125c3f84 100755
> > --- a/configure
> > +++ b/configure
> > @@ -291,34 +291,14 @@ for opt do
> >     esac
> >   done
> >   
> > -brlapi="auto"
>
> > -  --disable-brlapi) brlapi="disabled"
> > -  ;;
> > -  --enable-brlapi) brlapi="enabled"
> > -  ;;
>
> > @@ -5213,25 +4920,10 @@ if test "$skip_meson" = no; then
> >           -Db_pie=$(if test "$pie" = yes; then echo true; else echo false; fi) \
> >           ${staticpic:+-Db_staticpic=$staticpic} \
> >           -Db_coverage=$(if test "$gcov" = yes; then echo true; else echo 
> > false; fi) \
> > -        -Db_lto=$lto -Dcfi=$cfi -Dcfi_debug=$cfi_debug \
> > -        -Dmalloc=$malloc -Dmalloc_trim=$malloc_trim -Dsparse=$sparse \
> > -        -Dkvm=$kvm -Dhax=$hax -Dwhpx=$whpx -Dhvf=$hvf -Dnvmm=$nvmm \
> > -        -Dxen=$xen -Dxen_pci_passthrough=$xen_pci_passthrough -Dtcg=$tcg \
> > -        -Dcocoa=$cocoa -Dgtk=$gtk -Dmpath=$mpath -Dsdl=$sdl 
> > -Dsdl_image=$sdl_image \
> > -        -Dlibusb=$libusb -Dsmartcard=$smartcard -Dusb_redir=$usb_redir 
> > -Dvte=$vte \
> > -        -Dvnc=$vnc -Dvnc_sasl=$vnc_sasl -Dvnc_jpeg=$vnc_jpeg 
> > -Dvnc_png=$vnc_png \
> > -        -Dgettext=$gettext -Dxkbcommon=$xkbcommon -Du2f=$u2f 
> > -Dvirtiofsd=$virtiofsd \
> > -        -Dcapstone=$capstone -Dslirp=$slirp -Dfdt=$fdt -Dbrlapi=$brlapi \
>
> Pre-patch, if you used neither --enable-brlapi nor --disable-brlapi,
> we called $meson with -Dbrlapi=auto.
>
> > +        -Db_lto=$lto -Dcfi=$cfi -Dcfi_debug=$cfi_debug -Dmalloc=$malloc \
> > +        -Ddefault_devices=$default_devices -Dxen=$xen -Dtcg=$tcg -Dsdl=$sdl \
> > +        -Dcapstone=$capstone -Dslirp=$slirp -Dfdt=$fdt \
> > +        -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
> >           $(if test "$default_feature" = no; then echo 
> > "-Dauto_features=disabled"; fi) \
> >           -Dtcg_interpreter=$tcg_interpreter $meson_options \
>
> Post-patch, if you use an option, $meson_options includes
> -Dbrlapi=disabled or -Dbrlapi=enabled (per patch 1), but if you omit
> an option, now we aren't passing anything.  Does meson treat
> -Dbrlapi=auto and the absence of any mention of brlapi identically?

I can confirm what you've already discovered later in your review already: 
The "auto" setting is used by default in meson_options.txt already.

> > diff --git a/meson_options.txt b/meson_options.txt
> > index a9a9b8f4c6..2c89e79e8b 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -120,7 +120,7 @@ option('usb_redir', type : 'feature', value : 'auto',
> >          description: 'libusbredir support')
> >   option('virglrenderer', type : 'feature', value : 'auto',
> >          description: 'virgl rendering support')
> > -option('vnc', type : 'feature', value : 'enabled',
> > +option('vnc', type : 'feature', value : 'auto',
> >          description: 'VNC server')
>
> Why is the default for this option changed?  It looks unrelated to the
> mechanical changes in the rest of the patch, enough so to warrant its
> own patch, or at least special mention in the commit message.

Agreed, I think I'll best split it into a separate patch. For the context: I 
should have changed this already in 3a6a1256d46daa2121 ("configure: Allow 
vnc to get disabled with --without-default-features"), but I was not aware 
of the duplicated values between meson_options.txt and the configure script 
at that point in time yet.

 Thomas