[PATCH] makefile: teach git about NO_MSGFMT (as supported in GUI and gitk)

[PATCH] makefile: teach git about NO_MSGFMT (as supported in GUI and gitk)

[Carlo Marcelo Arenas Belón]

NO_MSGFMT can be used to indicate there is no msgfmt available, so
make git recognize that and avoid failing to build while trying to
generate i18n files.

while at it, refactor the change introduced in 4348824059
(artifacts-tar: respect NO_GETTEXT, 2021-07-04) with something as
functional but shorter.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 Makefile | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 9573190f1d..9f09a75801 100644
--- a/Makefile
+++ b/Makefile
@@ -86,6 +86,8 @@ all::
 # Define LIBC_CONTAINS_LIBINTL if your gettext implementation doesn't
 # need -lintl when linking.
 #
+# Define NO_MSGFMT if you don't have msgfmt
+#
 # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
 # doesn't support GNU extensions like --check and --statistics
 #
@@ -2691,10 +2693,8 @@ po/git.pot: $(GENERATED_H) FORCE
 .PHONY: pot
 pot: po/git.pot
 
-ifdef NO_GETTEXT
-POFILES :=
-MOFILES :=
-else
+ifndef NO_MSGFMT
+ifndef NO_GETTEXT
 POFILES := $(wildcard po/*.po)
 MOFILES := $(patsubst po/%.po,po/build/locale/%/LC_MESSAGES/git.mo,$(POFILES))
 
@@ -2703,6 +2703,7 @@ endif
 
 po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
 	$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
+endif
 
 LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm)
 LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
-- 
2.33.0.481.g26d3bed244

Re: [PATCH] makefile: teach git about NO_MSGFMT (as supported in GUI and gitk)

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 02 2021, Carlo Marcelo Arenas Belón wrote:

> NO_MSGFMT can be used to indicate there is no msgfmt available, so
> make git recognize that and avoid failing to build while trying to
> generate i18n files.

Why would we want to avoid failing the build if we don't have msgfmt?

I understand why you'd want NO_GETTEXT in that case, but what's the
point of building with NO_GETTEXT= NO_MSGFMT=Y?

If we can't build the *.mo files we'll have a completely broken
installation that can't do anything useful with gettext, so why not just
build without gettext at that point?

When this patch is applied a lot of things related to gettext in our
tests fail if you build with NO_GETTEXT= NO_MSGFMT=Y, because those
things are assuming that if NO_GETTEXT isn't defined we'll have the *.mo
files, po/build etc.

> while at it, refactor the change introduced in 4348824059
> (artifacts-tar: respect NO_GETTEXT, 2021-07-04) with something as
> functional but shorter.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  Makefile | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 9573190f1d..9f09a75801 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -86,6 +86,8 @@ all::
>  # Define LIBC_CONTAINS_LIBINTL if your gettext implementation doesn't
>  # need -lintl when linking.
>  #
> +# Define NO_MSGFMT if you don't have msgfmt
> +#
>  # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
>  # doesn't support GNU extensions like --check and --statistics
>  #
> @@ -2691,10 +2693,8 @@ po/git.pot: $(GENERATED_H) FORCE
>  .PHONY: pot
>  pot: po/git.pot
>  
> -ifdef NO_GETTEXT
> -POFILES :=
> -MOFILES :=
> -else
> +ifndef NO_MSGFMT
> +ifndef NO_GETTEXT
>  POFILES := $(wildcard po/*.po)
>  MOFILES := $(patsubst po/%.po,po/build/locale/%/LC_MESSAGES/git.mo,$(POFILES))
>  
> @@ -2703,6 +2703,7 @@ endif
>  
>  po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
>  	$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
> +endif
>  
>  LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm)
>  LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))

Re: [PATCH] makefile: teach git about NO_MSGFMT (as supported in GUI and gitk)

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 02 2021, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Sep 02 2021, Carlo Marcelo Arenas Belón wrote:
>
>> NO_MSGFMT can be used to indicate there is no msgfmt available, so
>> make git recognize that and avoid failing to build while trying to
>> generate i18n files.
>
> Why would we want to avoid failing the build if we don't have msgfmt?
>
> I understand why you'd want NO_GETTEXT in that case, but what's the
> point of building with NO_GETTEXT= NO_MSGFMT=Y?
>
> If we can't build the *.mo files we'll have a completely broken
> installation that can't do anything useful with gettext, so why not just
> build without gettext at that point?
>
> When this patch is applied a lot of things related to gettext in our
> tests fail if you build with NO_GETTEXT= NO_MSGFMT=Y, because those
> things are assuming that if NO_GETTEXT isn't defined we'll have the *.mo
> files, po/build etc.

Some further digging reveals that we've got in-tree git-gui/po/po2msg.sh
and gitk-git/po/po2msg.sh (copy/pasted, but slightly different) that
supports some subset of "msgfmt --tcl".

I.e. those programs with NO_MSGFMT still ends up with locae files they
can use, whereas in this patch we don't end up with anything at all...

Re: [PATCH] makefile: teach git about NO_MSGFMT (as supported in GUI and gitk)

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2147 bytes --]

Hi Carlo,

On Thu, 2 Sep 2021, Carlo Marcelo Arenas Belón wrote:

> NO_MSGFMT can be used to indicate there is no msgfmt available, so
> make git recognize that and avoid failing to build while trying to
> generate i18n files.
>
> while at it, refactor the change introduced in 4348824059
> (artifacts-tar: respect NO_GETTEXT, 2021-07-04) with something as
> functional but shorter.

To me, this commit message is not really related to the diff, which
essentially only adds a code comment and then has the only functional
change that _prevents_ `POFILES` and `MOFILES` from being set to empty
values when `NO_MSGFMT` is set.

I am therefore quite puzzled how that diff is supposed to achieve anything
that is described in the commit message (how does this "make git recognize
that"?).

Could you maybe clarify that?

Thanks,
Dscho

>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  Makefile | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 9573190f1d..9f09a75801 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -86,6 +86,8 @@ all::
>  # Define LIBC_CONTAINS_LIBINTL if your gettext implementation doesn't
>  # need -lintl when linking.
>  #
> +# Define NO_MSGFMT if you don't have msgfmt
> +#
>  # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
>  # doesn't support GNU extensions like --check and --statistics
>  #
> @@ -2691,10 +2693,8 @@ po/git.pot: $(GENERATED_H) FORCE
>  .PHONY: pot
>  pot: po/git.pot
>
> -ifdef NO_GETTEXT
> -POFILES :=
> -MOFILES :=
> -else
> +ifndef NO_MSGFMT
> +ifndef NO_GETTEXT
>  POFILES := $(wildcard po/*.po)
>  MOFILES := $(patsubst po/%.po,po/build/locale/%/LC_MESSAGES/git.mo,$(POFILES))
>
> @@ -2703,6 +2703,7 @@ endif
>
>  po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
>  	$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
> +endif
>
>  LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm)
>  LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
> --
> 2.33.0.481.g26d3bed244
>
>

Re: [PATCH] makefile: teach git about NO_MSGFMT (as supported in GUI and gitk)

[Carlo Arenas]

On Thu, Sep 2, 2021 at 8:32 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Thu, 2 Sep 2021, Carlo Marcelo Arenas Belón wrote:
>
> > NO_MSGFMT can be used to indicate there is no msgfmt available, so
> > make git recognize that and avoid failing to build while trying to
> > generate i18n files.
> >
> > while at it, refactor the change introduced in 4348824059
> > (artifacts-tar: respect NO_GETTEXT, 2021-07-04) with something as
> > functional but shorter.
>
> To me, this commit message is not really related to the diff, which
> essentially only adds a code comment and then has the only functional
> change that _prevents_ `POFILES` and `MOFILES` from being set to empty
> values when `NO_MSGFMT` is set.

correct, except that they should be empty already since nothing has
set them and therefore the end result is the same.
I should have mentioned I tracked back the conversation for this and
even the github issue about it (that I can't find anymore) to make
sure it wouldn't introduce a regression.

< I am therefore quite puzzled how that diff is supposed to achieve anything
> that is described in the commit message (how does this "make git recognize
> that"?).

It does not, which is on the (on an unrelated but touching the same
lines issue), which I am starting to suspect I shouldn't use anymore;
apologies for the noise.

Carlo

Re: [PATCH] makefile: teach git about NO_MSGFMT (as supported in GUI and gitk)

[Carlo Arenas]

OnThu, Sep 2, 2021 at 3:42 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Thu, Sep 02 2021, Ævar Arnfjörð Bjarmason wrote:
> > On Thu, Sep 02 2021, Carlo Marcelo Arenas Belón wrote:
> >> NO_MSGFMT can be used to indicate there is no msgfmt available, so
> >> make git recognize that and avoid failing to build while trying to
> >> generate i18n files.
> >
> > Why would we want to avoid failing the build if we don't have msgfmt?

OK, I have to admit, this is mainly because gettext and msgfmt come
from different packages in OpenBSD, and I keep forgetting to install
the latter.
It makes sense too, if you only care about a quick build and your
binary won't use the message files (ex: a git part of a build
container or a portable minimal package); you are correct I usually do
NO_GETTEXT=1 as well in that case, which is why I didn't saw the tests
breaking.

> > I understand why you'd want NO_GETTEXT in that case, but what's the
> > point of building with NO_GETTEXT= NO_MSGFMT=Y?

I think they are conceptually different, one instructs the build
system that we want to build international support in the binary,
while the other only says "I have this tool and want the catalogs
generated with it"

> > If we can't build the *.mo files we'll have a completely broken
> > installation that can't do anything useful with gettext, so why not just
> > build without gettext at that point?

the gettext() calls fail cleanly if there are no catalogs to use, so I
wouldn't agree it is broken
and more importantly, will start using them if they are provided and
the binary is pointed to them, they don't need to be freshly generated
or even match the version of the binary they are queried from.

> > When this patch is applied a lot of things related to gettext in our
> > tests fail if you build with NO_GETTEXT= NO_MSGFMT=Y, because those
> > things are assuming that if NO_GETTEXT isn't defined we'll have the *.mo
> > files, po/build etc.

apologies, thanks for checking, that was sloppy of me.

> Some further digging reveals that we've got in-tree git-gui/po/po2msg.sh
> and gitk-git/po/po2msg.sh (copy/pasted, but slightly different) that
> supports some subset of "msgfmt --tcl".
>
> I.e. those programs with NO_MSGFMT still end up with locale files they
> can use, whereas in this patch we don't end up with anything at all...

I have to admit I found odd we had those as I was expecting they
should behave like git in this case and don't generate any mo files.

FWIW Git for Windows seems to go to great extents to NOT include those
in the release.

Carlo