Re: [PATCH v2 5/7] artifacts-tar: respect NO_GETTEXT

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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

Hi Ævar,

On Sun, 4 Jul 2021, Ævar Arnfjörð Bjarmason wrote:

> On Sat, Jul 03 2021, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > We obviously do not want to bundle `.mo` files during `make
> > artifacts-tar NO_GETTEXT`, but that was the case.
>
> Should be:
>
>     make artifacts-tar NO_GETTEXT=YesPlease
>
> (Without the =<something> we try to find a "NO_GETTEXT" target)

Correct. Will fix.

> > To fix that, go a step beyond just fixing the symptom, and simply
> > define the lists of `.po` and `.mo` files as empty if `NO_GETTEXT` is
> > set.
>
> How about fixing the cause instead of the symptom...

Yes, from my point of view, I did that.

> > Helped-by: Matthias Aßhauer <mha1993@live.de>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  Makefile | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index c3565fc0f8f..04e852be015 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2675,10 +2675,13 @@ po/git.pot: $(GENERATED_H) FORCE
> >  .PHONY: pot
> >  pot: po/git.pot
> >
> > +ifdef NO_GETTEXT
> > +POFILES :=
> > +MOFILES :=
> > +else
> >  POFILES := $(wildcard po/*.po)
> >  MOFILES := $(patsubst po/%.po,po/build/locale/%/LC_MESSAGES/git.mo,$(POFILES))
> >
> > -ifndef NO_GETTEXT
> >  all:: $(MOFILES)
> >  endif
>
> ...i.e. this patch just seems like odd (ab)use of Makefile logic.
>
> Later on in the artifacts-tar rule we rely on our immediate dependency
> list in $^ to feed to "tar czf", and here we're going to set $(MOFILES)
> to an empty list, just to later interpolate that empty list into that
> list of dependencies.
>
> Wouldn't the mores straightforward thing to do be the diff I've got at
> the end here, perhaps with a preceding commit just for the split-up of
> the dependency list?
>
> This matches how we do things elsewhere, i.e. we don't ifdef e.g. this away:
>
>     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))
>
> rather we keep the list as-is, and ifdef the actual addition of the
> dependency, e.g.:
>
>     ifndef NO_PERL
>     all:: $(LIB_PERL_GEN)
>     [...]
>     endif
>
> One reason we do it like this is because we *don't* want to forget what
> the MOFILES were, because you want e.g. "make clean" to clean them up
> (not that it matters in this case, we rm -rf po/build).

We don't need to be careful about cleaning files we did not generate in
the first place.

Your suggestion amounts to unnecessary work. If we asked for NO_GETTEXT,
why bother generating the list of `.po` files at all? (Rhetorical
question; The answer is "we don't need to".)

> Doesn't matter much here, but following this pattern leads to subtle
> "bugs", e.g. an outstanding issue in your 179227d6e21 (Optionally skip
> linking/copying the built-ins, 2020-09-21) (which I noted on-list in
> passing before, IIRC) where during a build we end up with stale
> built-ins from a previous build in the build directory, because we
> pruned the list during definition time, as opposed to adding an inverse
> "I should remove this then" rule.
>
> ("bug" because it doesn't have any actual effect I know of other than
> bothering me that I have e.g. a git-add in my build-dir still :)
>
> diff --git a/Makefile b/Makefile
> index c3565fc0f8f..7fb1d4b6caa 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3146,9 +3146,18 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
>  OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
>  endif
>
> -artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
> -		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
> -		$(MOFILES)
> +ARTIFACTS_TAR =
> +ARTIFACTS_TAR += GIT-BUILD-OPTIONS
> +ARTIFACTS_TAR += $(ALL_COMMANDS_TO_INSTALL)
> +ARTIFACTS_TAR += $(SCRIPT_LIB)
> +ARTIFACTS_TAR += $(OTHER_PROGRAMS)
> +ARTIFACTS_TAR += $(TEST_PROGRAMS)
> +ARTIFACTS_TAR += $(test_bindir_programs)
> +ifndef NO_GETTEXT
> +ARTIFACTS_TAR += $(MOFILES)
> +endif
> +
> +artifacts-tar:: $(ARTIFACTS_TAR)

Apart from going out of its way to retain the construction of the `.po`
file list (which is totally pointless when operating under `NO_GETTEXT`),
this is also a sore to my eyes. So I won't do that.

Thank you for trying to assist in improving this patch series,
Dscho

>  	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
>  		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
>  	test -n "$(ARTIFACTS_DIRECTORY)"
>
>