[libvirt PATCH] rpm: Don't require qemu-img at build time
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 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
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
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Martin Kletzander -
Peter Krempa