Re: [musl] Re: localename: add support for musl libc

[Rich Felker]

On Sun, Feb 25, 2018 at 10:19:06PM +0100, Bruno Haible wrote:
> Hi Rich,
> 
> > Really use of NL_LOCALE_NAME should always be preferred if it's
> > available, since it's a clean public interface for the functionality
> > desired rather than a hack poking at implementation internals. But if
> > you really like poking at internals for other implementations ...
> 
> In a perfect world, what you say would make sense.
> 
> However, not all libc versions that define _NL_LOCALE_NAME also have
> a _NL_LOCALE_NAME that *works* [1]. It's not that I "really like poking
> at internals". It's that I want my code to actually work.

Which ones does it not work on, and what happens when it doesn't work?
If there are libcs that advertise a feature in their headers but don't
actually support it, it sounds like they're the ones that need
special-casing. (I think it's old glibc, or some mismatched version of
glibc headers and .so, in which case it's already special-cased,
right?) But probably it's just a clean failure at runtime (like
returning a null pointer) where you can try something else if it
failed.

> > The comment /* musl */ above is wrong and should not have been added.
> 
> How can you judge that a comment in gnulib code is adequate, when you
> are not familiar with the way gnulib is developed?
> 
> The comment /* musl */ says two things:
>   - If a developer makes changes to these piece of code, they should
> test in on a system with musl libc.
>   - If a developer sees that this code is being compiled/executed on
> a system without musl libc, they should review the #if chain, to
> make sure no mistake was introduced in #ifs.

OK. I think it would have been clearer as:

/* Known to work and used on musl libc. Also present
 * but not always working on [glibc?] so we use the
 * above case... */

If /* musl */ is fine for conveying that to the people who work on
your code, that's one thing, but I still think it's misleading to
others who come across and read it, because this is not an interface
musl invented but rather one we adopted because glibc, and maybe other
systems, already invented something that looked clean & reasonable to
solve a problem that had no other clean solution.

> Now back to my comment that you haven't addressed, regarding lack of
> __MUSL__:
> 
>   If someone else
>   creates a platform that shares the same superficial characteristics
>   (runs on Linux, has  and NL_LOCALE_NAME) but behaves
>   differently, we will accidentally run into the code intended for musl
>   on that platform. Whereas the fallback code (return "" in this case)
>   would be safer: it would make the unit test fail, but it would not
>   lead to a compilation error or to a code dump.

I disagree with the characterization of this code as "intended for
musl". It's possible that this is someone's intent, but it's not a
programming model musl aims to support. Rather I would see it as code
"intended for systems that provide the NL_LOCALE_NAME langinfo item to
query the name of the active locale". Whether musl is such a system is
version-dependent (1.1.17 or later) and easily determined via
configure- or preprocessor-time tests.

As for failure on other systems, it's testable at configure time that
NL_LOCALE_NAME is a macro accepting locale category macros as
arguments and producing an integer result, demonstrating that it won't
produce a compile error. In that case the worst thing that happens,
unless nl_langinfo is buggy, is returning an empty string or some
string that's not the locale name. (I know at one point in the past
musl erroneously returned null for undefined items, so it might be
worth checking for that too; other implementations might also have had
that bug at some point.)

>   And if that platform does not have an identifiying macro either, we
>   really got a problem how to distinguish the two.

I'm not clear what further distinguishing is needed. We've discussed
before and are open to designing an approach to advertise extended
functionality via macros defined in the standard headers -- something
like the _POSIX_* macros unistd.h defines, but for extensions. But not
just an "I am musl, go assume whatever was true in the version of musl
you looked at" macro. But in order for this to move forward there
should really be some interest in collaboration between
implementations (I think glibc would be open to it now, maybe one or
more BSDs too) and input from application programers (the consumers of
the macros) on what would be useful to them.

Rich

Re: localename: add support for musl libc

[Bruno Haible]

Hi Rich,

> Really use of NL_LOCALE_NAME should always be preferred if it's
> available, since it's a clean public interface for the functionality
> desired rather than a hack poking at implementation internals. But if
> you really like poking at internals for other implementations ...

In a perfect world, what you say would make sense.

However, not all libc versions that define _NL_LOCALE_NAME also have
a _NL_LOCALE_NAME that *works* [1]. It's not that I "really like poking
at internals". It's that I want my code to actually work.

> The comment /* musl */ above is wrong and should not have been added.

How can you judge that a comment in gnulib code is adequate, when you
are not familiar with the way gnulib is developed?

The comment /* musl */ says two things:
  - If a developer makes changes to these piece of code, they should
test in on a system with musl libc.
  - If a developer sees that this code is being compiled/executed on
a system without musl libc, they should review the #if chain, to
make sure no mistake was introduced in #ifs.

Now back to my comment that you haven't addressed, regarding lack of
__MUSL__:

  If someone else
  creates a platform that shares the same superficial characteristics
  (runs on Linux, has  and NL_LOCALE_NAME) but behaves
  differently, we will accidentally run into the code intended for musl
  on that platform. Whereas the fallback code (return "" in this case)
  would be safer: it would make the unit test fail, but it would not
  lead to a compilation error or to a code dump.

  And if that platform does not have an identifiying macro either, we
  really got a problem how to distinguish the two.

Bruno

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=10968

Re: localename: add support for musl libc

[Rich Felker]

On Sun, Feb 25, 2018 at 11:17:08AM +0100, Bruno Haible wrote:
> Hi Assaf,
> 
> > > +#  elif defined __linux__ && HAVE_LANGINFO_H && defined NL_LOCALE_NAME
> > > +/* musl libc */
> > 
> > A tiny comment about the comment :)
> > 
> > You wrote "musl libc", but what the "elif defined ..." is something like
> > "linux but not glibc, with langinfo.h" - which could (in theory) be
> > something other than musl-libc.
> 
> Yes, that's it. The refusal of the musl people to define a symbol such
> as __MUSL__ [1] makes it hard to write future-proof code. If someone else

The existence of it would not help futureproof and would promote
writing of non-futureproof code by hardcoding specific assumptions
about a specific version of musl rather than configure-time or
preprocessor-time detection of features.

> creates a platform that shares the same superficial characteristics
> (runs on Linux, has  and NL_LOCALE_NAME) but behaves
> differently, we will accidentally run into the code intended for musl
> on that platform. Whereas the fallback code (return "" in this case)
> would be safer: it would make the unit test fail, but it would not
> lead to a compilation error or to a code dump.
> 
> And if that platform does not have an identifiying macro either, we
> really got a problem how to distinguish the two.

The comment /* musl */ above is wrong and should not have been added.
Really use of NL_LOCALE_NAME should always be preferred if it's
available, since it's a clean public interface for the functionality
desired rather than a hack poking at implementation internals. But if
you really like poking at internals for other implementations, it also
works to leave it as the fallback case after the hardcoded list of
assumptions about particular known platforms. It should just be called
something more reasonable like /* otherwise, use public NL_LOCALE_NAME
interface if the system has it */ instead of /* musl */.

Rich

Re: localename: add support for musl libc

[Bruno Haible]

Hi Assaf,

> Perhaps it's worth expanding the comment to say "glibc not detected,
> assuming this is musl libc" ?

This is the intent of the comment, yes. But we never write it like this,
in gnulib, for brevity. This is the style we use in gnulib:

$ grep -h '^# *if' getloadavg.c fsusage.c mountlist.c | fgrep '/*'
#  if defined (alliant) && defined (i860) /* Alliant FX/2800 */
# if defined (alliant) && defined (i860) /* Alliant FX/2800 */
# if !defined (LDAV_DONE) && defined (HAVE_LIBKSTAT)   /* Solaris <= 2.6 */
# if ! defined LDAV_DONE && defined HAVE_LIBPERFSTAT   /* AIX */
# if !defined (LDAV_DONE) && defined (__NetBSD__)  /* NetBSD < 0.9 */
# if !defined (LDAV_DONE) && defined (NeXT)/* NeXTStep */
# if !defined (LDAV_DONE) && defined (OSF_ALPHA)   /* OSF/1 */
# if ! defined LDAV_DONE && defined __VMS  /* VMS */
# if !defined (LDAV_DONE) && defined (LOAD_AVE_TYPE) /* Including VMS.  */
#if STAT_STATVFS || STAT_STATVFS64 /* POSIX 1003.1-2001 (and later) with XSI */
# if HAVE_SYS_FS_S5PARAM_H  /* Fujitsu UXP/V */
# if HAVE_DUSTAT_H  /* AIX PS/2 */
#ifdef STAT_STATVFS /* POSIX, except pre-2.6.36 glibc/Linux */
#if defined STAT_STATVFS64/* AIX */
#if defined MOUNTED_GETFSSTAT   /* OSF_1 and Darwin1.3.x */
#ifdef MOUNTED_GETMNTENT1   /* 4.3BSD, SunOS, HP-UX, Dynix, Irix.  */
#  if defined _PATH_MOUNTED /* GNU libc  */
#  if defined MNT_MNTTAB/* HP-UX.  */
#  if defined MNTTABNAME/* Dynix.  */
#ifdef MOUNTED_GETMNTINFO   /* 4.4BSD.  */
#ifdef MOUNTED_GETMNTINFO2  /* NetBSD 3.0.  */
#ifdef MOUNTED_GETMNT   /* Ultrix.  */
#ifdef MOUNTED_FS_STAT_DEV  /* BeOS.  */
#ifdef MOUNTED_FREAD/* SVR2.  */
#ifdef MOUNTED_FREAD_FSTYP  /* SVR3.  */
#ifdef MOUNTED_GETMNTENT2   /* SVR4.  */
#ifdef MOUNTED_VMOUNT   /* AIX.  */
#ifdef MOUNTED_INTERIX_STATVFS  /* Interix. */
#ifdef MOUNTED_VMOUNT   /* AIX.  */
#ifdef MOUNTED_GETMNTENT1 /* GNU/Linux, 4.3BSD, SunOS, HP-UX, Dynix, Irix.  */
#ifdef MOUNTED_GETMNTINFO   /* 4.4BSD.  */
#ifdef MOUNTED_GETMNTINFO2  /* NetBSD 3.0.  */
#ifdef MOUNTED_GETMNT   /* Ultrix.  */
#if defined MOUNTED_FS_STAT_DEV /* BeOS */
#if defined MOUNTED_GETFSSTAT   /* __alpha running OSF_1 */
#if defined MOUNTED_FREAD || defined MOUNTED_FREAD_FSTYP /* SVR[23].  */
# ifdef GETFSTYP/* SVR3.  */
# ifdef GETFSTYP/* SVR3.  */
#ifdef MOUNTED_GETMNTTBL/* DolphinOS goes its own way.  */
#ifdef MOUNTED_GETMNTENT2   /* SVR4.  */
#ifdef MOUNTED_VMOUNT   /* AIX.  */

Bruno

Re: localename: add support for musl libc

[Bruno Haible]

Hi Assaf,

> > +#  elif defined __linux__ && HAVE_LANGINFO_H && defined NL_LOCALE_NAME
> > +/* musl libc */
> 
> A tiny comment about the comment :)
> 
> You wrote "musl libc", but what the "elif defined ..." is something like
> "linux but not glibc, with langinfo.h" - which could (in theory) be
> something other than musl-libc.

Yes, that's it. The refusal of the musl people to define a symbol such
as __MUSL__ [1] makes it hard to write future-proof code. If someone else
creates a platform that shares the same superficial characteristics
(runs on Linux, has  and NL_LOCALE_NAME) but behaves
differently, we will accidentally run into the code intended for musl
on that platform. Whereas the fallback code (return "" in this case)
would be safer: it would make the unit test fail, but it would not
lead to a compilation error or to a code dump.

And if that platform does not have an identifiying macro either, we
really got a problem how to distinguish the two.

Bruno

[1] https://wiki.musl-libc.org/faq.html

Re: localename: add support for musl libc

[Assaf Gordon]

Hello Bruno,

On Sat, Feb 24, 2018 at 01:01:07PM +0100, Bruno Haible wrote:
> On Alpine Linux 3.7.0, which uses musl libc, I see this test failure:
[...]
> diff --git a/lib/localename.c b/lib/localename.c
> index 2133cbc..74c8ee0 100644
> --- a/lib/localename.c
> +++ b/lib/localename.c
> @@ -40,7 +40,7 @@
>  # if defined __APPLE__ && defined __MACH__
>  #  include 
>  # endif
> -# if (__GLIBC__ >= 2 && !defined __UCLIBC__) || defined __CYGWIN__
> +# if (__GLIBC__ >= 2 && !defined __UCLIBC__) || (defined __linux__ && 
> HAVE_LANGINFO_H) || defined __CYGWIN__
>  #  include 
>  # endif
>  # if !defined IN_LIBINTL
> @@ -2703,6 +2703,9 @@ gl_locale_name_thread_unsafe (int category, const char 
> *categoryname)
>   nl_langinfo (_NL_LOCALE_NAME (category)).  */
>name = thread_locale->__names[category];
>  return name;
> +#  elif defined __linux__ && HAVE_LANGINFO_H && defined NL_LOCALE_NAME
> +/* musl libc */
> +return nl_langinfo_l (NL_LOCALE_NAME (category), thread_locale);

A tiny comment about the comment :)

You wrote "musl libc", but what the "elif defined ..." is something like
"linux but not glibc, with langinfo.h" - which could (in theory) be
something other than musl-libc.

Specifically, musl people purposefully object to adding any kind of
preprocessor #define value that would enable detection of musl:
 http://www.openwall.com/lists/musl/2013/02/08/9
And in a sense, this is exactly what we're detecting here.

Perhaps it's worth expanding the comment to say "glibc not detected,
assuming this is musl libc" ?

regards,
 -assaf

localename: add support for musl libc

[Bruno Haible]

On Alpine Linux 3.7.0, which uses musl libc, I see this test failure:

FAIL: test-localename
=

../../gltests/test-localename.c:183: assertion 'strcmp (name, "fr_FR.UTF-8") == 
0' failed
FAIL test-localename (exit status: 134)

This patch fixes it.


2018-02-24  Bruno Haible  <br...@clisp.org>

        localename: Add support for musl libc.
* m4/localename.m4 (gl_LOCALENAME): Check for .
* lib/localename.c (gl_locale_name_thread_unsafe): Use NL_LOCALE_NAME
on Linux platforms which define NL_LOCALE_NAME.

diff --git a/lib/localename.c b/lib/localename.c
index 2133cbc..74c8ee0 100644
--- a/lib/localename.c
+++ b/lib/localename.c
@@ -40,7 +40,7 @@
 # if defined __APPLE__ && defined __MACH__
 #  include 
 # endif
-# if (__GLIBC__ >= 2 && !defined __UCLIBC__) || defined __CYGWIN__
+# if (__GLIBC__ >= 2 && !defined __UCLIBC__) || (defined __linux__ && 
HAVE_LANGINFO_H) || defined __CYGWIN__
 #  include 
 # endif
 # if !defined IN_LIBINTL
@@ -2703,6 +2703,9 @@ gl_locale_name_thread_unsafe (int category, const char 
*categoryname)
  nl_langinfo (_NL_LOCALE_NAME (category)).  */
   name = thread_locale->__names[category];
 return name;
+#  elif defined __linux__ && HAVE_LANGINFO_H && defined NL_LOCALE_NAME
+/* musl libc */
+return nl_langinfo_l (NL_LOCALE_NAME (category), thread_locale);
 #  elif (defined __FreeBSD__ || defined __DragonFly__) || (defined __APPLE__ 
&& defined __MACH__)
 /* FreeBSD, Mac OS X */
 int mask;
diff --git a/m4/localename.m4 b/m4/localename.m4
index 0ac4529..a0e1367 100644
--- a/m4/localename.m4
+++ b/m4/localename.m4
@@ -1,4 +1,4 @@
-# localename.m4 serial 2
+# localename.m4 serial 3
 dnl Copyright (C) 2007, 2009-2018 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -8,6 +8,7 @@ AC_DEFUN([gl_LOCALENAME],
 [
   AC_REQUIRE([gt_LC_MESSAGES])
   AC_REQUIRE([gt_INTL_MACOSX])
+  AC_CHECK_HEADERS_ONCE([langinfo.h])
   AC_CHECK_FUNCS([setlocale uselocale])
   dnl Solaris 12 provides getlocalename_l, while Illumos doesn't have
   dnl it nor the equivalent.