On Thu, Nov 12, 2020 at 10:40:02PM +0100, Olaf Hering wrote: > Since meson does not support environment variables it seems the > only way to address this is to introduce an option in > meson_options.txt for each runtime executable. > > Will such change be accepted? This would be one way how to address runtime executables, currently we have a lot of them. These executables are checked in order to enable/disable some libvirt feature but is used only in runtime: parted, bhyve, bhyvectl, bhyveload, mount, umount, mkfs, pvcreate, vgcreate, lvcreate, pvremove, vgremove, lvremove, lvchange, vgchange, vgscan, pvs, vgs, lvs, (collie|dog), vstorage, vstorage-mount, zfs, zpool, numad This one is even more broken as if we don't find it during build time it is set to empty string and never checked in our code: showmount These are checked during build time but if not present they are set to known absolute path: qemu-bridge-helper, qemu-pr-helper, slirp-helper, dbus-daemon And we have a list of optional_programs that are checked during build time and if not present they are set to the name of the executable and resolved during runtime from $PATH. The last executable, pkcheck, is not used during build and in runtime. We only check its presence to enable/disable polkit support. We should be able to simply drop this check and figure out the presence of polkit using DBus only as we do for machined or logind and other DBus services that we use. All of this was copied from autoconf except for the fact that it is no longer possible to use ENV variables. I think we need to unify the process how we handle runtime executable dependencies and possibly add meson options for them.

Libvirt needs to be able to handle that missing binary at runtime anyway

On Thu, Nov 19, 2020 at 10:31:00PM +0100, Olaf Hering wrote: > Am Thu, 19 Nov 2020 22:15:27 +0100 > schrieb Martin Kletzander <mkletzan(a)redhat.com&gt;: > > > Libvirt needs to be able to handle that missing binary at runtime anyway > > It also needs to handle an existing binary. Just how is it supposed to handle it? > Um... run it? > Right now with meson it is either a full path, or just the name in case it was missing at build time (I have not checked if the launcher actually consults $PATH). > Right now, IMHO, all meson checks for binaries that are not needed at build time should be removed. During runtime we can just use the name of the binary. I don't know whether it used to be the case that it was thought that there might be security issues with supplying different binary in a directory in $PATH, but frankly, if you have (different-)user-writable directory in $PATH or non-root access to modifying system-wide $PATH then you have bigger problems to deal with. Even though I do not have anything to back this claim I think that might've been the original reason.

On Fri, 2020-11-20 at 01:19 +0100, Pavel Hrdina wrote: > On Thu, Nov 19, 2020 at 11:38:28PM +0100, Martin Kletzander wrote: > > Right now, IMHO, all meson checks for binaries that are not needed at build time > > should be removed. During runtime we can just use the name of the binary. I > > don't know whether it used to be the case that it was thought that there might > > be security issues with supplying different binary in a directory in $PATH, but > > frankly, if you have (different-)user-writable directory in $PATH or non-root > > access to modifying system-wide $PATH then you have bigger problems to deal > > with. Even though I do not have anything to back this claim I think that > > might've been the original reason. > > That was my take on the original reasoning as well. I completely agree > here with Martin and vote for removing these runtime binary checks from > meson completely. There would be also the benefit for testing purposes > that you can simply change the path to use your own compiled binary > without changing anything in libvirt. While I would love the simplification such an approach would yield, I have to point out that there is at least one advantage to checking for the availability of commands at build time, even if those commands will only ever be invoked at runtime: it makes it obvious those commands are going to be needed later on. Take the zfs example again: if there was no build time checking, one might very reasonably assume that the zfs storage driver is usable on its own (the same way the iscsi-direct driver, for example, is); the fact that this is not the case would only become apparent much later, as you attempt to perform some storage operation and get back a failure.

So while libvirt obviously needs to be able to cope gracefully with the commands that were detected at build time not being present at runtime (which would indicate a packaging bug), I'm not convinced that dropping the build time checks is the best solution. Making them overridable, as they were with autotools, sounds like a more solid approach; and for those commands where we don't already check at build time, such a check should probably be introduced.

-- Andrea Bolognani / Red Hat / Virtualization

On Fri, 2020-11-20 at 13:58 +0100, Martin Kletzander wrote: > On Fri, Nov 20, 2020 at 12:24:38PM +0100, Andrea Bolognani wrote: > > While I would love the simplification such an approach would yield, > > I have to point out that there is at least one advantage to checking > > for the availability of commands at build time, even if those > > commands will only ever be invoked at runtime: it makes it obvious > > those commands are going to be needed later on.

Lastly, if we stopped looking for commands at build time we would be enabling features that can, in reality, never work: iscsiadm, for example, is Linux-only, but if we removed the corresponding build time check we would start building the iscsi storage driver, which requires the command to be present, on FreeBSD and macOS too. This would be incredibly misleading and, well, just plain wrong IMHO.

On Fri, 2020-11-20 at 14:36 +0000, Daniel P. Berrangé wrote: > On Fri, Nov 20, 2020 at 03:31:39PM +0100, Andrea Bolognani wrote: > > Lastly, if we stopped looking for commands at build time we would be > > enabling features that can, in reality, never work: iscsiadm, for > > example, is Linux-only, but if we removed the corresponding build > > time check we would start building the iscsi storage driver, which > > requires the command to be present, on FreeBSD and macOS too. This > > would be incredibly misleading and, well, just plain wrong IMHO. > > If you remove the iscsiadm check, it has to be replaced with > an os == linux check in meson instead. If you did that, then you wouldn't have simplified meson.build much while at the same time you'd have ended up with a far less nuanced check that does not catch scenarios ranging from the command simply not being installed on the build machine to the package not being available, perhaps temporarily, for the specific Linux architecture in use. Doesn't really sound like an improvement to me.

On Fri, 2020-11-20 at 17:31 +0100, Pavel Hrdina wrote: > On Fri, Nov 20, 2020 at 04:38:20PM +0100, Andrea Bolognani wrote: > > If you did that, then you wouldn't have simplified meson.build much > > while at the same time you'd have ended up with a far less nuanced > > check that does not catch scenarios ranging from the command simply > > not being installed on the build machine to the package not being > > available, perhaps temporarily, for the specific Linux architecture > > in use. Doesn't really sound like an improvement to me. > > I personally don't see any issue with this. The binary is simply runtime > dependency, there is no need to have it present when building libvirt. > > If someone is building any project from source code manually you can > consider these users advanced enough to figure all of this out or they > are following some tutorial where they most likely installed all > required build and runtime dependencies mentioned in the tutorial. > > As Martin mentioned this would mostly affect only maintainers of > downstream libvirt packages where they have to have some understanding > of the project and it's dependencies in order to create the package > properly. I feel like "some understanding" is an understatement considering the sheer size of libvirt: it's entirely possible to have a pretty good understanding of it and still get some of the details wrong. In my opinion this move would place a significant burden upon downstreams and users while taking very little burden away from upstream developers because, regardless of whether or not build time checks are performed, the task of maintaining the list of runtime dependencies in *some* form still falls upon upstream: you would just have changed it from updating three files to updating two.

On Fri, Nov 20, 2020 at 06:49:15PM +0100, Andrea Bolognani wrote: > In my opinion this move would place a significant burden upon > downstreams and users while taking very little burden away from > upstream developers because, regardless of whether or not build time > checks are performed, the task of maintaining the list of runtime > dependencies in *some* form still falls upon upstream: you would just > have changed it from updating three files to updating two. I would say that nobody is arguing about the responsibility of providing list of runtime dependencies. In fact IMHO the ideal solution would be to have a complete list of all our dependencies with links to the respective upstream projects to make it clear.

For downstream maintainers having a list of dependencies is IMO way better then running the build system several times to figure out what else they need to add to the package dependencies in order to successfully build the package. Obviously this applies to non-RPM based distributions.

On Fri, 2020-11-13 at 12:37 +0100, Pavel Hrdina wrote: > On Thu, Nov 12, 2020 at 10:40:02PM +0100, Olaf Hering wrote: > > Since meson does not support environment variables it seems the > > only way to address this is to introduce an option in > > meson_options.txt for each runtime executable. > > > > Will such change be accepted? > > This would be one way how to address runtime executables, currently we > have a lot of them. These executables are checked in order to > enable/disable some libvirt feature but is used only in runtime: > > parted, bhyve, bhyvectl, bhyveload, mount, umount, mkfs, pvcreate, > vgcreate, lvcreate, pvremove, vgremove, lvremove, lvchange, vgchange, > vgscan, pvs, vgs, lvs, (collie|dog), vstorage, vstorage-mount, zfs, > zpool, numad > > This one is even more broken as if we don't find it during build time it > is set to empty string and never checked in our code: > > showmount > > These are checked during build time but if not present they are set to > known absolute path: > > qemu-bridge-helper, qemu-pr-helper, slirp-helper, dbus-daemon > > And we have a list of optional_programs that are checked during build > time and if not present they are set to the name of the executable and > resolved during runtime from $PATH. > > The last executable, pkcheck, is not used during build and in runtime. > We only check its presence to enable/disable polkit support. We should > be able to simply drop this check and figure out the presence of polkit > using DBus only as we do for machined or logind and other DBus services > that we use. > > All of this was copied from autoconf except for the fact that it is no > longer possible to use ENV variables. > > I think we need to unify the process how we handle runtime executable > dependencies and possibly add meson options for them. As another data point, Debian currently carries a patch[1] which allows us to enable the ZFS driver without installing the ZFS packages in the build environment: this is necessary because, due to its license, ZFS is kept outside of the main Debian repository. Being able to use something like -Dprog_zfs=/sbin/zfs -Dprog_zfspool=/sbin/zfspool to achieve the same result would allow us to drop that patch, which I would be *extremely* happy about :) [1] https://salsa.debian.org/libvirt-team/libvirt/-/blob/debian/master/debian...

On Thu, 2020-11-19 at 16:56 -0500, Neal Gompa wrote: > On Fri, Nov 13, 2020 at 8:14 AM Andrea Bolognani <abologna(a)redhat.com&gt; wrote: > > As another data point, Debian currently carries a patch[1] which > > allows us to enable the ZFS driver without installing the ZFS > > packages in the build environment: this is necessary because, due to > > its license, ZFS is kept outside of the main Debian repository. > > > > Being able to use something like > > > > -Dprog_zfs=/sbin/zfs -Dprog_zfspool=/sbin/zfspool > > > > to achieve the same result would allow us to drop that patch, which I > > would be *extremely* happy about :) > > The way I solved *that* specific problem was to use zfs-fuse at > build-time and have runtime work with either implementation. That > should work for you too, since Debian has zfs-fuse in main. This sounds intriguing. Can you please point me to the code?

On Fri, 2020-11-20 at 08:51 -0500, Neal Gompa wrote: > On Fri, Nov 20, 2020 at 6:25 AM Andrea Bolognani <abologna(a)redhat.com&gt; wrote: > > On Thu, 2020-11-19 at 16:56 -0500, Neal Gompa wrote: > > > The way I solved *that* specific problem was to use zfs-fuse at > > > build-time and have runtime work with either implementation. That > > > should work for you too, since Debian has zfs-fuse in main. > > > > This sounds intriguing. Can you please point me to the code? > > I build libvirt for Debian/Ubuntu using a modified version of the > upstream spec file (if y'all are interested, I can upstream those > modifications so we could build and test that way in CI too), but > here's what I did: > > * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596... > * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596... > * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596... > > For work, we *only* want to use ZoL at runtime, but this can be done > where zfs-fuse could be a valid candidate too. > > It's not terribly difficult to translate this into debian/control goop. :) I just looked at your spec file and that's certainly unique! Out of curiosity, are you shipping the resulting Debian packages anywhere?