[PATCH] lib/gettext.h: fix warning if gettext is already present

[PATCH] lib/gettext.h: fix warning if gettext is already present

[Giulio Benetti]

Building on an environment where gettext is already present leads to
emitting a warning about gettext_noop() alread defined. And if -Werror
is passed this warning will be treated like an error, so let's #undef
gettext_noop() before #define it.

Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
---
 lib/gettext.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/gettext.h b/lib/gettext.h
index 249668af6..641113e43 100644
--- a/lib/gettext.h
+++ b/lib/gettext.h
@@ -106,6 +106,7 @@
    and other string expressions won't work.
    The macro's expansion is not parenthesized, so that it is suitable as
    initializer for static 'char[]' or 'const char[]' variables.  */
+#undef gettext_noop
 #define gettext_noop(String) String
 
 /* The separator between msgctxt and msgid in a .mo file.  */
-- 
2.20.1

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Bruno Haible]

Hi Giulio,

> Building on an environment where gettext is already present leads to
> emitting a warning about gettext_noop() alread defined.

Where is the first definition of gettext_noop located? In another copy
of gettext.h? In a third-party .h file? Or in the .c file that is being
compiled?

Bruno

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Giulio Benetti]

Hi Bruno,

On 1/26/20 11:12 PM, Bruno Haible wrote:
> Hi Giulio,
> 
>> Building on an environment where gettext is already present leads to
>> emitting a warning about gettext_noop() alread defined.
> 
> Where is the first definition of gettext_noop located? In another copy
> of gettext.h? In a third-party .h file? Or in the .c file that is being
> compiled?

In a entire gettext. In my specific case I'm adding package libbytesize 
[1], where they add src/gettext.h but they define ENABLE_NLS in any case 
[2]. Maybe configure.ac should define it or not according to gettext 
presence... What do you think?

Here I've opened a pull request:
https://github.com/storaged-project/libbytesize/pull/64

The goal is to add libbytesize to Buildroot.

[1]: https://github.com/storaged-project/libbytesize
[2]: 
https://github.com/storaged-project/libbytesize/blob/master/configure.ac#L21

-- 
Giulio Benetti
Benetti Engineering sas

> Bruno
> 
> 

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Giulio Benetti]

On 1/26/20 11:47 PM, Giulio Benetti wrote:
> Hi Bruno,
> 
> On 1/26/20 11:12 PM, Bruno Haible wrote:
>> Hi Giulio,
>>
>>> Building on an environment where gettext is already present leads to
>>> emitting a warning about gettext_noop() alread defined.
>>
>> Where is the first definition of gettext_noop located? In another copy
>> of gettext.h? In a third-party .h file? Or in the .c file that is being
>> compiled?
> 
> In a entire gettext. In my specific case I'm adding package libbytesize

Here ^^^^^^^^^^^^^^^^ I meant:
In a environment where gettext is already installed.

> [1], where they add src/gettext.h but they define ENABLE_NLS in any case
> [2]. Maybe configure.ac should define it or not according to gettext
> presence... What do you think?
> 
> Here I've opened a pull request:
> https://github.com/storaged-project/libbytesize/pull/64
> 
> The goal is to add libbytesize to Buildroot.
> 
> [1]: https://github.com/storaged-project/libbytesize
> [2]:
> https://github.com/storaged-project/libbytesize/blob/master/configure.ac#L21
> 

-- 
Giulio Benetti
Benetti Engineering sas

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Bruno Haible]

Giulio Benetti wrote:
> In my specific case I'm adding package libbytesize 
> [1], where they add src/gettext.h

But libbytesize uses gettext.h as they should: They use the file internally,
without installing it in public locations upon "make install".

Since you reported a redefinition warning regarding 'gettext_noop',
gettext_noop must have been defined as a macro already elsewhere. Where?

> but they define ENABLE_NLS in any case

Defining ENABLE_NLS several times to the same value 1 is a no-brainer.
It does not produce GCC warnings.

Bruno

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Giulio Benetti]

Hi Bruno,

On 1/27/20 2:41 AM, Bruno Haible wrote:
> Giulio Benetti wrote:
>> In my specific case I'm adding package libbytesize
>> [1], where they add src/gettext.h
> 
> But libbytesize uses gettext.h as they should: They use the file internally,
> without installing it in public locations upon "make install".
> 
> Since you reported a redefinition warning regarding 'gettext_noop',
> gettext_noop must have been defined as a macro already elsewhere. Where?

gettext_noop() is defined in environment gettext.h and in 
libbytesize/configure.ac they define:
CFLAGS="${CFLAGS} -DENABLE_NLS"
without taking into account if we already are in an environment where 
gettext is already installed. So I think that this is problem, 
libbytesize should define ENABLE_NLS according to host gettext presence 
otherwise host gettext could be indirectly included(like in my case) and 
expose gettext_noop() and when local "gettext.h" is included and 
ENABLE_NLS is defined(because there are no checks on environment) 
gettext_noop() will be defined twice.

> 
>> but they define ENABLE_NLS in any case
> 
> Defining ENABLE_NLS several times to the same value 1 is a no-brainer.
> It does not produce GCC warnings.

Here I meant that in libbytesize they define ENABLE_NLS in configure.c 
without guarding for host gettext presence. So if you define ENABLE_NLS 
only once, it's enough to see gettext_noop() redefined, since it's 
defined in host.

Anyway gnulib is correct, the problem to be fixed is in libbytesize and 
it's not fixed by the PR I've already open, I need to modify 
configure.ac to emit ENABLE_NLS according to host gettext presence.

Sorry for the noise

Best and kind regards
-- 
Giulio Benetti
Benetti Engineering sas

> Bruno
> 

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Bruno Haible]

Giulio Benetti wrote:
> > Since you reported a redefinition warning regarding 'gettext_noop',
> > gettext_noop must have been defined as a macro already elsewhere. Where?
> 
> gettext_noop() is defined in environment gettext.h

What do you mean by "environment gettext.h"??

The documentation [1] says
  "Do not install the gettext.h file in public locations. Every package
   that needs it should contain a copy of it on its own."

There should not be a <gettext.h> in public include file directories!

> and in 
> libbytesize/configure.ac they define:
> CFLAGS="${CFLAGS} -DENABLE_NLS"
> ...
> libbytesize should define ENABLE_NLS according to host gettext presence 

This is perfectly OK. It implies that their package won't compile if
gettext() is not contained in libc or GNU gettext was not installed.
But it is NOT the cause of the warning that you are seeing.

> the problem to be fixed is in libbytesize and 
> it's not fixed by the PR I've already open, I need to modify 
> configure.ac to emit ENABLE_NLS according to host gettext presence.

No, libbytesize is OK.

> only once, it's enough to see gettext_noop() redefined, since it's 
> defined in host.

The "host" that you are alluding to is the problem.

Bruno

[1] https://www.gnu.org/software/gettext/manual/html_node/lib_002fgettext_002eh.html

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Giulio Benetti]

Hi Bruno,

sorry I'm following too many things in parallel and I've made a mess on 
explaining!

On 1/27/20 6:38 PM, Bruno Haible wrote:
> Giulio Benetti wrote:
>>> Since you reported a redefinition warning regarding 'gettext_noop',
>>> gettext_noop must have been defined as a macro already elsewhere. Where?
>>
>> gettext_noop() is defined in environment gettext.h
> 
> What do you mean by "environment gettext.h"??
> 
> The documentation [1] says
>    "Do not install the gettext.h file in public locations. Every package
>     that needs it should contain a copy of it on its own."
> 
> There should not be a <gettext.h> in public include file directories!

Indeed there is not, there is <libintl.h> and it is part of uclibc:
https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/include/libintl.h

>> and in
>> libbytesize/configure.ac they define:
>> CFLAGS="${CFLAGS} -DENABLE_NLS"
>> ...
>> libbytesize should define ENABLE_NLS according to host gettext presence
> 
> This is perfectly OK. It implies that their package won't compile if
> gettext() is not contained in libc or GNU gettext was not installed.
> But it is NOT the cause of the warning that you are seeing.

AFAIK ENABLE_NLS is emitted in any case, there is not check.
I'm using Buildroot test-pkg and it fails with uClibc-ng that contains 
libintl.h where they already:
#undef gettext_noop
#define gettext_noop()

but in your gettext.h:
#include <libintl.h>
and after:
#define gettext_noop(String) String

without before:
#undef gettext_noop

so you get that gettext_noop() is redefined.

What I was proposing at the beginning was to add:
#undef gettext_noop

the same way uClibc does.

So now libbytesize PR I've opened makes sense.

I've explained myself very bad, sorry, hope now it's clear.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

>> the problem to be fixed is in libbytesize and
>> it's not fixed by the PR I've already open, I need to modify
>> configure.ac to emit ENABLE_NLS according to host gettext presence.
> 
> No, libbytesize is OK.
> 
>> only once, it's enough to see gettext_noop() redefined, since it's
>> defined in host.
> 
> The "host" that you are alluding to is the problem.
> 
> Bruno
> 
> [1] https://www.gnu.org/software/gettext/manual/html_node/lib_002fgettext_002eh.html
> 

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Bruno Haible]

Giulio Benetti wrote:
> > There should not be a <gettext.h> in public include file directories!
> 
> Indeed there is not, there is <libintl.h> and it is part of uclibc:
> https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/include/libintl.h

Ah! Most of the contents of this <libintl.h> is OK. But
  - gettext_noop
  - gettext_printf
don't belong there.

> What I was proposing at the beginning was to add:
> #undef gettext_noop
> 
> the same way uClibc does.

The compiler warning about a redefined macros is meant to catch unintentional
overriding definitions. For example, when a program defines gettext_noop
and afterwards #includes "gettext.h". It is reasonable to keep this warning
in the general case.

Therefore the right action is that the uClibc people remove their definition
of gettext_noop and gettext_printf from this file.

Bruno

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Giulio Benetti]

Hi Bruno,

On 1/27/20 8:29 PM, Bruno Haible wrote:
> Giulio Benetti wrote:
>>> There should not be a <gettext.h> in public include file directories!
>>
>> Indeed there is not, there is <libintl.h> and it is part of uclibc:
>> https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/include/libintl.h
> 
> Ah! Most of the contents of this <libintl.h> is OK. But
>    - gettext_noop
>    - gettext_printf
> don't belong there.

I see that usually glibc #define gettext_noop() in regex_internal.h that 
in order #include libintl.h[1] so, the same usage as gnulib does.

>> What I was proposing at the beginning was to add:
>> #undef gettext_noop
>>
>> the same way uClibc does.
> 
> The compiler warning about a redefined macros is meant to catch unintentional
> overriding definitions. For example, when a program defines gettext_noop
> and afterwards #includes "gettext.h". It is reasonable to keep this warning
> in the general case.

Yes, but we get problems when somebody passes -Werror and this can occur.

> Therefore the right action is that the uClibc people remove their definition
> of gettext_noop and gettext_printf from this file.

uClibc-ng embeds gettext-tiny which has that libintl.h flavour where 
they define gettext_noop() and gettext_printf() [2]. But their purpose 
is exaclty to provide a stub for gettext. So I end up with the 
conclusion that libbytesize has the problem, none of you all(gnulib, 
uClibc-ng, glibc). Basically the problem is that they define 
-DENABLE_NLS in any case without taking into account if the build system 
provide or not a gettext lib(gettext, gettext-tiny), so they enable 
gnulib's gettext.h by default then we get to double #define.

The only fix to do is in libbytesize/configure.ac: append -DENABLE_NLS 
to CFLAGS only if gettext is not found in build system.

In the meanwhile I've added the workaround I've pointed here with this 
patch, but this is not the solution.

Writing this e-mail everything got clear to me :-) so sorry for the 
noise, anyway it was also to give you an answer.

[1]:https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/regex_internal.h;h=6f761c6ed66940b5b3670060200eeb8e42641559;hb=HEAD#l90
[2]:https://github.com/sabotage-linux/gettext-tiny/blob/master/include/libintl.h#L34

Kind regards
-- 
Giulio Benetti
Benetti Engineering sas

Re: [PATCH] lib/gettext.h: fix warning if gettext is already present

[Bruno Haible]

Giulio Benetti wrote:
> > Ah! Most of the contents of this <libintl.h> is OK. But
> >    - gettext_noop
> >    - gettext_printf
> > don't belong there.
> 
> I see that usually glibc #define gettext_noop() in regex_internal.h that 
> in order #include libintl.h[1] so, the same usage as gnulib does.

The big difference is that libintl.h is a public header file (installed in
/usr/include), whereas regex_internal.h is not.

> > Therefore the right action is that the uClibc people remove their definition
> > of gettext_noop and gettext_printf from this file.
> 
> uClibc-ng embeds gettext-tiny which has that libintl.h flavour where 
> they define gettext_noop() and gettext_printf() [2].

Ah. Then your bug report should go to this 'gettext-tiny' project.

> So I end up with the 
> conclusion that libbytesize has the problem, none of you all(gnulib, 
> uClibc-ng, glibc). Basically the problem is that they define 
> -DENABLE_NLS in any case ...

There is no documentation that discourages or forbids defining ENABLE_NLS
always. Therefore libbytesize is right. Please see the more elaborate
explanation in an earlier mail of this thread.

Bruno