[vlc-devel] [PATCH] configure.ac: check newlocale against android_support

[Alexandre Janniaux]

Hi,

On Fri, May 08, 2020 at 07:11:07PM +0300, Rémi Denis-Courmont wrote:
> Le perjantaina 8. toukokuuta 2020, 14.35.42 EEST Alexandre Janniaux a écrit :
> > Android has uselocale through libandroid_support, which depends on
> > libc++abi for new/delete operators as being a C++ library.
> > ---
> > +dnl Android has uselocale through Android support on API < 21
> > +AC_SEARCH_LIBS([uselocale], [android_support], [
> > +  AS_IF([test "$ac_cv_search_newlocale" = "-landroid_support"], [
> > +    AX_APPEND_FLAG([-landroid_support -lc++abi], [LDFLAGS]) ])
>
> This looks like it belongs in LIBS, not LDFLAGS.

Indeed. there is also a typo newlocale/uselocale which makes
me wonder how it worked in the first time...

>
> But first, is there a benefit to uselocale() on Android? Does it actually work
> for the purpose that VLC uses it?

I think you're right, but I'm not sure if the other fixes
I have are really better or not. They probably are since
they remove a link dependency but it might break the code
more easily.

The longer explanation:

There is support for two locales in bionic: C and en_US.
But bionic doesn't matter here as it doesn't need this so
we can mainly focus on the other case.

android_support only support one (C) locale, which is the
one we actually want in VLC, so uselocale actually always
returns the same value and does nothing on the targets
built with it.

A cleaner way would probably be to be able to remove the
locale handling since we don't care about locale when there
is no support for it, which is more or less what the fixups
are doing.

What this patch is actually fixing is two different issues,
which are:

 + vlc_fixup tries to provide the function but redefine types
   that already exists (locale_t).

 + vlc_fixup tries to redefine uselocale but the prototype is
   defined in locale.h. Autoconf need -landroid_support to be
   able to detect that it's actually implemented.

    /* android locale.h */
    #if __ANDROID_API__ < __ANDROID_API_L__
    struct lconv* localeconv(void);
    locale_t duplocale(locale_t);
    void freelocale(locale_t);
    locale_t newlocale(int, const char*, locale_t);
    locale_t uselocale(locale_t);
    #endif

So it's technically incorrect as it is a workaround instead of
using the correct implementation in fixup, however I tried
enforcing the fixups and prevent including locale.h in those
case, plus not redefining the symbol for C files, which works
for C files.

And then C++ files are compiled and fixup conflicts with them
because C++ headers are transitively including xlocale.h
which declares the locale_t type.

In the end, I end up reverting the following patch without
providing an alternative way for what it is providing for
now locally.

797efbd40743251090d4cfa1db176b2317291507

To sum up, it seems to me that google made it a mess and the
easiest way to solve it without large and complex hacks that
are painful in the codebase is to put all their mess
together.

Would you have any input regarding this?

Regards,
--
Alexandre Janniaux
Videolabs