regression in meson build, AC_PATH_PROG lost
autoconf allows to specify the path to a runtime binary that is not required during build via an environment variable: AC_PATH_PROG([PARTED], [parted], [], [$LIBVIRT_SBIN_PATH]) meson lacks such essential feature. As a result the package build environment needs to have more packages than necessary installed. This excessive list also pushes libvirt closer to the end of the build dependency chain. Was this considered while the buildsystem was switched to meson? 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? Olaf
On Thu, Nov 12, 2020 at 10:40:02PM +0100, Olaf Hering wrote:
autoconf allows to specify the path to a runtime binary that is not required during build via an environment variable: AC_PATH_PROG([PARTED], [parted], [], [$LIBVIRT_SBIN_PATH])
meson lacks such essential feature. As a result the package build environment needs to have more packages than necessary installed. This excessive list also pushes libvirt closer to the end of the build dependency chain.
Was this considered while the buildsystem was switched to meson?
Hi, I did not considered this specific use-case where the runtime executable dependency is not available during build time. Mainly because we have a lot of other executables that are runtime dependencies but are check by meson to figure out if some libvirt feature should be enabled or disabled.
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. Pavel
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/pa... -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Nov 13, 2020 at 02:13:38PM +0100, Andrea Bolognani wrote:
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 :)
It sounds like most of the things are just caused by the binary being required at runtime while being checked during build time. Libvirt needs to be able to handle that missing binary at runtime anyway and we should just not require it at build time (or, if worse comes to worst, emit a non-fatal message about it missing) and that's it. When, and only when, someone needs to change the path to the executable we might have a discussion about dealing with that. Ideally we'd just get it fixed. We started with _some_ build-time requirements like that, but as far as I know nobody continued the work.
[1] https://salsa.debian.org/libvirt-team/libvirt/-/blob/debian/master/debian/pa... -- Andrea Bolognani / Red Hat / Virtualization
Am Thu, 19 Nov 2020 22:15:27 +0100 schrieb Martin Kletzander <mkletzan@redhat.com>:
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? 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). Anyway, what would be the desired approach to tell libvirt the runtime path? I was thinking about something like: option('runtime_path_exe', type: 'string', value: 'exe', description: 'runtime path of exe') runtime_path_exe = get_option('runtime_path_exe') if not get_option('runtime_path_exe').enabled() runtime_path_exe = find_program(runtime_path_exe, required: true).path() conf.set_quoted('EXE', runtime_path_exe) Olaf
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@redhat.com>:
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.
Anyway, what would be the desired approach to tell libvirt the runtime path?
Find it at runtime with virFindFileInPath() just like we already do with a few binaries (and did for some since 2009 or so) or even without it as virCommand() already searches $PATH since: commit e0d014f2379ddde175c0c3126273911221c3e645 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Tue Mar 15 16:58:28 2011 +0000 Ensure binary is resolved wrt $PATH in virExec
I was thinking about something like: option('runtime_path_exe', type: 'string', value: 'exe', description: 'runtime path of exe') runtime_path_exe = get_option('runtime_path_exe') if not get_option('runtime_path_exe').enabled() runtime_path_exe = find_program(runtime_path_exe, required: true).path() conf.set_quoted('EXE', runtime_path_exe)
Olaf
Just for the sake of completeness here are some examples on how to do it: commit 274f09cbc5ed8be00127380327d9525b852a2d1d Author: Stefan Berger <stefanb@us.ibm.com> Date: Wed Apr 14 06:29:55 2010 -0400 nwfilter: use virFindFileInPath for needed CLI tools commit 2e045a4f9bf2757199c0997b9842d8dd8510459f Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Jan 19 10:27:11 2017 +0000 storage: avoid use of undefined GLUSTER_CLI variable Martin P.S.: It is not *strictly* necessary that the commit hash starts with '2' ;)
On Thu, Nov 19, 2020 at 11:38:28PM +0100, Martin Kletzander wrote:
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@redhat.com>:
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.
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. I guess we have to have some exceptions like for the QEMU helpers that usually lives in /usr/libexec which is not intentionally part of $PATH. Pavel
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, Nov 20, 2020 at 12:24:38PM +0100, Andrea Bolognani wrote:
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.
This should be handled by a package maintainer who would put such requirement into the package for libvirt-deamon-driver-storage-zfs (or similar). It would just be in Requires instead of BuildRequires. Or DEPEND instead of RDEPEND. And based on what you say that's actaully what debian does, just with a hackish workaround. I understand that if we add new functionality that depends on a new binary and it only fails at runtime then it might happen that the maintainer misses it and people will run into an issue. It's most probably not going to be a regression, though. And we need to document it somewhere anyway. And my understanding is that the maintainer should at least roughly look at the changes. Maybe if we had like a purpose built file in the repository that would document the building process and requirements so that it was easier to filter the changes between releases to figure out if the build needs changing. Oh, wait, we have libvirt.spec.in ;) Honestly, I would not be against having the same file in the repo for every distro that ships libvirt, but that's another discussion. But anyway, just checking for changes in the spec file is enough.
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.
I would just like to know if there is anyone who needs them overridable for any other reason than to disable the build-time check. Because if no, then we're just talking about adding pointless complexity.
-- 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.
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.
This should be handled by a package maintainer who would put such requirement into the package for libvirt-deamon-driver-storage-zfs (or similar). It would just be in Requires instead of BuildRequires. Or DEPEND instead of RDEPEND. And based on what you say that's actaully what debian does, just with a hackish workaround.
As I explained earlier, in the case of zfs the workaround is needed for licensing reasons. In retrospect, zfs is probably not the best example, so let's use iscsiadm instead: in this case too, without this command the corresponding driver can't work properly, but such dependency is not as obvious as the one between the iscsi-direct driver and libiscsi.
I understand that if we add new functionality that depends on a new binary and it only fails at runtime then it might happen that the maintainer misses it and people will run into an issue. It's most probably not going to be a regression, though. And we need to document it somewhere anyway. And my understanding is that the maintainer should at least roughly look at the changes. Maybe if we had like a purpose built file in the repository that would document the building process and requirements so that it was easier to filter the changes between releases to figure out if the build needs changing. Oh, wait, we have libvirt.spec.in ;) Honestly, I would not be against having the same file in the repo for every distro that ships libvirt, but that's another discussion. But anyway, just checking for changes in the spec file is enough.
The spec file is of course the first thing I look at when I'm wearing my Debian maintainer hat and I'm in the process of importing a new version of libvirt :) And I skim through the overall changes too, but the diff between subsequent libvirt releases is usually so massive that it's simply not realistic to expect that no detail will ever be missed in the process. Anyway, the above seems to assume that the spec file is a perfect representation of requirements at all times, which is not necessarily the case; and our CI, of course, only *builds* the RPMs, so if all the checks happen at runtime it has no chance of catching mistakes... Moreover, there are people who are building libvirt from source, and for those getting a build time warning/error is definitely a better experience than having to sift through the spec file to figure out for themselves all the runtime dependencies that are not tracked by the build system. 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.
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.
I would just like to know if there is anyone who needs them overridable for any other reason than to disable the build-time check. Because if no, then we're just talking about adding pointless complexity.
As far as the Debian packaging is concerned, having the ability to override the command for zfs-related tools would be enough. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Nov 20, 2020 at 03:31:39PM +0100, Andrea Bolognani wrote:
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.
If you remove the iscsiadm check, it has to be replaced with an os == linux check in meson instead. 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 :|
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. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Nov 20, 2020 at 04:38:20PM +0100, Andrea Bolognani wrote:
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.
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. Pavel
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. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Nov 20, 2020 at 06:49:15PM +0100, Andrea Bolognani wrote:
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.
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. Having checks for runtime dependencies in our build system and use these to figure out if the feature should be enabled or disabled is wrong and should be removed completely and where needed replaced with different checks for targeted OS, architecture, etc. It's not only about the burden. The feature can be compiled without the presence of the binary and the binary can be provided later. The same way it can be present during the compilation but missing when libvirt is actually running. 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. Like Martin already wrote, the current situation is already broken as most of the runtime dependencies are optional and the build will not fail if they are missing, so there is already a requirement for that knowledge. Only if few cases the runtime dependency is used to enable/disable some libvirt feature. And there are other runtime dependencies that are not even mentioned in our build system. Like I wrote, end-users compiling libvirt directly are usually following some tutorial where the list of dependencies is already present and even with the build time check they can still shoot themselves by removing the runtime dependency once libvirt is compiled and/or installed so there is no guarantee that it will solve all the scenarios for end users. The possibility to provide explicit absolute path to the runtime dependencies is related but completely different issue. Again IMHO I don't think that it's necessary to have these options. Pavel
On Fri, Nov 20, 2020 at 07:32:44PM +0100, Pavel Hrdina wrote:
On Fri, Nov 20, 2020 at 06:49:15PM +0100, Andrea Bolognani wrote:
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.
So let's assume the state of downstream packages is OK now and we are only talking about added functionality from now on (technically it would also involve new packaging systems and new distributions, but that's a corner case). When we start using new binary than in your case we have to add a new dependency in meson.build and libvirt.spec.in. If we go with my approach it consists of updating only one file instead. Saying that updating something could be error-prone is therefore a moo point because we are relying on that already and the more so with the approach you are suggesting.
In my opinion this move would place a significant burden upon downstreams and users while taking very little burden away from
Not really. As Olaf pointed out they need to rewrite more than just one check because of licence implications. And there might be more. Also build-checking libvirt requires way more packages to be installed even when you are just building it. If we ever forget to add the requirement into the specfile (or any other package build descriptor file) then what will be the outcome? User will try to do something and they get an error that binary "blah" is missing. They will install "blah" and move on. Ideally they will report that to their distro maintainer and they will be able to fix it right away. And since we are talking about future features only that will most probably not happen on some automated deployment because any such setup needs to be tested first. I really do not see more problems with the approach I'm suggesting.
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.
Having checks for runtime dependencies in our build system and use these to figure out if the feature should be enabled or disabled is wrong and should be removed completely and where needed replaced with different checks for targeted OS, architecture, etc. It's not only about the burden. The feature can be compiled without the presence of the binary and the binary can be provided later. The same way it can be present during the compilation but missing when libvirt is actually running.
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.
Like Martin already wrote, the current situation is already broken as most of the runtime dependencies are optional and the build will not fail if they are missing, so there is already a requirement for that knowledge. Only if few cases the runtime dependency is used to enable/disable some libvirt feature. And there are other runtime dependencies that are not even mentioned in our build system.
Like I wrote, end-users compiling libvirt directly are usually following some tutorial where the list of dependencies is already present and even with the build time check they can still shoot themselves by removing the runtime dependency once libvirt is compiled and/or installed so there is no guarantee that it will solve all the scenarios for end users.
The possibility to provide explicit absolute path to the runtime dependencies is related but completely different issue. Again IMHO I don't think that it's necessary to have these options.
Pavel
On Fri, 2020-11-20 at 19:32 +0100, Pavel Hrdina wrote:
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.
If we take this opportunity to take stock of all the existing, non-library runtime dependencies and document them accurately in a document that's also going to be kept up-to-date going forward, then most of my concerns are mitigated. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Nov 20, 2020 at 01:19:33AM +0100, Pavel Hrdina wrote:
On Thu, Nov 19, 2020 at 11:38:28PM +0100, Martin Kletzander wrote:
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@redhat.com>:
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.
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.
I guess we have to have some exceptions like for the QEMU helpers that usually lives in /usr/libexec which is not intentionally part of $PATH.
Some of them look like they are handled already, but of course we need to make sure all of them work. Maybe just prepending/appending /usr/libexec/ to the list of directories in virFileFindInPath() or a similar method would do, I don't know. I'm glad we're on the same canoe =)
Pavel
On Fri, Nov 13, 2020 at 8:14 AM Andrea Bolognani <abologna@redhat.com> wrote:
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/pa...
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. -- 真実はいつも一つ！/ Always, there's only one truth!
On Thu, 2020-11-19 at 16:56 -0500, Neal Gompa wrote:
On Fri, Nov 13, 2020 at 8:14 AM Andrea Bolognani <abologna@redhat.com> 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? -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Nov 20, 2020 at 6:25 AM Andrea Bolognani <abologna@redhat.com> wrote:
On Thu, 2020-11-19 at 16:56 -0500, Neal Gompa wrote:
On Fri, Nov 13, 2020 at 8:14 AM Andrea Bolognani <abologna@redhat.com> 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?
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/90d9373670564341503f768880e996f01da596a6/... * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/... * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/... 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. :) -- 真実はいつも一つ！/ Always, there's only one truth!
On Fri, 2020-11-20 at 08:51 -0500, Neal Gompa wrote:
On Fri, Nov 20, 2020 at 6:25 AM Andrea Bolognani <abologna@redhat.com> 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/90d9373670564341503f768880e996f01da596a6/... * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/... * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/...
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? -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Nov 23, 2020 at 11:24 AM Andrea Bolognani <abologna@redhat.com> wrote:
On Fri, 2020-11-20 at 08:51 -0500, Neal Gompa wrote:
On Fri, Nov 20, 2020 at 6:25 AM Andrea Bolognani <abologna@redhat.com> 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/90d9373670564341503f768880e996f01da596a6/... * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/... * https://pagure.io/libvirt-deb/blob/90d9373670564341503f768880e996f01da596a6/...
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?
I had been previously building internally at work, but I've set up a public build here now: https://build.opensuse.org/package/show/home:Pharaoh_Atem:libvirt-dev/libvir... We had to backport Meson from Fedora as well to build it, but that wasn't a big deal. :) -- 真実はいつも一つ！/ Always, there's only one truth!
Am Thu, 12 Nov 2020 22:40:02 +0100 schrieb Olaf Hering <olaf@aepfle.de>:
AC_PATH_PROG([PARTED], [parted], [], [$LIBVIRT_SBIN_PATH])
Is there a consensus now how to address this?
From what I understand, using just exec(BASENAME) to search for the binary in PATH is acceptable. It may result in surprises on systems which have BASENAME in /usr/local/*bin. Previously the absolute path at build time was used. I think this can be tolerated.
Should there be a list of each called binary in meson.build? This would at least give some hint about what might be called at runtime, even if the result of find_program() will not be stored in the resulting libvirt binaries. Olaf
participants (6)
-
Andrea Bolognani -
Daniel P. Berrangé -
Martin Kletzander -
Neal Gompa -
Olaf Hering -
Pavel Hrdina