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
Attachment:
signature.asc
Description: PGP signature
