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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jul 05 2021, Johannes Schindelin wrote:

> 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".)

I'm not saying that you in the Windows CI job generated them, but that
in general we want to support doing these in sequence:

    make NO_GETTEXT=Y <target>
    make NO_GETTEXT= <target>

...

>> 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`),


..and that yes, generally speaking there *is* a point in doing
that. E.g. we have another discussion now on-list about incorrectly
spelled/copied config variables in po/*.po files.

It would be a natural thing to want to have some "lint" or "check"
target for that which used $(POFILES) as a source, and you'd not want
that:

    make check-pofiles

To do nothing under NO_GETTEXT=Y, but still use other Makefile
dependencies, e.g. use config-list.h as a source of truth.

To be clear I don't think anything's breaking now with your patch, I
just find the pattern of conflating the declaration of files in the
Makefile with the current logic of the rules that happen to need them
right now to be an anti-pattern.

> this is also a sore to my eyes. So I won't do that.

I agree that converting it is an eyesore, that's quite verbose, but it
makes any later patch much easier to read. You'll just need to add
line(s), not modify that big dependency list in-place.

> 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)"
>>
>>