[vlc-devel] [PATCH v2 3/5] contrib: detect the pkg-config variant to use when cross-compiling

[Alexandre Janniaux]

Hi,

On Thu, Mar 19, 2020 at 05:02:29PM +0100, Steve Lhomme wrote:
> On 2020-03-19 16:34, Alexandre Janniaux wrote:
> > Hi,
> >
> > On Wed, Mar 18, 2020 at 09:46:14AM +0100, Steve Lhomme wrote:
> > > And use it for all contribs detection and building.
> > >
> > > on Debian, the x86_64-w64-mingw32-pkg-config may exist and run but give an
> > > error when actually trying to find a package because the package architecture
> > > doesn't exist for that target. So we have to test it thoroughly.
> > >
> > > If we revert to pkg-config when cross-compiling we also force the
> > > PKG_CONFIG_LIBDIR, otherwise we leave it untouched as the system/environment may
> > > provide more that we know.
> > >
> > > Both /usr/$(HOST)/lib/pkgconfig and /usr/lib/$(HOST)/pkgconfig variants exist,
> > > at least on Debian.
> > > ---
> > >   contrib/src/main.mak | 20 +++++++++++++++++---
> > >   1 file changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/contrib/src/main.mak b/contrib/src/main.mak
> > > index 555d96a4b42..bff9cd82413 100644
> > > --- a/contrib/src/main.mak
> > > +++ b/contrib/src/main.mak
> > > @@ -82,6 +82,7 @@ RANLIB ?= ranlib
> > >   STRIP ?= strip
> > >   WIDL ?= widl
> > >   WINDRES ?= windres
> > > +PKG_CONFIG ?= pkg-config
> > >   else
> > >   ifneq ($(findstring $(origin CC),undefined default),)
> > >   CC := $(HOST)-gcc
> > > @@ -99,6 +100,22 @@ RANLIB ?= $(HOST)-ranlib
> > >   STRIP ?= $(HOST)-strip
> > >   WIDL ?= $(HOST)-widl
> > >   WINDRES ?= $(HOST)-windres
> > > +
> > > +# On Debian x86_64-w64-mingw32-pkg-config exists, runs but returns an error when checking packages
> > > +ifeq ($(shell unset PKG_CONFIG_LIBDIR; $(HOST)-pkg-config --version 1>/dev/null 2>/dev/null || echo FAIL),)
> > > +PKG_CONFIG ?= $(HOST)-pkg-config
> > > +else
> > > +# Use the regular pkg-config and set some PKG_CONFIG_LIBDIR ourselves
> > > +PKG_CONFIG = pkg-config
> >
> > The PKG_CONFIG assignment above is missing an additional ?= right?
> > Otherwise there is no point in putting a ?= above.
>
> No in the former case the HOST-pkg-config is tested but the user is allowed
> to change the value.
> In the latter case we force the PKG_CONFIG_LIBDIR and so we don't want to
> use whatever the user provided that might mess with what we're doing.

This seems wrong to me.

On the one hand, you're basically allowing pkg-config to be
overridden when not in cross compilation, in the situation
where there is almost no point in changing pkg-config.

https://github.com/videolan/vlc/blob/master/contrib/src/main.mak#L77

On the other hand, here, you check for $(HOST)-pkg-config,
mentioning the following text:

# On Debian x86_64-w64-mingw32-pkg-config exists, runs but returns an error when checking packages
ifeq ($(shell unset PKG_CONFIG_LIBDIR; $(HOST)-pkg-config --version 1>/dev/null 2>/dev/null || echo FAIL),)
PKG_CONFIG ?= $(HOST)-pkg-config

But it still in the `ifndef HAVE_CROSS_COMPILE` so you're
basically always setting PKG_CONFIG to pkg-config on
non-cross-compilation builds.

I assume it was supposed to be in `ifdef HAVE_CROSS_COMPILE`
as the value would already be defined anyway so this line
would never assign anything then.

And then when talking albout PKG_CONFIG_LIBDIR:

> In the latter case we force the PKG_CONFIG_LIBDIR and so we don't want to
> use whatever the user provided that might mess with what we're doing.

It feels like saying "the user might provide an invalid pkg-config
executable so prevent him to do that". But then why would you prevent
the user to set CC/CXX for example?

In addition, it means that if there is a $(HOST)-pkg-config in the
path, we could provide an invalid PKG_CONFIG value, but when there
is not it would be forbidden, which is more confusing than expecting
the buildsystem to fail if the user pkg-config is invalid.


Regards,
--
Alexandre Janniaux
Videolabs