These binaries are used only during runtime so technically there is no
need to check for them while compiling libvirt.
Usually the location is the same while compiling and running but it may
not be true. In addition they are not strictly required to compile the
code so this way developers don't have to install it or create fake
binaries in order to compile the code.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
meson.build | 170 ++-----------------------
src/bhyve/bhyve_command.c | 4 +
src/locking/lock_driver_lockd.c | 12 +-
src/network/bridge_driver.c | 2 +
src/node_device/node_device_driver.c | 2 +
src/qemu/qemu_conf.c | 5 +
src/storage/storage_backend_logical.c | 13 ++
src/storage/storage_backend_sheepdog.c | 2 +
src/storage/storage_backend_zfs.c | 3 +
src/storage/storage_util.c | 2 +
src/storage/storage_util.h | 6 +
src/util/virdnsmasq.c | 1 +
src/util/virfirewall.h | 4 +
src/util/viriscsi.h | 2 +
src/util/virkmod.h | 3 +
src/util/virnetdevbandwidth.h | 2 +
src/util/virnetdevip.c | 2 +
src/util/virnetdevmidonet.c | 2 +
src/util/virnetdevopenvswitch.c | 2 +
src/util/virnuma.c | 1 +
src/util/virsysinfo.c | 1 +
src/util/virutil.c | 2 +
22 files changed, 72 insertions(+), 171 deletions(-)
diff --git a/meson.build b/meson.build
index 837955de37..1c27dd030e 100644
--- a/meson.build
+++ b/meson.build
@@ -926,24 +926,8 @@ endforeach
optional_programs = [
'augparse',
- 'dmidecode',
- 'dnsmasq',
- 'ebtables',
'flake8',
- 'ip',
- 'ip6tables',
- 'iptables',
- 'iscsiadm',
- 'mdevctl',
- 'mm-ctl',
- 'modprobe',
- 'ovs-vsctl',
'pdwtags',
- 'radvd',
- 'rmmod',
- 'scrub',
- 'tc',
- 'udevadm',
]
foreach name : optional_programs
@@ -1088,14 +1072,6 @@ endif
libparted_version = '1.8.0'
libparted_dep = dependency('libparted', version: '>=' + libparted_version, required: false)
-if libparted_dep.found()
- parted_prog = find_program('parted', required: false, dirs: libvirt_sbin_path)
- if parted_prog.found()
- conf.set_quoted('PARTED', parted_prog.path())
- else
- libparted_dep = dependency('', required: false)
- endif
-endif
libpcap_version = '1.5.0'
if not get_option('libpcap').disabled()
@@ -1498,16 +1474,7 @@ if not get_option('driver_libvirtd').disabled()
endif
if not get_option('driver_bhyve').disabled() and host_machine.system() == 'freebsd'
- bhyve_prog = find_program('bhyve', required: get_option('driver_bhyve'))
- bhyvectl_prog = find_program('bhyvectl', required: get_option('driver_bhyve'))
- bhyveload_prog = find_program('bhyveload', required: get_option('driver_bhyve'))
-
- if bhyve_prog.found() and bhyvectl_prog.found() and bhyveload_prog.found()
- conf.set('WITH_BHYVE', 1)
- conf.set_quoted('BHYVE', bhyve_prog.path())
- conf.set_quoted('BHYVECTL', bhyvectl_prog.path())
- conf.set_quoted('BHYVELOAD', bhyveload_prog.path())
- endif
+ conf.set('WITH_BHYVE', 1)
elif get_option('driver_bhyve').enabled()
error('The bhyve driver cannot be enabled')
endif
@@ -1706,54 +1673,6 @@ if not get_option('driver_qemu').disabled()
endif
conf.set_quoted('QEMU_USER', qemu_user)
conf.set_quoted('QEMU_GROUP', qemu_group)
-
- qemu_bridge_prog = find_program(
- 'qemu-bridge-helper',
- dirs: [ '/usr/libexec', '/usr/lib/qemu', '/usr/lib' ],
- required: false
- )
- if qemu_bridge_prog.found()
- qemu_bridge_path = qemu_bridge_prog.path()
- else
- qemu_bridge_path = '/usr/libexec/qemu-bridge-helper'
- endif
- conf.set_quoted('QEMU_BRIDGE_HELPER', qemu_bridge_path)
-
- qemu_pr_prog = find_program(
- 'qemu-pr-helper',
- dirs: [ '/usr/bin', '/usr/libexec' ],
- required: false
- )
- if qemu_pr_prog.found()
- qemu_pr_path = qemu_pr_prog.path()
- else
- qemu_pr_path = '/usr/bin/qemu-pr-helper'
- endif
- conf.set_quoted('QEMU_PR_HELPER', qemu_pr_path)
-
- qemu_slirp_prog = find_program(
- 'slirp-helper',
- dirs: [ '/usr/bin', '/usr/libexec' ],
- required: false
- )
- if qemu_slirp_prog.found()
- qemu_slirp_path = qemu_slirp_prog.path()
- else
- qemu_slirp_path = '/usr/bin/slirp-helper'
- endif
- conf.set_quoted('QEMU_SLIRP_HELPER', qemu_slirp_path)
-
- qemu_dbus_daemon_prog = find_program(
- 'dbus-daemon',
- dirs: [ '/usr/bin', '/usr/libexec' ],
- required: false
- )
- if qemu_dbus_daemon_prog.found()
- qemu_dbus_daemon_path = qemu_dbus_daemon_prog.path()
- else
- qemu_dbus_daemon_path = '/usr/bin/dbus-daemon'
- endif
- conf.set_quoted('QEMU_DBUS_DAEMON', qemu_dbus_daemon_path)
endif
endif
@@ -1831,30 +1750,9 @@ if conf.has('WITH_LIBVIRTD')
endif
endif
- if fs_enable
- mount_prog = find_program('mount', required: get_option('storage_fs'), dirs: libvirt_sbin_path)
- umount_prog = find_program('umount', required: get_option('storage_fs'), dirs: libvirt_sbin_path)
- mkfs_prog = find_program('mkfs', required: get_option('storage_fs'), dirs: libvirt_sbin_path)
-
- if not mount_prog.found() or not umount_prog.found() or not mkfs_prog.found()
- fs_enable = false
- endif
- endif
-
if fs_enable
use_storage = true
-
conf.set('WITH_STORAGE_FS', 1)
- conf.set_quoted('MOUNT', mount_prog.path())
- conf.set_quoted('UMOUNT', umount_prog.path())
- conf.set_quoted('MKFS', mkfs_prog.path())
-
- showmount_prog = find_program('showmount', required: false, dirs: libvirt_sbin_path)
- showmount_path = ''
- if showmount_prog.found()
- showmount_path = showmount_prog.path()
- endif
- conf.set_quoted('SHOWMOUNT', showmount_path)
endif
endif
@@ -1865,11 +1763,9 @@ if conf.has('WITH_LIBVIRTD')
error('Need glusterfs (libgfapi) for gluster storage driver')
endif
- if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
+ if not get_option('storage_iscsi').disabled()
use_storage = true
conf.set('WITH_STORAGE_ISCSI', 1)
- elif get_option('storage_iscsi').enabled()
- error('We need iscsiadm for iSCSI storage driver')
endif
if not get_option('storage_iscsi_direct').disabled() and libiscsi_dep.found()
@@ -1880,31 +1776,8 @@ if conf.has('WITH_LIBVIRTD')
endif
if not get_option('storage_lvm').disabled()
- lvm_enable = true
- lvm_progs = [
- 'pvcreate', 'vgcreate', 'lvcreate',
- 'pvremove', 'vgremove', 'lvremove',
- 'lvchange', 'vgchange', 'vgscan',
- 'pvs', 'vgs', 'lvs',
- ]
- foreach name : lvm_progs
- set_variable(
- '@0@_prog'.format(name),
- find_program(name, required: get_option('storage_lvm'), dirs: libvirt_sbin_path)
- )
- if not get_variable('@0@_prog'.format(name)).found()
- lvm_enable = false
- endif
- endforeach
-
- if lvm_enable
- use_storage = true
- conf.set('WITH_STORAGE_LVM', 1)
-
- foreach name : lvm_progs
- conf.set_quoted(name.to_upper(), get_variable('@0@_prog'.format(name)).path())
- endforeach
- endif
+ use_storage = true
+ conf.set('WITH_STORAGE_LVM', 1)
endif
if not get_option('storage_mpath').disabled() and host_machine.system() == 'linux' and devmapper_dep.found()
@@ -1927,13 +1800,8 @@ if conf.has('WITH_LIBVIRTD')
endif
if not get_option('storage_sheepdog').disabled()
- sheepdogcli_prog = find_program('dog', required: get_option('storage_sheepdog'), dirs: libvirt_sbin_path)
-
- if sheepdogcli_prog.found()
- use_storage = true
- conf.set('WITH_STORAGE_SHEEPDOG', 1)
- conf.set_quoted('SHEEPDOGCLI', sheepdogcli_prog.path())
- endif
+ use_storage = true
+ conf.set('WITH_STORAGE_SHEEPDOG', 1)
endif
if not get_option('storage_vstorage').disabled()
@@ -1953,24 +1821,8 @@ if conf.has('WITH_LIBVIRTD')
endif
if not get_option('storage_zfs').disabled()
- zfs_enable = true
- foreach name : ['zfs', 'zpool']
- set_variable(
- '@0@_prog'.format(name),
- find_program(name, required: get_option('storage_zfs'), dirs: libvirt_sbin_path)
- )
- if not get_variable('@0@_prog'.format(name)).found()
- zfs_enable = false
- endif
- endforeach
-
- if zfs_enable
- use_storage = true
- conf.set('WITH_STORAGE_ZFS', 1)
- foreach name : ['zfs', 'zpool']
- conf.set_quoted(name.to_upper(), get_variable('@0@_prog'.format(name)).path())
- endforeach
- endif
+ use_storage = true
+ conf.set('WITH_STORAGE_ZFS', 1)
endif
endif
@@ -2080,11 +1932,7 @@ if not get_option('nss').disabled()
endif
if not get_option('numad').disabled() and numactl_dep.found()
- numad_prog = find_program('numad', required: get_option('numad'), dirs: libvirt_sbin_path)
- if numad_prog.found()
- conf.set('WITH_NUMAD', 1)
- conf.set_quoted('NUMAD', numad_prog.path())
- endif
+ conf.set('WITH_NUMAD', 1)
elif get_option('numad').enabled()
error('You must have numactl enabled for numad support.')
endif
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index b1558132e1..798736a1ec 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -37,6 +37,10 @@
#define VIR_FROM_THIS VIR_FROM_BHYVE
+#define BHYVE "bhyve"
+#define BHYVECTL "bhyvectl"
+#define BHYVELOAD "bhyveload"
+
VIR_LOG_INIT("bhyve.bhyve_command");
static int
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 823b918db3..ea787bf87d 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -454,7 +454,8 @@ static int virLockManagerLockDaemonNew(virLockManager *lock,
}
-#ifdef LVS
+#define LVS "lvs"
+
static int
virLockManagerGetLVMKey(const char *path,
char **key)
@@ -508,15 +509,6 @@ virLockManagerGetLVMKey(const char *path,
return ret;
}
-#else
-static int
-virLockManagerGetLVMKey(const char *path,
- char **key G_GNUC_UNUSED)
-{
- virReportSystemError(ENOSYS, _("Unable to get LVM key for %s"), path);
- return -1;
-}
-#endif
static int virLockManagerLockDaemonAddResource(virLockManager *lock,
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ee3f9dab0a..8e1a42b119 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -74,6 +74,8 @@
#define VIR_FROM_THIS VIR_FROM_NETWORK
#define MAX_BRIDGE_ID 256
+#define RADVD "radvd"
+
static virMutex bridgeNameValidateMutex = VIR_MUTEX_INITIALIZER;
/**
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 8227cd8342..52fcb85ee8 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -48,6 +48,8 @@
VIR_LOG_INIT("node_device.node_device_driver");
+#define MDEVCTL "mdevctl"
+
virNodeDeviceDriverState *driver;
virDrvOpenStatus
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 437b3ce2be..d228504d8c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -55,6 +55,11 @@
VIR_LOG_INIT("qemu.qemu_conf");
+#define QEMU_BRIDGE_HELPER "qemu-bridge-helper"
+#define QEMU_PR_HELPER "qemu-pr-helper"
+#define QEMU_SLIRP_HELPER "slirp-helper"
+#define QEMU_DBUS_DAEMON "dbus-daemon"
+
/* These are only defaults, they can be changed now in qemu.conf and
* explicitly specified port is checked against these two (makes
* sense to limit the values).
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
index ed8e47d880..4bbb585d07 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -40,6 +40,19 @@
VIR_LOG_INIT("storage.storage_backend_logical");
+#define PVCREATE "pvcreate"
+#define PVREMOVE "pvremove"
+#define PVS "pvs"
+#define VGCREATE "vgcreate"
+#define VGREMOVE "vgremove"
+#define VGCHANGE "vgchange"
+#define VGSCAN "vgscan"
+#define VGS "vgs"
+#define LVCREATE "lvcreate"
+#define LVREMOVE "lvremove"
+#define LVCHANGE "lvchange"
+#define LVS "lvs"
+
static int
virStorageBackendLogicalSetActive(virStoragePoolObj *pool,
diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c
index 6490dfae52..beb6445e1e 100644
--- a/src/storage/storage_backend_sheepdog.c
+++ b/src/storage/storage_backend_sheepdog.c
@@ -35,6 +35,8 @@
#define VIR_FROM_THIS VIR_FROM_STORAGE
+#define SHEEPDOGCLI "dog"
+
static int virStorageBackendSheepdogRefreshVol(virStoragePoolObj *pool,
virStorageVolDef *vol);
diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
index 6fabeade11..2a5d74357d 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -33,6 +33,9 @@
VIR_LOG_INIT("storage.storage_backend_zfs");
+#define ZFS "zfs"
+#define ZPOOL "zpool"
+
/*
* Some common flags of zfs and zpool commands we use:
* -H -- don't print headers and separate fields by tab
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 7efadc2197..c225f2ab6b 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -84,6 +84,8 @@ VIR_LOG_INIT("storage.storage_util");
# define S_IRWXUGO (S_IRWXU | S_IRWXG | S_IRWXO)
#endif
+#define SCRUB "scrub"
+
/* virStorageBackendNamespaceInit:
* @poolType: virStoragePoolType
* @xmlns: Storage Pool specific namespace callback methods
diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h
index aa3c25e9fc..487345ff22 100644
--- a/src/storage/storage_util.h
+++ b/src/storage/storage_util.h
@@ -25,6 +25,12 @@
#include "storage_driver.h"
#include "storage_backend.h"
+#define PARTED "parted"
+#define MOUNT "mount"
+#define UMOUNT "umount"
+#define MKFS "mkfs"
+#define SHOWMOUNT "showmount"
+
/* Storage Pool Namespace options to share w/ storage_backend_fs.c and
* the virStorageBackendFileSystemMountCmd method */
typedef struct _virStoragePoolFSMountOptionsDef virStoragePoolFSMountOptionsDef;
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index f2f606913f..b666c2f9d3 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -48,6 +48,7 @@ VIR_LOG_INIT("util.dnsmasq");
#define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile"
#define DNSMASQ_ADDNHOSTSFILE_SUFFIX "addnhosts"
+#define DNSMASQ "dnsmasq"
static void
dhcphostFreeContent(dnsmasqDhcpHost *host)
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 169d99fe2b..a3954c5ed0 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -22,6 +22,10 @@
#include "internal.h"
+#define EBTABLES_PATH "ebtables"
+#define IPTABLES_PATH "iptables"
+#define IP6TABLES_PATH "ip6tables"
+
typedef struct _virFirewall virFirewall;
typedef struct _virFirewallRule virFirewallRule;
diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h
index 6d17d139eb..fa7ff280c2 100644
--- a/src/util/viriscsi.h
+++ b/src/util/viriscsi.h
@@ -23,6 +23,8 @@
#include "internal.h"
+#define ISCSIADM "iscsiadm"
+
char *
virISCSIGetSession(const char *devpath,
bool probe)
diff --git a/src/util/virkmod.h b/src/util/virkmod.h
index bb043d4876..58311909eb 100644
--- a/src/util/virkmod.h
+++ b/src/util/virkmod.h
@@ -23,6 +23,9 @@
#include "internal.h"
+#define MODPROBE "modprobe"
+#define RMMOD "rmmod"
+
char *virKModLoad(const char *)
ATTRIBUTE_NONNULL(1);
char *virKModUnload(const char *)
diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
index 3d520721f6..6c91bd0f33 100644
--- a/src/util/virnetdevbandwidth.h
+++ b/src/util/virnetdevbandwidth.h
@@ -21,6 +21,8 @@
#include "internal.h"
#include "virmacaddr.h"
+#define TC "tc"
+
typedef struct _virNetDevBandwidthRate virNetDevBandwidthRate;
struct _virNetDevBandwidthRate {
unsigned long long average; /* kbytes/s */
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 0ce8f5b536..f4cf6ac39f 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -50,6 +50,8 @@
#define VIR_FROM_THIS VIR_FROM_NONE
+#define IP_PATH "ip"
+
VIR_LOG_INIT("util.netdevip");
#if defined(WITH_LIBNL)
diff --git a/src/util/virnetdevmidonet.c b/src/util/virnetdevmidonet.c
index 9061f1516f..735607e5df 100644
--- a/src/util/virnetdevmidonet.c
+++ b/src/util/virnetdevmidonet.c
@@ -26,6 +26,8 @@
#define VIR_FROM_THIS VIR_FROM_NONE
+#define MM_CTL "mm-ctl"
+
/**
* virNetDevMidonetBindPort:
* @ifname: the network interface name
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 21ee4bdd42..a05128dbca 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -35,6 +35,8 @@
VIR_LOG_INIT("util.netdevopenvswitch");
+#define OVS_VSCTL "ovs-vsctl"
+
/*
* Set openvswitch default timeout
*/
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 0c9599003a..2a540d7a11 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -52,6 +52,7 @@
VIR_LOG_INIT("util.numa");
+#define NUMAD "numad"
#if WITH_NUMAD
char *
diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
index af9e03c5ac..8bbda9ea69 100644
--- a/src/util/virsysinfo.c
+++ b/src/util/virsysinfo.c
@@ -53,6 +53,7 @@ static const char *sysinfoCpuinfo = "/proc/cpuinfo";
#define SYSINFO sysinfoSysinfo
#define CPUINFO sysinfoCpuinfo
#define CPUINFO_FILE_LEN (1024*1024) /* 1MB limit for /proc/cpuinfo file */
+#define DMIDECODE "dmidecode"
void
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 3f49a469e5..b7a9808f6a 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -57,6 +57,8 @@
#include "virutil.h"
#include "virsocket.h"
+#define UDEVADM "udevadm"
+
G_STATIC_ASSERT(sizeof(gid_t) <= sizeof(unsigned int) &&
sizeof(uid_t) <= sizeof(unsigned int));
--
2.30.2
On Fri, Apr 16, 2021 at 09:12:45PM +0200, Pavel Hrdina wrote:
> These binaries are used only during runtime so technically there is no
> need to check for them while compiling libvirt.
>
> Usually the location is the same while compiling and running but it may
> not be true. In addition they are not strictly required to compile the
> code so this way developers don't have to install it or create fake
> binaries in order to compile the code.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> meson.build | 170 ++-----------------------
> src/bhyve/bhyve_command.c | 4 +
> src/locking/lock_driver_lockd.c | 12 +-
> src/network/bridge_driver.c | 2 +
> src/node_device/node_device_driver.c | 2 +
> src/qemu/qemu_conf.c | 5 +
> src/storage/storage_backend_logical.c | 13 ++
> src/storage/storage_backend_sheepdog.c | 2 +
> src/storage/storage_backend_zfs.c | 3 +
> src/storage/storage_util.c | 2 +
> src/storage/storage_util.h | 6 +
> src/util/virdnsmasq.c | 1 +
> src/util/virfirewall.h | 4 +
> src/util/viriscsi.h | 2 +
> src/util/virkmod.h | 3 +
> src/util/virnetdevbandwidth.h | 2 +
> src/util/virnetdevip.c | 2 +
> src/util/virnetdevmidonet.c | 2 +
> src/util/virnetdevopenvswitch.c | 2 +
> src/util/virnuma.c | 1 +
> src/util/virsysinfo.c | 1 +
> src/util/virutil.c | 2 +
> 22 files changed, 72 insertions(+), 171 deletions(-)
>
> @@ -1865,11 +1763,9 @@ if conf.has('WITH_LIBVIRTD')
> error('Need glusterfs (libgfapi) for gluster storage driver')
> endif
>
> - if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
> + if not get_option('storage_iscsi').disabled()
> use_storage = true
> conf.set('WITH_STORAGE_ISCSI', 1)
So this now enables iSCSI even on platforms which don't support iscsiadm
tools.
> - elif get_option('storage_iscsi').enabled()
> - error('We need iscsiadm for iSCSI storage driver')
> endif
>
> if not get_option('storage_iscsi_direct').disabled() and libiscsi_dep.found()
> @@ -1880,31 +1776,8 @@ if conf.has('WITH_LIBVIRTD')
> endif
>
> if not get_option('storage_lvm').disabled()
snip
> + use_storage = true
> + conf.set('WITH_STORAGE_LVM', 1)
And enables LVM on all platforms, even though this is Linux only..
Overall this patch makes it so that our meson rules don't
"do the right thing" as a default behaviour, and just enable
everything whether you have it installed or not.
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 Mon, 2021-04-19 at 13:35 +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 16, 2021 at 09:12:45PM +0200, Pavel Hrdina wrote:
> > - if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
> > + if not get_option('storage_iscsi').disabled()
> > use_storage = true
> > conf.set('WITH_STORAGE_ISCSI', 1)
>
> So this now enables iSCSI even on platforms which don't support iscsiadm
> tools.
>
> > + use_storage = true
> > + conf.set('WITH_STORAGE_LVM', 1)
>
> And enables LVM on all platforms, even though this is Linux only..
>
> Overall this patch makes it so that our meson rules don't
> "do the right thing" as a default behaviour, and just enable
> everything whether you have it installed or not.
Can't we address that by implementing explicit OS checks?
Right now we're using "the iscsiadm command happens to be installed
on the build host" as a proxy for "the iscsiadm command will be
available on the deployment host", which is not a bad approximation
but is not quite right either.
--
Andrea Bolognani / Red Hat / Virtualization
On Mon, Apr 19, 2021 at 06:01:42PM +0200, Andrea Bolognani wrote:
> On Mon, 2021-04-19 at 13:35 +0100, Daniel P. Berrangé wrote:
> > On Fri, Apr 16, 2021 at 09:12:45PM +0200, Pavel Hrdina wrote:
> > > - if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
> > > + if not get_option('storage_iscsi').disabled()
> > > use_storage = true
> > > conf.set('WITH_STORAGE_ISCSI', 1)
> >
> > So this now enables iSCSI even on platforms which don't support iscsiadm
> > tools.
> >
> > > + use_storage = true
> > > + conf.set('WITH_STORAGE_LVM', 1)
> >
> > And enables LVM on all platforms, even though this is Linux only..
> >
> > Overall this patch makes it so that our meson rules don't
> > "do the right thing" as a default behaviour, and just enable
> > everything whether you have it installed or not.
>
> Can't we address that by implementing explicit OS checks?
For some of the that's easy, if they're obviously Linux or FreeBSD
specific. Others though the platform supportability is not so
clear, at least not unless someone wants to check the tools and
what platforms they genuinely claim to support.
> Right now we're using "the iscsiadm command happens to be installed
> on the build host" as a proxy for "the iscsiadm command will be
> available on the deployment host", which is not a bad approximation
> but is not quite right either.
Both approaches are approximations - checking the binary may miss out
platforms where the feature is possible but not installed by the user.
Checking the OS platform may miss out platforms where libvirt devs
didn't realize there was support for the 3rd party tool. The former
was more the favoured approach as it removes libvirt devs from being
the problem, and makes the users' be the problem :-)
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 Mon, Apr 19, 2021 at 05:05:56PM +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 19, 2021 at 06:01:42PM +0200, Andrea Bolognani wrote:
> > On Mon, 2021-04-19 at 13:35 +0100, Daniel P. Berrangé wrote:
> > > On Fri, Apr 16, 2021 at 09:12:45PM +0200, Pavel Hrdina wrote:
> > > > - if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
> > > > + if not get_option('storage_iscsi').disabled()
> > > > use_storage = true
> > > > conf.set('WITH_STORAGE_ISCSI', 1)
> > >
> > > So this now enables iSCSI even on platforms which don't support iscsiadm
> > > tools.
> > >
> > > > + use_storage = true
> > > > + conf.set('WITH_STORAGE_LVM', 1)
> > >
> > > And enables LVM on all platforms, even though this is Linux only..
> > >
> > > Overall this patch makes it so that our meson rules don't
> > > "do the right thing" as a default behaviour, and just enable
> > > everything whether you have it installed or not.
> >
> > Can't we address that by implementing explicit OS checks?
>
> For some of the that's easy, if they're obviously Linux or FreeBSD
> specific. Others though the platform supportability is not so
> clear, at least not unless someone wants to check the tools and
> what platforms they genuinely claim to support.
>
> > Right now we're using "the iscsiadm command happens to be installed
> > on the build host" as a proxy for "the iscsiadm command will be
> > available on the deployment host", which is not a bad approximation
> > but is not quite right either.
>
> Both approaches are approximations - checking the binary may miss out
> platforms where the feature is possible but not installed by the user.
> Checking the OS platform may miss out platforms where libvirt devs
> didn't realize there was support for the 3rd party tool. The former
> was more the favoured approach as it removes libvirt devs from being
> the problem, and makes the users' be the problem :-)
In addition it allows devs to compile that part of code without
installing the binaries or in some cases without using hacks like this:
ln -s /required/bin /usr/bin/true
So I would prefer checking binaries instead of OS checks.
Pavel
On Mon, Apr 19, 2021 at 06:22:18PM +0200, Pavel Hrdina wrote:
> On Mon, Apr 19, 2021 at 05:05:56PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Apr 19, 2021 at 06:01:42PM +0200, Andrea Bolognani wrote:
> > > On Mon, 2021-04-19 at 13:35 +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Apr 16, 2021 at 09:12:45PM +0200, Pavel Hrdina wrote:
> > > > > - if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
> > > > > + if not get_option('storage_iscsi').disabled()
> > > > > use_storage = true
> > > > > conf.set('WITH_STORAGE_ISCSI', 1)
> > > >
> > > > So this now enables iSCSI even on platforms which don't support iscsiadm
> > > > tools.
> > > >
> > > > > + use_storage = true
> > > > > + conf.set('WITH_STORAGE_LVM', 1)
> > > >
> > > > And enables LVM on all platforms, even though this is Linux only..
> > > >
> > > > Overall this patch makes it so that our meson rules don't
> > > > "do the right thing" as a default behaviour, and just enable
> > > > everything whether you have it installed or not.
> > >
> > > Can't we address that by implementing explicit OS checks?
> >
> > For some of the that's easy, if they're obviously Linux or FreeBSD
> > specific. Others though the platform supportability is not so
> > clear, at least not unless someone wants to check the tools and
> > what platforms they genuinely claim to support.
> >
> > > Right now we're using "the iscsiadm command happens to be installed
> > > on the build host" as a proxy for "the iscsiadm command will be
> > > available on the deployment host", which is not a bad approximation
> > > but is not quite right either.
> >
> > Both approaches are approximations - checking the binary may miss out
> > platforms where the feature is possible but not installed by the user.
> > Checking the OS platform may miss out platforms where libvirt devs
> > didn't realize there was support for the 3rd party tool. The former
> > was more the favoured approach as it removes libvirt devs from being
> > the problem, and makes the users' be the problem :-)
>
> In addition it allows devs to compile that part of code without
> installing the binaries or in some cases without using hacks like this:
>
> ln -s /required/bin /usr/bin/true
>
> So I would prefer checking binaries instead of OS checks.
If we only check when the meson feature config is "auto", then we dont
need the link hacks - just set "-Dfeature=on" when running meson.
Once we only use the checks for the "auto" case, then it is less
critical if our check is wrong, as it can be overridden. Given
this, I think we could have a mixture of binary or OS checks
depending on what is best suited.
eg LVM could easily be a linux OS check, whle sheepdog still best
as a binary check.
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 Mon, Apr 19, 2021 at 05:26:55PM +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 19, 2021 at 06:22:18PM +0200, Pavel Hrdina wrote:
> > On Mon, Apr 19, 2021 at 05:05:56PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Apr 19, 2021 at 06:01:42PM +0200, Andrea Bolognani wrote:
> > > > On Mon, 2021-04-19 at 13:35 +0100, Daniel P. Berrangé wrote:
> > > > > On Fri, Apr 16, 2021 at 09:12:45PM +0200, Pavel Hrdina wrote:
> > > > > > - if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
> > > > > > + if not get_option('storage_iscsi').disabled()
> > > > > > use_storage = true
> > > > > > conf.set('WITH_STORAGE_ISCSI', 1)
> > > > >
> > > > > So this now enables iSCSI even on platforms which don't support iscsiadm
> > > > > tools.
> > > > >
> > > > > > + use_storage = true
> > > > > > + conf.set('WITH_STORAGE_LVM', 1)
> > > > >
> > > > > And enables LVM on all platforms, even though this is Linux only..
> > > > >
> > > > > Overall this patch makes it so that our meson rules don't
> > > > > "do the right thing" as a default behaviour, and just enable
> > > > > everything whether you have it installed or not.
> > > >
> > > > Can't we address that by implementing explicit OS checks?
> > >
> > > For some of the that's easy, if they're obviously Linux or FreeBSD
> > > specific. Others though the platform supportability is not so
> > > clear, at least not unless someone wants to check the tools and
> > > what platforms they genuinely claim to support.
> > >
> > > > Right now we're using "the iscsiadm command happens to be installed
> > > > on the build host" as a proxy for "the iscsiadm command will be
> > > > available on the deployment host", which is not a bad approximation
> > > > but is not quite right either.
> > >
> > > Both approaches are approximations - checking the binary may miss out
> > > platforms where the feature is possible but not installed by the user.
> > > Checking the OS platform may miss out platforms where libvirt devs
> > > didn't realize there was support for the 3rd party tool. The former
> > > was more the favoured approach as it removes libvirt devs from being
> > > the problem, and makes the users' be the problem :-)
> >
> > In addition it allows devs to compile that part of code without
> > installing the binaries or in some cases without using hacks like this:
> >
> > ln -s /required/bin /usr/bin/true
> >
> > So I would prefer checking binaries instead of OS checks.
>
> If we only check when the meson feature config is "auto", then we dont
> need the link hacks - just set "-Dfeature=on" when running meson.
>
> Once we only use the checks for the "auto" case, then it is less
> critical if our check is wrong, as it can be overridden. Given
> this, I think we could have a mixture of binary or OS checks
> depending on what is best suited.
That's what I'm aiming for in v2. Currently this is not possible and
that hack is the only way in some cases.
Pavel
> eg LVM could easily be a linux OS check, whle sheepdog still best
> as a binary check.
>
> 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 :|
>
© 2016 - 2026 Red Hat, Inc.