[PATCH 0/4] Fixes for the --without-default-features configure switch

[PATCH 0/4] Fixes for the --without-default-features configure switch

[Thomas Huth]

Many features do not get properly disabled when the user runs the
configure script with --without-default-features. Let's fix that now.

Thomas Huth (4):
  configure: Fix --without-default-features propagation to meson
  configure: Allow vnc to get disabled with --without-default-features
  configure: Fix the default setting of the "xen" feature
  configure: Let --without-default-features disable vhost-kernel and
    vhost-vdpa

 configure   | 8 +++++---
 meson.build | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH 1/4] configure: Fix --without-default-features propagation to meson

[Thomas Huth]

A typo prevents that many features get disabled when the user
runs "configure" with the --without-default-features switch.

Reported-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 85db248ac1..229ea52516 100755
--- a/configure
+++ b/configure
@@ -5205,7 +5205,7 @@ if test "$skip_meson" = no; then
         -Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
         -Dvhost_user_blk_server=$vhost_user_blk_server -Dmultiprocess=$multiprocess \
         -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek -Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
-        $(if test "$default_features" = no; then echo "-Dauto_features=disabled"; fi) \
+        $(if test "$default_feature" = no; then echo "-Dauto_features=disabled"; fi) \
 	-Dtcg_interpreter=$tcg_interpreter \
         $cross_arg \
         "$PWD" "$source_path"
-- 
2.27.0

Re: [PATCH 1/4] configure: Fix --without-default-features propagation to meson

[Philippe Mathieu-Daudé]

+Alex

On 7/13/21 11:31 AM, Thomas Huth wrote:
> A typo prevents that many features get disabled when the user
> runs "configure" with the --without-default-features switch.
> 
> Reported-by: Cole Robinson <crobinso@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 85db248ac1..229ea52516 100755
> --- a/configure
> +++ b/configure
> @@ -5205,7 +5205,7 @@ if test "$skip_meson" = no; then
>          -Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
>          -Dvhost_user_blk_server=$vhost_user_blk_server -Dmultiprocess=$multiprocess \
>          -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek -Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
> -        $(if test "$default_features" = no; then echo "-Dauto_features=disabled"; fi) \
> +        $(if test "$default_feature" = no; then echo "-Dauto_features=disabled"; fi) \

The option should be name plural (default_features)...

What is 'auto_features' used for?

>  	-Dtcg_interpreter=$tcg_interpreter \
>          $cross_arg \
>          "$PWD" "$source_path"
> 

Re: [PATCH 1/4] configure: Fix --without-default-features propagation to meson

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> +Alex
>
> On 7/13/21 11:31 AM, Thomas Huth wrote:
>> A typo prevents that many features get disabled when the user
>> runs "configure" with the --without-default-features switch.
>> 
>> Reported-by: Cole Robinson <crobinso@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  configure | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/configure b/configure
>> index 85db248ac1..229ea52516 100755
>> --- a/configure
>> +++ b/configure
>> @@ -5205,7 +5205,7 @@ if test "$skip_meson" = no; then
>>          -Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
>>          -Dvhost_user_blk_server=$vhost_user_blk_server -Dmultiprocess=$multiprocess \
>>          -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek -Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
>> -        $(if test "$default_features" = no; then echo "-Dauto_features=disabled"; fi) \
>> +        $(if test "$default_feature" = no; then echo "-Dauto_features=disabled"; fi) \
>
> The option should be name plural (default_features)...

Actually, no.  The variable holds the initial value for the feature
variables that are to be controlled by --with-default-features.
Example:

    vde="$default_feature"

Perhaps $feature_default would be a better name.

> What is 'auto_features' used for?
>
>>  	-Dtcg_interpreter=$tcg_interpreter \
>>          $cross_arg \
>>          "$PWD" "$source_path"
>> 

Re: [PATCH 1/4] configure: Fix --without-default-features propagation to meson

[Thomas Huth]

On 13/07/2021 18.42, Philippe Mathieu-Daudé wrote:
> +Alex
> 
> On 7/13/21 11:31 AM, Thomas Huth wrote:
>> A typo prevents that many features get disabled when the user
>> runs "configure" with the --without-default-features switch.
>>
>> Reported-by: Cole Robinson <crobinso@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   configure | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index 85db248ac1..229ea52516 100755
>> --- a/configure
>> +++ b/configure
>> @@ -5205,7 +5205,7 @@ if test "$skip_meson" = no; then
>>           -Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
>>           -Dvhost_user_blk_server=$vhost_user_blk_server -Dmultiprocess=$multiprocess \
>>           -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek -Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
>> -        $(if test "$default_features" = no; then echo "-Dauto_features=disabled"; fi) \
>> +        $(if test "$default_feature" = no; then echo "-Dauto_features=disabled"; fi) \
> 
> The option should be name plural (default_features)...

I agree with Markus - the way it is used in the configure script, it's 
rather meant as singular.

> What is 'auto_features' used for?

https://mesonbuild.com/Build-options.html#features

  HTH,
   Thomas

Re: [PATCH 1/4] configure: Fix --without-default-features propagation to meson

[Philippe Mathieu-Daudé]

On 7/14/21 9:35 AM, Thomas Huth wrote:
> On 13/07/2021 18.42, Philippe Mathieu-Daudé wrote:
>> +Alex
>>
>> On 7/13/21 11:31 AM, Thomas Huth wrote:
>>> A typo prevents that many features get disabled when the user
>>> runs "configure" with the --without-default-features switch.
>>>
>>> Reported-by: Cole Robinson <crobinso@redhat.com>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   configure | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/configure b/configure
>>> index 85db248ac1..229ea52516 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -5205,7 +5205,7 @@ if test "$skip_meson" = no; then
>>>           -Ddocs=$docs -Dsphinx_build=$sphinx_build
>>> -Dinstall_blobs=$blobs \
>>>           -Dvhost_user_blk_server=$vhost_user_blk_server
>>> -Dmultiprocess=$multiprocess \
>>>           -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek
>>> -Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
>>> -        $(if test "$default_features" = no; then echo
>>> "-Dauto_features=disabled"; fi) \
>>> +        $(if test "$default_feature" = no; then echo
>>> "-Dauto_features=disabled"; fi) \
>>
>> The option should be name plural (default_features)...
> 
> I agree with Markus - the way it is used in the configure script, it's
> rather meant as singular.

OK.

>> What is 'auto_features' used for?
> 
> https://mesonbuild.com/Build-options.html#features

I see, thank you.

[PATCH 2/4] configure: Allow vnc to get disabled with --without-default-features

[Thomas Huth]

There's no reason why we should keep VNC enabled when the user
specified --without-default-features.

Reported-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure   | 2 +-
 meson.build | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 229ea52516..1974c46f6e 100755
--- a/configure
+++ b/configure
@@ -304,7 +304,7 @@ virtiofsd="auto"
 virtfs="auto"
 libudev="auto"
 mpath="auto"
-vnc="enabled"
+vnc="auto"
 sparse="auto"
 vde="$default_feature"
 vnc_sasl="auto"
diff --git a/meson.build b/meson.build
index 6cd2dc582a..5dedaf73e3 100644
--- a/meson.build
+++ b/meson.build
@@ -900,7 +900,7 @@ vnc = not_found
 png = not_found
 jpeg = not_found
 sasl = not_found
-if get_option('vnc').enabled()
+if not get_option('vnc').disabled()
   vnc = declare_dependency() # dummy dependency
   png = dependency('libpng', required: get_option('vnc_png'),
                    method: 'pkg-config', kwargs: static_kwargs)
-- 
2.27.0

[PATCH 3/4] configure: Fix the default setting of the "xen" feature

[Thomas Huth]

The "xen" variable should either contain "enabled", "disabled" or
nothing (for auto detection). But when the user currently runs the
configure script with --without-default-features, it gets set to
"no" instead. This does not work as expected, the feature will still
be enabled if the Xen headers are present. Thus set the variable
to "disabled" instead if default_feature switch has been set.

Reported-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 1974c46f6e..6c7336b763 100755
--- a/configure
+++ b/configure
@@ -311,7 +311,7 @@ vnc_sasl="auto"
 vnc_jpeg="auto"
 vnc_png="auto"
 xkbcommon="auto"
-xen="$default_feature"
+xen=${default_feature:+disabled}
 xen_ctrl_version="$default_feature"
 xen_pci_passthrough="auto"
 linux_aio="$default_feature"
-- 
2.27.0

[PATCH 4/4] configure: Let --without-default-features disable vhost-kernel and vhost-vdpa

[Thomas Huth]

The vhost_kernel and vhost_vdpa variables should be pre-initialized with
the $default_feature setting so that these features get disabled when
the user runs the configure scripts with --without-default-features.

Reported-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configure b/configure
index 6c7336b763..89cc9b24eb 100755
--- a/configure
+++ b/configure
@@ -321,6 +321,7 @@ attr="auto"
 xfs="$default_feature"
 tcg="enabled"
 membarrier="$default_feature"
+vhost_kernel="$default_feature"
 vhost_net="$default_feature"
 vhost_crypto="$default_feature"
 vhost_scsi="$default_feature"
@@ -328,6 +329,7 @@ vhost_vsock="$default_feature"
 vhost_user="no"
 vhost_user_blk_server="auto"
 vhost_user_fs="$default_feature"
+vhost_vdpa="$default_feature"
 bpf="auto"
 kvm="auto"
 hax="auto"
-- 
2.27.0

Re: [PATCH 0/4] Fixes for the --without-default-features configure switch

[Cole Robinson]

On 7/13/21 5:31 AM, Thomas Huth wrote:
> Many features do not get properly disabled when the user runs the
> configure script with --without-default-features. Let's fix that now.
> 
> Thomas Huth (4):
>   configure: Fix --without-default-features propagation to meson
>   configure: Allow vnc to get disabled with --without-default-features
>   configure: Fix the default setting of the "xen" feature
>   configure: Let --without-default-features disable vhost-kernel and
>     vhost-vdpa
> 

Patches look fine and fix some issues but others persist
(--disable-system isn't triggered). IMO this needs an audit, but more
importantly 'configure' should be rearranged a bit to make this less
likely to regress:

* move all the --enable/--disable variable init into one section with
nothing else mixed in

* convert the values to all use
$default_yes/no/auto/enabled/disabled/... variable syntax so visually
it's consistent, and if a default is ever changed like $default_no ->
$default_yes then we behave correctly (as opposed to 'no' -> 'yes').

Thanks,
Cole

Re: [PATCH 0/4] Fixes for the --without-default-features configure switch

[Paolo Bonzini]

On 13/07/21 16:54, Cole Robinson wrote:
> Patches look fine and fix some issues but others persist
> (--disable-system isn't triggered).

I wouldn't say --disable-system counts as a feature, since it's really a 
shortcut for choosing a subset of the targets.  Likewise for linux_user 
and bsd_user.

> IMO this needs an audit, but more
> importantly 'configure' should be rearranged a bit to make this less
> likely to regress:
> 
> * move all the --enable/--disable variable init into one section with
> nothing else mixed in
> 
> * convert the values to all use
> $default_yes/no/auto/enabled/disabled/... variable syntax so visually
> it's consistent, and if a default is ever changed like $default_no ->
> $default_yes then we behave correctly (as opposed to 'no' -> 'yes').

This is a nice idea.  We should only have default_yes/no/auto, plus 
"auto" for Meson options.

Also there's the idea of parsing --enable/--disable options for Meson 
options automatically from the introspection data.  This has the 
advantage that you get the default automatically from meson_options.txt 
and -Dauto_features, without any code in configure.

Paolo

Re: [PATCH 0/4] Fixes for the --without-default-features configure switch

[Paolo Bonzini]

On 13/07/21 11:31, Thomas Huth wrote:
> Many features do not get properly disabled when the user runs the
> configure script with --without-default-features. Let's fix that now.
> 
> Thomas Huth (4):
>    configure: Fix --without-default-features propagation to meson
>    configure: Allow vnc to get disabled with --without-default-features
>    configure: Fix the default setting of the "xen" feature
>    configure: Let --without-default-features disable vhost-kernel and
>      vhost-vdpa
> 
>   configure   | 8 +++++---
>   meson.build | 2 +-
>   2 files changed, 6 insertions(+), 4 deletions(-)
> 

Queued, thanks.

Paolo