Re: [Qemu-devel] [PATCH RESEND] xen: limit pkg-config to PKG_CONFIG_PATH for xen libraries

[Paul Durrant]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> address@hidden On Behalf Of Paul Durrant
> Sent: 24 March 2017 19:18
> To: 'Stefano Stabellini' <address@hidden>; Juergen Gross
> <address@hidden>
> Cc: Anthony Perard <address@hidden>; xen-
> address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH RESEND] xen: limit pkg-config to
> PKG_CONFIG_PATH for xen libraries
> 
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:address@hidden
> > Sent: 24 March 2017 19:12
> > To: Juergen Gross <address@hidden>
> > Cc: Paul Durrant <address@hidden>; address@hidden;
> xen-
> > address@hidden; Anthony Perard
> <address@hidden>;
> > Stefano Stabellini <address@hidden>
> > Subject: Re: [PATCH RESEND] xen: limit pkg-config to PKG_CONFIG_PATH
> for
> > xen libraries
> >
> > On Fri, 24 Mar 2017, Juergen Gross wrote:
> > > On 24/03/17 18:20, Paul Durrant wrote:
> > > > The Xen tools Makefile has been modified to set PKG_CONFIG_PATH
> > such that
> > > > use of pkg-config in QEMU configure finds the newly built Xen libraries.
> > > > However, because older versions of Xen do not set
> PKG_CONFIG_PATH
> > in the
> > > > Makefile, the QEMU configure script will pick up any Xen libraries that
> > may
> > > > be installed in the build system rather than the newly built ones. Thus,
> > > > if Xen 4.9 is built and installed it becomes impossible to build tools 
> > > > for
> > > > an older version of Xen on the same system (without manual de-
> > installtion).
> > > >
> > > > This patch modifies configure to set PKG_CONFIG_LIBDIR to empty
> when
> > > > looking for Xen libraries to ensure the search is limited only to
> > > > PKG_CONFIG_PATH. This makes sure that, for versions of Xen prior to
> > 4.9,
> > > > pkg-config fails to find the Xen libraries an approriately falls back to
> > > > previous methods of probing.
> > > >
> > > > Signed-off-by: Paul Durrant <address@hidden>
> >
> > Paul, thanks for spotting this. I didn't spot it because I don't have
> > Xen installed in my build environment, but we should not rely on that.
> >
> >
> > > NAK. You are breaking normal qemu build with installed Xen pkg-config
> > > files.
> >
> > The normal QEMU build, done as part of the xen-unstable build, is
> > actually done without Xen being installed in the system. So, if I am not
> > mistaken, the current mechanism also breaks the following entirely
> > legitimate scenario:
> >
> > - user has 4.9 installed on her system
> > - user git clones xen 4.10
> > - user build xen 4.10
> >   - as part of the xen build, qemu is cloned and built
> >   - the qemu configure script picks up the pkgconfig file from
> >     /usr/share and misconfigure the xen version, setting it to 4.9
> >     instead of 4.10
> >   - the qemu build fails
> > - the xen build fails
> >
> > Am I right?
> >
> 
> I think the above would actually work because the 4.10 files would be found
> before the 4.9 files. What fails is installing 4.9 and then trying to build 
> 4.8.

I should qualify that with saying 'trying to build 4.8 with a newer QEMU (i.e. 
one that has a configure which knows about pkg-config).
I still think that's a legitimate use-case though.

  Paul

> 
> > Regardless, both cases need to work correctly. A lot of people rely on
> > out of tree builds, including cross-compilations and openembedded. This
> > is a regression.
> >
> > I suggest we set PKG_CONFIG_LIBDIR (or PKG_CONFIG_PATH?) to the
> right
> > place depending on where the configure script is picking up the Xen
> > header files:
> >
> > - If the configure script picks up the Xen header files from /usr/include,
> > then we want the pkgconfig dir to be /usr/share/pkgconfig.
> >
> > - If the configure script picks up the Xen header files from
> > /local/xen-unstable.git/tools/libxc/include, then we want the pkgconfig
> > dir to be local/xen-unstable.git/tools/pkg-config.
> >
> 
> Yes, that's what is needed.
> 
> > If that's too complex, we can always go back to the good old,
> > non-pkgconfig days.
> >
> 
> Unless this can be speedily resolved I think reverting the pkg-config changes
> would be best.
> 
> Another possibility perhaps would be an explicit configure flag for in-tree 
> and
> out-of-tree builds so we can say exactly where QEMU should be getting its
> environment from?
> 
>   Paul
> 
> >
> > > > Cc: Anthony Perard <address@hidden>
> > > > Cc: Stefano Stabellini <address@hidden>
> > > > Cc: Juergen Gross <address@hidden>
> > > > ---
> > > >  configure | 12 ++++++++----
> > > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/configure b/configure
> > > > index fdf47e4..6ef5980 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -1974,6 +1974,10 @@ fi
> > > >  ##########################################
> > > >  # xen probe
> > > >
> > > > +xen_query_pkg_config() {
> > > > +    PKG_CONFIG_LIBDIR= ${pkg_config_exe} "$@"
> > > > +}
> > > > +
> > > >  if test "$xen" != "no" ; then
> > > >    xen_libs="-lxenstore -lxenctrl -lxenguest"
> > > >    xen_stable_libs="-lxenforeignmemory -lxengnttab -lxenevtchn"
> > > > @@ -1997,9 +2001,9 @@ EOF
> > > >      xen=no
> > > >
> > > >    # Xen version via pkg-config (Xen 4.9.0 and newer)
> > > > -  elif $pkg_config --exists xencontrol ; then
> > > > +  elif xen_query_pkg_config --exists xencontrol; then
> > > >      xen_ctrl_version="$(printf '%d%02d%02d' \
> > > > -      $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
> > > > +      $(xen_query_pkg_config --modversion xencontrol | sed 's/\./ /g')
> )"
> > > >      xen=yes
> > > >
> > > >    elif
> > > > @@ -2216,8 +2220,8 @@ EOF
> > > >      if test $xen_ctrl_version -ge 40900 ; then
> > > >        xen_pc="xencontrol xenstore xenguest xenforeignmemory
> > xengnttab xenevtchn"
> > > >        xen_pc="$xen_pc xendevicemodel"
> > > > -      xen_libs="$($pkg_config --libs $xen_pc)"
> > > > -      QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
> > > > +      xen_libs="$(xen_query_pkg_config --libs $xen_pc)"
> > > > +      QEMU_CFLAGS="$QEMU_CFLAGS $(xen_query_pkg_config --
> cflags
> > $xen_pc)"
> > > >      elif test $xen_ctrl_version -ge 40701 ; then
> > > >        libs_softmmu="$xen_stable_libs $libs_softmmu"
> > > >      fi
> > > >
> > >