It's not used as part of the build process or even searched for
at build time. The QEMU driver detects its path at runtime.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 libvirt.spec.in | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index c542ec3b2b..69cafc8b91 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -310,8 +310,6 @@ BuildRequires: util-linux
 %if %{with_qemu}
 # For managing ACLs
 BuildRequires: libacl-devel
-# From QEMU RPMs
-BuildRequires: /usr/bin/qemu-img
 %endif
 # For LVM drivers
 BuildRequires: lvm2
-- 
2.40.1

On Fri, May 05, 2023 at 08:05:05PM +0200, Andrea Bolognani wrote:
>It's not used as part of the build process or even searched for
>at build time. The QEMU driver detects its path at runtime.
>

But we do run tests at build time and virstoragetest.c is looking for
qemu-img and using it.  Without it the tests would not fail, but would
be skipped, which might be ever worse because we would not notice.

>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
> libvirt.spec.in | 2 --
> 1 file changed, 2 deletions(-)
>
>diff --git a/libvirt.spec.in b/libvirt.spec.in
>index c542ec3b2b..69cafc8b91 100644
>--- a/libvirt.spec.in
>+++ b/libvirt.spec.in
>@@ -310,8 +310,6 @@ BuildRequires: util-linux
> %if %{with_qemu}
> # For managing ACLs
> BuildRequires: libacl-devel
>-# From QEMU RPMs
>-BuildRequires: /usr/bin/qemu-img
> %endif
> # For LVM drivers
> BuildRequires: lvm2
>-- 
>2.40.1
>

On Wed, May 10, 2023 at 12:01:03PM +0200, Martin Kletzander wrote:
> On Fri, May 05, 2023 at 08:05:05PM +0200, Andrea Bolognani wrote:
> > It's not used as part of the build process or even searched for
> > at build time. The QEMU driver detects its path at runtime.
>
> But we do run tests at build time and virstoragetest.c is looking for
> qemu-img and using it.  Without it the tests would not fail, but would
> be skipped, which might be ever worse because we would not notice.

Good catch! You're right, we definitely don't want that.

I see that we only have 3 calls to 'qemu-img create' in that test,
with everything else being done via preformatted images that are
stored in tests/virstoragetestdata/. Several more calls were present
in the past, but the vast majority were dropped with [1].

Peter, is there a reason why we're still calling 'qemu-img create'
for those specific images? Or could we replace those calls with
preformatted images as well, and no longer use qemu-img from the test
suite at all?


[1] https://listman.redhat.com/archives/libvir-list/2021-September/222689.html
-- 
Andrea Bolognani / Red Hat / Virtualization

On Wed, May 10, 2023 at 05:28:56 -0700, Andrea Bolognani wrote:
> On Wed, May 10, 2023 at 12:01:03PM +0200, Martin Kletzander wrote:
> > On Fri, May 05, 2023 at 08:05:05PM +0200, Andrea Bolognani wrote:
> > > It's not used as part of the build process or even searched for
> > > at build time. The QEMU driver detects its path at runtime.
> >
> > But we do run tests at build time and virstoragetest.c is looking for
> > qemu-img and using it.  Without it the tests would not fail, but would
> > be skipped, which might be ever worse because we would not notice.
> 
> Good catch! You're right, we definitely don't want that.
> 
> I see that we only have 3 calls to 'qemu-img create' in that test,
> with everything else being done via preformatted images that are
> stored in tests/virstoragetestdata/. Several more calls were present
> in the past, but the vast majority were dropped with [1].
> 
> Peter, is there a reason why we're still calling 'qemu-img create'
> for those specific images? Or could we replace those calls with
> preformatted images as well, and no longer use qemu-img from the test
> suite at all?

The idea why I kept using freshly formatted images is to validate our
qcow2 header parser (we have our own, which is intentionally kept very
simple to only parse the format and backing image location) against the
current state of qemu, to catch potential regressions when qemu's format
would change. It's a form of integration testing albeit a bit weird in
how it's executed (notably it depeds on the system version of qemu-img
rather than doing anything specific to test against current upstream).

Arguably that would not be good for qemu as well when they'd change the
format so incompatibly it so I suppose it's already tested in qemu.

I don't think it would be too much of a loss for downstreams if those
tests were skipped. Replacing it by preformatted images makes no sense
as there are already other tests doing the same, thus only reasonable
other course of actions is to drop the tests entirely.

On the other hand, I don't really think that dropping the build
dependency would simplify or optimize anything.

On Wed, May 10, 2023 at 02:40:38PM +0200, Peter Krempa wrote:
> On Wed, May 10, 2023 at 05:28:56 -0700, Andrea Bolognani wrote:
> > Peter, is there a reason why we're still calling 'qemu-img create'
> > for those specific images? Or could we replace those calls with
> > preformatted images as well, and no longer use qemu-img from the test
> > suite at all?
>
> The idea why I kept using freshly formatted images is to validate our
> qcow2 header parser (we have our own, which is intentionally kept very
> simple to only parse the format and backing image location) against the
> current state of qemu, to catch potential regressions when qemu's format
> would change. It's a form of integration testing albeit a bit weird in
> how it's executed (notably it depeds on the system version of qemu-img
> rather than doing anything specific to test against current upstream).

Alright, so it makes sense to keep running qemu-img as part of the
test suite and thus to have it as a build dependency. I'm just going
to update the comment in the spec file and point out this usage
explicitly, because it's clearly non-obvious right now.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Wed, May 10, 2023 at 06:01:28AM -0700, Andrea Bolognani wrote:
>On Wed, May 10, 2023 at 02:40:38PM +0200, Peter Krempa wrote:
>> On Wed, May 10, 2023 at 05:28:56 -0700, Andrea Bolognani wrote:
>> > Peter, is there a reason why we're still calling 'qemu-img create'
>> > for those specific images? Or could we replace those calls with
>> > preformatted images as well, and no longer use qemu-img from the test
>> > suite at all?
>>
>> The idea why I kept using freshly formatted images is to validate our
>> qcow2 header parser (we have our own, which is intentionally kept very
>> simple to only parse the format and backing image location) against the
>> current state of qemu, to catch potential regressions when qemu's format
>> would change. It's a form of integration testing albeit a bit weird in
>> how it's executed (notably it depeds on the system version of qemu-img
>> rather than doing anything specific to test against current upstream).
>
>Alright, so it makes sense to keep running qemu-img as part of the
>test suite and thus to have it as a build dependency. I'm just going
>to update the comment in the spec file and point out this usage
>explicitly, because it's clearly non-obvious right now.
>

Well, technically it is not "required" an since some of us will run
newer qemu-img than the machines in CI we're testing on, not having it
does not break anything and could just be tested occasionally by devs.
But whatever you and Peter decide is fine with me thanks to the
explanations.

>--
>Andrea Bolognani / Red Hat / Virtualization
>

On a Friday in 2023, Andrea Bolognani wrote:
>It's not used as part of the build process or even searched for
>at build time. The QEMU driver detects its path at runtime.
>
>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
> libvirt.spec.in | 2 --
> 1 file changed, 2 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano