{"thread":{"id":"50949","subject":"[PATCH 1/1] configure.ac: Properly check for libintl","startedAt":"2019-04-18T04:54:11Z","lastAt":"2019-04-19T19:37:19Z","messageCount":6,"participants":["Vadim Kochan","Junio C Hamano","Vadym Kochan"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"374086","messageId":"20190418050419.21114-1-vadim4j@gmail.com","threadId":"50949","inReplyTo":null,"subject":"[PATCH 1/1] configure.ac: Properly check for libintl","fromName":"Vadim Kochan","fromEmail":"vadim4j@gmail.com","sentAt":"2019-04-18T05:04:19Z","receivedAt":"2019-04-18T04:54:11Z","isPatch":true,"sender":{"key":"vadim4j@gmail.com","avatar":"https://avatars.githubusercontent.com/u/949597?v=4"},"body":"Some libc implementations like uclibc or musl provides\ngettext stubs via libintl library but this case is not checked\nby AC_CHECK_LIB(c, gettext ...) because gcc has gettext as builtin\nwhich passess the check.\n\nSo check it with included libintl.h where gettext may unfold into\nlibintl_gettext which will cause check to fail if libintl_gettext are\nneeded to be linked with -lintl.\n\nSigned-off-by: Vadim Kochan <vadim4j@gmail.com>\n---\n configure.ac | 16 +++++++++++++---\n 1 file changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/configure.ac b/configure.ac\nindex e0d0da3c0c..be3b55f1cc 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -763,9 +763,19 @@ AC_CHECK_LIB([c], [basename],\n GIT_CONF_SUBST([NEEDS_LIBGEN])\n test -n \"$NEEDS_LIBGEN\" && LIBS=\"$LIBS -lgen\"\n \n-AC_CHECK_LIB([c], [gettext],\n-[LIBC_CONTAINS_LIBINTL=YesPlease],\n-[LIBC_CONTAINS_LIBINTL=])\n+AC_DEFUN([LIBINTL_SRC], [\n+AC_LANG_PROGRAM([[\n+#include <libintl.h>\n+]],[[\n+char *msg = gettext(\"test\");\n+]])])\n+\n+AC_MSG_CHECKING([if libc contains libintl])\n+AC_LINK_IFELSE([LIBINTL_SRC],\n+\t[AC_MSG_RESULT([yes])\n+\tLIBC_CONTAINS_LIBINTL=YesPlease],\n+\t[AC_MSG_RESULT([no])\n+\tLIBC_CONTAINS_LIBINTL=])\n GIT_CONF_SUBST([LIBC_CONTAINS_LIBINTL])\n \n #\n-- \n2.14.1\n\n"},{"id":"374088","messageId":"xmqqftqfhnmz.fsf@gitster-ct.c.googlers.com","threadId":"50949","inReplyTo":"20190418050419.21114-1-vadim4j@gmail.com","subject":"Re: [PATCH 1/1] configure.ac: Properly check for libintl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-18T05:45:56Z","receivedAt":"2019-04-18T05:46:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vadim Kochan <vadim4j@gmail.com> writes:\n\n> Some libc implementations like uclibc or musl provides\n> gettext stubs via libintl library but this case is not checked\n> by AC_CHECK_LIB(c, gettext ...) because gcc has gettext as builtin\n> which passess the check.\n>\n> So check it with included libintl.h where gettext may unfold into\n> libintl_gettext which will cause check to fail if libintl_gettext are\n> needed to be linked with -lintl.\n\nLet me make sure I can understand the above correctly (otherwise,\nthat is a sign that the proposed log message is lacking) by trying\nto rephrase (iow, this is not me saying \"your log message should be\nrewritten like so\"; it is \"if I read what you wrote above correctly,\nI think what I am going to write here is also correct\"):\n\n    Some libc implementations have function called gettext() that\n    can be linked via -lc without -lintl, but these are mere stubs\n    and do not do useful i18n.  On these systems, if a program that\n    calls gettext() is built _with_ \"#include <libintl.h>\", the\n    linker calls for the real version (e.g. libintl_gettext()) and\n    that can be satisfied only by linking with -lintl.\n\n    The current check to see if -lc provides with gettext() is\n    sufficient for libc implementations like GNU libc that actually\n    has full fledged gettext(); to detect libc with stub gettext()\n    and libintl with real gettext(), aliased via <libintl.h>, the\n    check to see if -lintl is necessary must be done with a sample\n    source that #include's the header file.\n\nIs that what is going on and why this patch is needed?\n\nI think the only possibile kind of system this change could break\nthat currently is working is the one that has a usable gettext()\nin -lc, but does not offer <libintl.h>, as the new test program\nadded by this patch will fail to compile, but I do not think that\nis possible in practice---our own gettext.c #include's <libintl.h>\nso there is no way such a hypothetical system that would be broken\nby this change could possibly have built Git successfully.\n\nAssuming that the way I read your log message is in line with what\nyou wanted to say, I think the patch looks good.\n\nThanks.\n\n\n> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>\n> ---\n>  configure.ac | 16 +++++++++++++---\n>  1 file changed, 13 insertions(+), 3 deletions(-)\n>\n> diff --git a/configure.ac b/configure.ac\n> index e0d0da3c0c..be3b55f1cc 100644\n> --- a/configure.ac\n> +++ b/configure.ac\n> @@ -763,9 +763,19 @@ AC_CHECK_LIB([c], [basename],\n>  GIT_CONF_SUBST([NEEDS_LIBGEN])\n>  test -n \"$NEEDS_LIBGEN\" && LIBS=\"$LIBS -lgen\"\n>  \n> -AC_CHECK_LIB([c], [gettext],\n> -[LIBC_CONTAINS_LIBINTL=YesPlease],\n> -[LIBC_CONTAINS_LIBINTL=])\n> +AC_DEFUN([LIBINTL_SRC], [\n> +AC_LANG_PROGRAM([[\n> +#include <libintl.h>\n> +]],[[\n> +char *msg = gettext(\"test\");\n> +]])])\n> +\n> +AC_MSG_CHECKING([if libc contains libintl])\n> +AC_LINK_IFELSE([LIBINTL_SRC],\n> +\t[AC_MSG_RESULT([yes])\n> +\tLIBC_CONTAINS_LIBINTL=YesPlease],\n> +\t[AC_MSG_RESULT([no])\n> +\tLIBC_CONTAINS_LIBINTL=])\n>  GIT_CONF_SUBST([LIBC_CONTAINS_LIBINTL])\n>  \n>  #\n"},{"id":"374091","messageId":"20190418082844.GA10068@lwo1-lhp-f71841","threadId":"50949","inReplyTo":"xmqqftqfhnmz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] configure.ac: Properly check for libintl","fromName":"Vadym Kochan","fromEmail":"vadim4j@gmail.com","sentAt":"2019-04-18T08:28:44Z","receivedAt":"2019-04-18T08:28:53Z","isPatch":true,"sender":{"key":"vadim4j@gmail.com","avatar":"https://avatars.githubusercontent.com/u/949597?v=4"},"body":"Hi Junio,\n\nOn Thu, Apr 18, 2019 at 02:45:56PM +0900, Junio C Hamano wrote:\n> Vadim Kochan <vadim4j@gmail.com> writes:\n> \n> > Some libc implementations like uclibc or musl provides\n> > gettext stubs via libintl library but this case is not checked\n> > by AC_CHECK_LIB(c, gettext ...) because gcc has gettext as builtin\n> > which passess the check.\n> >\n> > So check it with included libintl.h where gettext may unfold into\n> > libintl_gettext which will cause check to fail if libintl_gettext are\n> > needed to be linked with -lintl.\n> \n> Let me make sure I can understand the above correctly (otherwise,\n> that is a sign that the proposed log message is lacking) by trying\n> to rephrase (iow, this is not me saying \"your log message should be\n> rewritten like so\"; it is \"if I read what you wrote above correctly,\n> I think what I am going to write here is also correct\"):\n> \n>     Some libc implementations have function called gettext() that\n>     can be linked via -lc without -lintl, but these are mere stubs\n>     and do not do useful i18n.  On these systems, if a program that\n>     calls gettext() is built _with_ \"#include <libintl.h>\", the\n>     linker calls for the real version (e.g. libintl_gettext()) and\n>     that can be satisfied only by linking with -lintl.\n> \n>     The current check to see if -lc provides with gettext() is\n>     sufficient for libc implementations like GNU libc that actually\n>     has full fledged gettext(); to detect libc with stub gettext()\n>     and libintl with real gettext(), aliased via <libintl.h>, the\n>     check to see if -lintl is necessary must be done with a sample\n>     source that #include's the header file.\n> \n> Is that what is going on and why this patch is needed?\n> \n> I think the only possibile kind of system this change could break\n> that currently is working is the one that has a usable gettext()\n> in -lc, but does not offer <libintl.h>, as the new test program\n> added by this patch will fail to compile, but I do not think that\n> is possible in practice---our own gettext.c #include's <libintl.h>\n> so there is no way such a hypothetical system that would be broken\n> by this change could possibly have built Git successfully.\n> \n> Assuming that the way I read your log message is in line with what\n> you wanted to say, I think the patch looks good.\n> \n> Thanks.\n\nYes you are correct. 'gettext' even might be defined as libintl_gettext.\nWhen I got build error I checked config.log and I saw that gcc claims\nthat gettext builtin is used, so I think that was the reason why test\npassed for non-glibc. So, if to use <libintl.h> then gettext might be\npreprocessed to libintl_gettext in the test. The original error which I\nwas fixing is:\n\n\thttp://autobuild.buildroot.net/results/8eeac7f7ddd97576eaeb87311bf0988d59d8b132/build-end.log\n\n> \n> \n> > Signed-off-by: Vadim Kochan <vadim4j@gmail.com>\n> > ---\n> >  configure.ac | 16 +++++++++++++---\n> >  1 file changed, 13 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/configure.ac b/configure.ac\n> > index e0d0da3c0c..be3b55f1cc 100644\n> > --- a/configure.ac\n> > +++ b/configure.ac\n> > @@ -763,9 +763,19 @@ AC_CHECK_LIB([c], [basename],\n> >  GIT_CONF_SUBST([NEEDS_LIBGEN])\n> >  test -n \"$NEEDS_LIBGEN\" && LIBS=\"$LIBS -lgen\"\n> >  \n> > -AC_CHECK_LIB([c], [gettext],\n> > -[LIBC_CONTAINS_LIBINTL=YesPlease],\n> > -[LIBC_CONTAINS_LIBINTL=])\n> > +AC_DEFUN([LIBINTL_SRC], [\n> > +AC_LANG_PROGRAM([[\n> > +#include <libintl.h>\n> > +]],[[\n> > +char *msg = gettext(\"test\");\n> > +]])])\n> > +\n> > +AC_MSG_CHECKING([if libc contains libintl])\n> > +AC_LINK_IFELSE([LIBINTL_SRC],\n> > +\t[AC_MSG_RESULT([yes])\n> > +\tLIBC_CONTAINS_LIBINTL=YesPlease],\n> > +\t[AC_MSG_RESULT([no])\n> > +\tLIBC_CONTAINS_LIBINTL=])\n> >  GIT_CONF_SUBST([LIBC_CONTAINS_LIBINTL])\n> >  \n> >  #\n"},{"id":"374149","messageId":"xmqqmukmegof.fsf@gitster-ct.c.googlers.com","threadId":"50949","inReplyTo":"20190418082844.GA10068@lwo1-lhp-f71841","subject":"Re: [PATCH 1/1] configure.ac: Properly check for libintl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-19T04:56:48Z","receivedAt":"2019-04-19T18:49:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vadym Kochan <vadim4j@gmail.com> writes:\n\n>>     Some libc implementations have function called gettext() that\n>>     can be linked via -lc without -lintl, but these are mere stubs\n>>     and do not do useful i18n.  On these systems, if a program that\n>>     calls gettext() is built _with_ \"#include <libintl.h>\", the\n>>     linker calls for the real version (e.g. libintl_gettext()) and\n>>     that can be satisfied only by linking with -lintl.\n>> \n>>     The current check to see if -lc provides with gettext() is\n>>     sufficient for libc implementations like GNU libc that actually\n>>     has full fledged gettext(); to detect libc with stub gettext()\n>>     and libintl with real gettext(), aliased via <libintl.h>, the\n>>     check to see if -lintl is necessary must be done with a sample\n>>     source that #include's the header file.\n>> \n>> Is that what is going on and why this patch is needed?\n>> \n> Yes you are correct. 'gettext' even might be defined as libintl_gettext.\n\nWith this exchange, I was aiming for extracting a more useful title\nfor this patch out of you ;-), and I think I accomplished my goal.\n\n\"Properly\" is fairly a useless adverb in the context of a patch\ntitle, as it does not tell us why we thought the way in which the\nupdated code works is more \"proper\".  In addition, because no code\nis perfect, future developers are bound to find something inproperly\ndone in checking for libintl after this patch gets applied.  It is\nbetter to say the most important thing the change does concisely and\nconcretely.\n\nI think\n\n\tautoconf: #include <libintl.h> when checking for gettext()\n\nis probably a better title.  \n\nTogether with your originally proposed log message, which we now\nknow explains why this inclusion makes a difference sufficiently to\nbe understandable by an average Git developer, the resulting commit\nwill communicate to our future developers the reason why we thought\nthis was a good change clearly.\n\nThanks.\n"},{"id":"374150","messageId":"20190419051249.oiypxcs6hexjdf2d@vkochan-ThinkPad-T470p","threadId":"50949","inReplyTo":"xmqqmukmegof.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] configure.ac: Properly check for libintl","fromName":"Vadim Kochan","fromEmail":"vadim4j@gmail.com","sentAt":"2019-04-19T05:12:49Z","receivedAt":"2019-04-19T18:50:45Z","isPatch":true,"sender":{"key":"vadim4j@gmail.com","avatar":"https://avatars.githubusercontent.com/u/949597?v=4"},"body":"Hi Junio,\n\nOn Fri, Apr 19, 2019 at 01:56:48PM +0900, Junio C Hamano wrote:\n> Vadym Kochan <vadim4j@gmail.com> writes:\n> \n> >>     Some libc implementations have function called gettext() that\n> >>     can be linked via -lc without -lintl, but these are mere stubs\n> >>     and do not do useful i18n.  On these systems, if a program that\n> >>     calls gettext() is built _with_ \"#include <libintl.h>\", the\n> >>     linker calls for the real version (e.g. libintl_gettext()) and\n> >>     that can be satisfied only by linking with -lintl.\n> >> \n> >>     The current check to see if -lc provides with gettext() is\n> >>     sufficient for libc implementations like GNU libc that actually\n> >>     has full fledged gettext(); to detect libc with stub gettext()\n> >>     and libintl with real gettext(), aliased via <libintl.h>, the\n> >>     check to see if -lintl is necessary must be done with a sample\n> >>     source that #include's the header file.\n> >> \n> >> Is that what is going on and why this patch is needed?\n> >> \n> > Yes you are correct. 'gettext' even might be defined as libintl_gettext.\n> \n> With this exchange, I was aiming for extracting a more useful title\n> for this patch out of you ;-), and I think I accomplished my goal.\n> \n> \"Properly\" is fairly a useless adverb in the context of a patch\n> title, as it does not tell us why we thought the way in which the\n> updated code works is more \"proper\".  In addition, because no code\n> is perfect, future developers are bound to find something inproperly\n> done in checking for libintl after this patch gets applied.  It is\n> better to say the most important thing the change does concisely and\n> concretely.\n> \n> I think\n> \n> \tautoconf: #include <libintl.h> when checking for gettext()\n> \n> is probably a better title.  \n> \n> Together with your originally proposed log message, which we now\n> know explains why this inclusion makes a difference sufficiently to\n> be understandable by an average Git developer, the resulting commit\n> will communicate to our future developers the reason why we thought\n> this was a good change clearly.\n> \n> Thanks.\n\nThanks! Should I re-submit patch with title updated v2 ?\n\nRegards,\nVadim Kochan\n"},{"id":"374157","messageId":"xmqqtveucxgu.fsf@gitster-ct.c.googlers.com","threadId":"50949","inReplyTo":"20190419051249.oiypxcs6hexjdf2d@vkochan-ThinkPad-T470p","subject":"Re: [PATCH 1/1] configure.ac: Properly check for libintl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-19T06:37:05Z","receivedAt":"2019-04-19T19:37:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vadim Kochan <vadim4j@gmail.com> writes:\n\n>> I think\n>> \n>> \tautoconf: #include <libintl.h> when checking for gettext()\n>> \n>> is probably a better title.  \n>> \n>> Together with your originally proposed log message, which we now\n>> know explains why this inclusion makes a difference sufficiently to\n>> be understandable by an average Git developer, the resulting commit\n>> will communicate to our future developers the reason why we thought\n>> this was a good change clearly.\n>> \n>> Thanks.\n>\n> Thanks! Should I re-submit patch with title updated v2 ?\n\nYou could if you feel like it, but in a case like this, responding\nwith \"Yeah, I agree with your suggestion\" and telling me to retitle\nis sufficient, if you have nothing else to change in the v2.\n\nI've already did the retitling while queuing it, so no need to\nresend.\n\nThanks.\n"}]}