| Message ID | 20210902085438.54121-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | makefile: teach git about NO_MSGFMT (as supported in GUI and gitk) | expand |
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))
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...
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 > >
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
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
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))
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(-)