search

storaged-project / libbytesize

A tiny library providing a C "class" for working with arbitrary big sizes in bytes
GNU Lesser General Public License v2.1
21 stars 21 forks source link

src/gettext: fix warning if gettext is already present #64

Closed giuliobenetti closed 4 years ago

giuliobenetti commented 4 years ago

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

vojtechtrefny commented 4 years ago

Jenkins, ok to test.

giuliobenetti commented 4 years ago

Ok, I will send a patch upstream ASAIC and then we should bump to latest gettext.h file with my fix included. What do you think?

giuliobenetti commented 4 years ago

Patch is pending upstream: https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00156.html

giuliobenetti commented 4 years ago

Well, on gnulib ML I've realized that the build failure is due to https://github.com/storaged-project/libbytesize/blob/master/configure.ac#L21. If host already has gettext installed, in some sneaky case it can be inderictly included by other header. So I'm going to patch configure.ac to define or not ENABLE_NLS according to host gettext presence. What do you think about it?

giuliobenetti commented 4 years ago

I've reproduced the problem. libintl.h provided by some toolchain includes gettext_noop(). So I close this PR and try this approach https://github.com/storaged-project/libbytesize/pull/64#issuecomment-578833123

giuliobenetti commented 4 years ago

@vojtechtrefny, following this: https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00166.html We ended up to ask uClibc-ng to modify their , that is where their gettext_noop() is defined(gettext_printf() too). But in the meantime this doesn't fix the problem. I see 2 options: 1) use the patch of this PR 2) remove -Werror from CFLAGS

I would prefer solution 1), but please, let me know what you think about this.

Thank you

vojtechtrefny commented 4 years ago

I am ok with merging this patch as workaround. Thank you for investigating this. I'll do a new upstream release today or tomorrow.