Re: localename: add support for musl libc

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 <langinfo.h> 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 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 <langinfo.h> 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: 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 <langinfo.h> 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