Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

[Daniel P . Berrangé]

On Mon, Nov 14, 2022 at 12:13:48PM +0300, Michael Tokarev wrote:
> 14.11.2022 11:58, Daniel P. Berrangé wrote:
> ..
> > > On current systems, using <capstone/capstone.h> works
> > > now (despite the pkg-config-supplied -I/usr/include/capstone) -
> > > since on all systems capstone headers are put into capstone/
> > > subdirectory of a system include dir. So this change is
> > > compatible with both the obsolete way of including it
> > > and the only future way.
> > 
> > AFAIR, macOS HomeBrew does not put anything into the system
> > include dir, and always requires -I flags to be correct.
> 
> Does it work with the capstone-supplied --cflags and the proposed
> include path?  What does pkg-config --cflags capstone return there?

I see the QEMU build logs adding:

 -I/usr/local/Cellar/capstone/4.0.2/include/capstone

so  #include <capstone/capstone.h>   seems unlikely to work

> > > -  if capstone.found() and not cc.compiles('#include <capstone.h>',
> > > +  if capstone.found() and not cc.compiles('#include 
> > > <capstone/capstone.h>',
> > >                                             dependencies: [capstone])
> > 
> > To retain back compat this could probe for both ways
> > 
> >      if capstone.found()
> >          if cc.compiles('#include <capstone/capstone.h>',
> >                    dependencies: [capstone])
> >             ...
> >          else if cc.compiles('#include <capstone.h>',
> >                         dependencies: [capstone])
> >             ...
> > then, the source file can try the correct #include based on what
> > we detect works here.
> 
> I don't think this deserves the complexity really, unless there *is*
> a system out there which actually needs this.
> 
> I mean, these little compat tweaks, - it becomes twisty with time,
> and no one knows which code paths and config variables are needed
> for what, and whole thing slowly becomes unmanageable... If it's
> easy to make it unconditional, it should be done. IMHO anyway :)

Well you're proposing a change during RC time which is likely to
break builds if the assumption that its always in the system
include path is wrong. So I think the explicit compatibility is
required to reduce the risk of this creating a regression.

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