[libvirt PATCH v2] spec: Merge -bash-completion and -admin into -client

Andrea Bolognani posted 1 patch 3 years ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210415125228.24417-1-abologna@redhat.com
libvirt.spec.in | 34 ++++++++++------------------------
1 file changed, 10 insertions(+), 24 deletions(-)
[libvirt PATCH v2] spec: Merge -bash-completion and -admin into -client
Posted by Andrea Bolognani 3 years ago
The former is ridiculously tiny and doesn't have any use on
its own, so it hardly warrants the overhead of an additional
package; the latter is also very small and, just like virsh,
is something that you likely want to have available on any
virtualization host to help with management and debugging
tasks.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
Changes from [v1]:

  * add Obsoletes/Provides for a smooth transition.

[v1] https://listman.redhat.com/archives/libvir-list/2021-April/msg00604.html

 libvirt.spec.in | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index f9af330186..a90f6abe38 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -901,7 +901,12 @@ Requires: %{name}-libs = %{version}-%{release}
 Requires: gettext
 # Needed by virt-pki-validate script.
 Requires: gnutls-utils
-Requires: %{name}-bash-completion = %{version}-%{release}
+# The -admin and -bash-completion packages have been merged into
+# the -client package: ensure this change is handled smoothly
+Obsoletes: libvirt-admin < 7.3.0
+Obsoletes: libvirt-bash-completion < 7.3.0
+Provides: libvirt-admin
+Provides: libvirt-bash-completion
 
 %description client
 The client binaries needed to access the virtualization
@@ -918,20 +923,6 @@ Requires: cyrus-sasl-gssapi
 %description libs
 Shared libraries for accessing the libvirt daemon.
 
-%package admin
-Summary: Set of tools to control libvirt daemon
-Requires: %{name}-libs = %{version}-%{release}
-Requires: %{name}-bash-completion = %{version}-%{release}
-
-%description admin
-The client side utilities to control the libvirt daemon.
-
-%package bash-completion
-Summary: Bash completion script
-
-%description bash-completion
-Bash completion script stub.
-
 %if %{with_wireshark}
 %package wireshark
 Summary: Wireshark dissector plugin for libvirt RPC transactions
@@ -1872,10 +1863,12 @@ exit 0
 
 %files client
 %{_mandir}/man1/virsh.1*
+%{_mandir}/man1/virt-admin.1*
 %{_mandir}/man1/virt-xml-validate.1*
 %{_mandir}/man1/virt-pki-validate.1*
 %{_mandir}/man1/virt-host-validate.1*
 %{_bindir}/virsh
+%{_bindir}/virt-admin
 %{_bindir}/virt-xml-validate
 %{_bindir}/virt-pki-validate
 %{_bindir}/virt-host-validate
@@ -1886,8 +1879,9 @@ exit 0
 %{_datadir}/systemtap/tapset/libvirt_qemu_probes*.stp
 %endif
 
+%{_datadir}/bash-completion/completions/vsh
 %{_datadir}/bash-completion/completions/virsh
-
+%{_datadir}/bash-completion/completions/virt-admin
 
 %{_unitdir}/libvirt-guests.service
 %config(noreplace) %{_sysconfdir}/sysconfig/libvirt-guests
@@ -1911,14 +1905,6 @@ exit 0
 
 %{_datadir}/libvirt/test-screenshot.png
 
-%files admin
-%{_mandir}/man1/virt-admin.1*
-%{_bindir}/virt-admin
-%{_datadir}/bash-completion/completions/virt-admin
-
-%files bash-completion
-%{_datadir}/bash-completion/completions/vsh
-
 %if %{with_wireshark}
 %files wireshark
 %{wireshark_plugindir}/libvirt.so
-- 
2.26.3

Re: [libvirt PATCH v2] spec: Merge -bash-completion and -admin into -client
Posted by Michal Privoznik 3 years ago
On 4/15/21 2:52 PM, Andrea Bolognani wrote:
> The former is ridiculously tiny and doesn't have any use on
> its own, so it hardly warrants the overhead of an additional
> package; 

The idea was, that if you'd install virt-admin only, you would have bash 
completion too, hence a separate package.

> the latter is also very small and, just like virsh,
> is something that you likely want to have available on any
> virtualization host to help with management and debugging
> tasks.

Exactly! Host, not client. Not every virt client is virt host too. Also, 
what's stopping management from installing libvirt-admin among with 
other RPMs? It's tiny, as you say. But I think I've expressed my concern 
enough.

> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> Changes from [v1]:
> 
>    * add Obsoletes/Provides for a smooth transition.
> 
> [v1] https://listman.redhat.com/archives/libvir-list/2021-April/msg00604.html
> 
>   libvirt.spec.in | 34 ++++++++++------------------------
>   1 file changed, 10 insertions(+), 24 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [libvirt PATCH v2] spec: Merge -bash-completion and -admin into -client
Posted by Daniel P. Berrangé 3 years ago
On Thu, Apr 15, 2021 at 02:52:28PM +0200, Andrea Bolognani wrote:
> The former is ridiculously tiny and doesn't have any use on
> its own, so it hardly warrants the overhead of an additional
> package; the latter is also very small and, just like virsh,
> is something that you likely want to have available on any
> virtualization host to help with management and debugging
> tasks.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> Changes from [v1]:
> 
>   * add Obsoletes/Provides for a smooth transition.
> 
> [v1] https://listman.redhat.com/archives/libvir-list/2021-April/msg00604.html
> 
>  libvirt.spec.in | 34 ++++++++++------------------------
>  1 file changed, 10 insertions(+), 24 deletions(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index f9af330186..a90f6abe38 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -901,7 +901,12 @@ Requires: %{name}-libs = %{version}-%{release}
>  Requires: gettext
>  # Needed by virt-pki-validate script.
>  Requires: gnutls-utils
> -Requires: %{name}-bash-completion = %{version}-%{release}
> +# The -admin and -bash-completion packages have been merged into
> +# the -client package: ensure this change is handled smoothly
> +Obsoletes: libvirt-admin < 7.3.0
> +Obsoletes: libvirt-bash-completion < 7.3.0
> +Provides: libvirt-admin
> +Provides: libvirt-bash-completion
>  
>  %description client
>  The client binaries needed to access the virtualization
> @@ -918,20 +923,6 @@ Requires: cyrus-sasl-gssapi
>  %description libs
>  Shared libraries for accessing the libvirt daemon.
>  
> -%package admin
> -Summary: Set of tools to control libvirt daemon
> -Requires: %{name}-libs = %{version}-%{release}
> -Requires: %{name}-bash-completion = %{version}-%{release}
> -
> -%description admin
> -The client side utilities to control the libvirt daemon.
> -
> -%package bash-completion
> -Summary: Bash completion script
> -
> -%description bash-completion
> -Bash completion script stub.
> -
>  %if %{with_wireshark}
>  %package wireshark
>  Summary: Wireshark dissector plugin for libvirt RPC transactions
> @@ -1872,10 +1863,12 @@ exit 0
>  
>  %files client
>  %{_mandir}/man1/virsh.1*
> +%{_mandir}/man1/virt-admin.1*
>  %{_mandir}/man1/virt-xml-validate.1*
>  %{_mandir}/man1/virt-pki-validate.1*
>  %{_mandir}/man1/virt-host-validate.1*
>  %{_bindir}/virsh
> +%{_bindir}/virt-admin

Per discussion on the previous version though, we better to have
installed any time the daemon is installed, and so if we do that
it means we also want the bash completion separate from the client.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH v2] spec: Merge -bash-completion and -admin into -client
Posted by Andrea Bolognani 3 years ago
On Mon, 2021-04-19 at 11:45 +0200, Michal Privoznik wrote:
> On 4/15/21 2:52 PM, Andrea Bolognani wrote:
> > the latter is also very small and, just like virsh,
> > is something that you likely want to have available on any
> > virtualization host to help with management and debugging
> > tasks.
> 
> Exactly! Host, not client. Not every virt client is virt host too. Also, 
> what's stopping management from installing libvirt-admin among with 
> other RPMs? It's tiny, as you say. But I think I've expressed my concern 
> enough.

Okay, let's not have virt-admin as part of -client then. Can we at
least have it in -daemon as opposed to its own package? And while
we're at it, we can move there also the other commands we agreed are
probably not a good fit for -client.

virt-xml-validate just performs validation on the input XML, so
having it in -client seems reasonable; on slighly closer inspection,
virt-pki-validate can validate not only the server-side PKI situation
but also the client-side one, so again -client is probably the most
sensible home for it.

virt-host-validate is intended for the hypervisor host, so we can
move it to -daemon; libvirt-guests should go there too, because it
should live where the daemon does.

What about the systemtap probes? Do they belong in -client, or maybe
they should go into -libs? I'm not familiar with their use.

On Mon, 2021-04-19 at 11:45 +0200, Michal Privoznik wrote:
> On 4/15/21 2:52 PM, Andrea Bolognani wrote:
> > The former is ridiculously tiny and doesn't have any use on
> > its own, so it hardly warrants the overhead of an additional
> > package; 
> 
> The idea was, that if you'd install virt-admin only, you would have bash 
> completion too, hence a separate package.

On Mon, 2021-04-19 at 10:57 +0100, Daniel P. Berrangé wrote:
> Per discussion on the previous version though, we better to have
> installed any time the daemon is installed and so if we do that
> it means we also want the bash completion separate from the client.

I would really like to get rid of the -bash-completion package.

  $ rpm -ql libvirt-bash-completion
  /usr/share/bash-completion/completions/vsh
  $ du -sh /usr/share/bash-completion/completions/vsh
  4.0K	/usr/share/bash-completion/completions/vsh
  $ du -sh .../libvirt-bash-completion-7.2.0-1.fc32.x86_64.rpm
  8.0K	.../libvirt-bash-completion-7.2.0-1.fc32.x86_64.rpm

The completion file itself uses as much disk space as the RPM
metadata for it. With that in mind, can we just install the file
twice instead of having a single file with two additional symlinks
pointing to it?

We could also turn it into a .in file so that instead of

  #
  # virsh & virt-admin completion command
  #

  _vsh_complete()
  {
      # ...
  } &&
  complete -o default -o filenames -F _vsh_complete virsh &&
  complete -o default -o filenames -F _vsh_complete virt-admin
  
the installed file for each command looks like
  
  #
  # @cmd@ completion command
  #

  _vsh_complete()
  {
      # ...
  } &&
  complete -o default -o filenames -F _vsh_complete @cmd@

Basically, if we can't avoid the overhead of duplication altogether,
I'd rather pay it on the filesystem than in the RPM database, where
it's felt over and over again when dowloading repodata and when
performing package management.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2] spec: Merge -bash-completion and -admin into -client
Posted by Daniel P. Berrangé 3 years ago
On Mon, Apr 19, 2021 at 05:42:01PM +0200, Andrea Bolognani wrote:
> On Mon, 2021-04-19 at 11:45 +0200, Michal Privoznik wrote:
> > On 4/15/21 2:52 PM, Andrea Bolognani wrote:
> > > the latter is also very small and, just like virsh,
> > > is something that you likely want to have available on any
> > > virtualization host to help with management and debugging
> > > tasks.
> > 
> > Exactly! Host, not client. Not every virt client is virt host too. Also, 
> > what's stopping management from installing libvirt-admin among with 
> > other RPMs? It's tiny, as you say. But I think I've expressed my concern 
> > enough.
> 
> Okay, let's not have virt-admin as part of -client then. Can we at
> least have it in -daemon as opposed to its own package? And while
> we're at it, we can move there also the other commands we agreed are
> probably not a good fit for -client.
> 
> virt-xml-validate just performs validation on the input XML, so
> having it in -client seems reasonable; on slighly closer inspection,
> virt-pki-validate can validate not only the server-side PKI situation
> but also the client-side one, so again -client is probably the most
> sensible home for it.

Yep, the RNGs are shipped with the library, so server can use tem
already.

> virt-host-validate is intended for the hypervisor host, so we can
> move it to -daemon; libvirt-guests should go there too, because it
> should live where the daemon does.

virt-host-validate clearly is daemon only. libvirt-guests  is a little
odd because in theory it doesn't depend on the daemon - it can work
with stateless drivers. IIRC thats why it is in the clients package.
I'm sceptical that anyone will ever even think about using the
libvirt-guests script to automate startup/shutdown of VMware/Hyperv/Etc
guests though.

> What about the systemtap probes? Do they belong in -client, or maybe
> they should go into -libs? I'm not familiar with their use.

Probes can be compiled into pretty much any binary artifact - daemon,
virsh, libvirt.so, driver modules.  -libs is the common place to all
of those.

> On Mon, 2021-04-19 at 11:45 +0200, Michal Privoznik wrote:
> > On 4/15/21 2:52 PM, Andrea Bolognani wrote:
> > > The former is ridiculously tiny and doesn't have any use on
> > > its own, so it hardly warrants the overhead of an additional
> > > package; 
> > 
> > The idea was, that if you'd install virt-admin only, you would have bash 
> > completion too, hence a separate package.
> 
> On Mon, 2021-04-19 at 10:57 +0100, Daniel P. Berrangé wrote:
> > Per discussion on the previous version though, we better to have
> > installed any time the daemon is installed and so if we do that
> > it means we also want the bash completion separate from the client.
> 
> I would really like to get rid of the -bash-completion package.
> 
>   $ rpm -ql libvirt-bash-completion
>   /usr/share/bash-completion/completions/vsh
>   $ du -sh /usr/share/bash-completion/completions/vsh
>   4.0K	/usr/share/bash-completion/completions/vsh
>   $ du -sh .../libvirt-bash-completion-7.2.0-1.fc32.x86_64.rpm
>   8.0K	.../libvirt-bash-completion-7.2.0-1.fc32.x86_64.rpm
> 
> The completion file itself uses as much disk space as the RPM
> metadata for it. With that in mind, can we just install the file
> twice instead of having a single file with two additional symlinks
> pointing to it?
> 
> We could also turn it into a .in file so that instead of
> 
>   #
>   # virsh & virt-admin completion command
>   #
> 
>   _vsh_complete()
>   {
>       # ...
>   } &&
>   complete -o default -o filenames -F _vsh_complete virsh &&
>   complete -o default -o filenames -F _vsh_complete virt-admin
>   
> the installed file for each command looks like
>   
>   #
>   # @cmd@ completion command
>   #
> 
>   _vsh_complete()
>   {
>       # ...
>   } &&
>   complete -o default -o filenames -F _vsh_complete @cmd@
> 
> Basically, if we can't avoid the overhead of duplication altogether,
> I'd rather pay it on the filesystem than in the RPM database, where
> it's felt over and over again when dowloading repodata and when
> performing package management.

I think this worrying about micro-optimizing a problem that doesn't
really have any real world impact. 8k is lost in the noise of an
RPM transaction.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH v2] spec: Merge -bash-completion and -admin into -client
Posted by Andrea Bolognani 3 years ago
On Mon, 2021-04-19 at 16:51 +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 19, 2021 at 05:42:01PM +0200, Andrea Bolognani wrote:
> > Basically, if we can't avoid the overhead of duplication altogether,
> > I'd rather pay it on the filesystem than in the RPM database, where
> > it's felt over and over again when dowloading repodata and when
> > performing package management.
> 
> I think this worrying about micro-optimizing a problem that doesn't
> really have any real world impact. 8k is lost in the noise of an
> RPM transaction.

I will use the same argument with s/an RPM transaction/a Linux
install/ ;)

dnf is slow enough as it is, and anything we can do to help it out is
a good change in my book. The impact of a single package might not be
measurable, but hopefully other packagers are also paying some
attention to the topic and avoiding creating unnecessary binary
packages.

Anyway, I'll try to cook up a patch for this that is completely
stand-alone with respect to the other changes, so once it's on the
list we can discuss whether to merge it or throw it away :)

-- 
Andrea Bolognani / Red Hat / Virtualization