Re: [libvirt PATCH 16/17] meson: drop check for runtime binary dependencies

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 :|