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

[Steve Lhomme]

On 2020-03-19 17:25, Alexandre Janniaux wrote:
> 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.

Nope. The comment gives a hint. The HOST-pkg-config on Debian (dunno 
about others) is broken in intricate ways. Going back to pkg-config is 
the way to go. To do that you just need to tweak the PKG_CONFIG_LIBDIR 
to use the same cross compilation environment HOST-pkg-config would use.

In the end HOST-pkg-config is always pkg-config with some environment 
set for a particular platform. The regular pkg-config can be made to 
perform exactly the same. And that's what this patch does. (it will 
never be universal and match all system specific ways but it's close 
enough).

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

Because the user is probably not aware that we are going to mess with 
the PKG_CONFIG_LIBDIR when (s)he provides a pkg-config. It's usually 
only PKG_CONFIG_PATH that is changed for a particular project.

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

Do you mean we should also test that plain pkg-config is usable too ?