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 > > > > 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 = 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 > > Signed-off-by: Johannes Schindelin > > --- > > 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)" > >