git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v2 5/7] artifacts-tar: respect NO_GETTEXT
Date: Sun, 04 Jul 2021 10:30:27 +0200	[thread overview]
Message-ID: <878s2m8ns9.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <c31d2e7f44a8b27210dbde9bc6938ce16a9e0c17.1625347592.git.gitgitgadget@gmail.com>


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)

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

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

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)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 	test -n "$(ARTIFACTS_DIRECTORY)"

  reply	other threads:[~2021-07-04  8:52 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-23 15:24 [PATCH 0/6] ci: speed-up the Windows parts of our GitHub workflow Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 1/6] ci: use the new GitHub Action to download git-sdk-64-minimal Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 2/6] ci (vs-build): use `cmd` to copy the DLLs, not `powershell` Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 3/6] ci: upgrade to using actions/{up,down}load-artifacts v2 Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 4/6] ci(windows): transfer also the Git-tracked files to the test jobs Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 5/6] ci(vs-build): build with NO_GETTEXT Dennis Ameling via GitGitGadget
2021-07-04  8:27   ` Ævar Arnfjörð Bjarmason
2021-07-04  8:52     ` Ævar Arnfjörð Bjarmason
2021-07-13 12:19     ` Johannes Schindelin
2021-07-13 15:11       ` Philip Oakley
2021-07-14  8:51         ` Johannes Schindelin
2021-07-14  7:54       ` Ævar Arnfjörð Bjarmason
2021-06-23 15:24 ` [PATCH 6/6] ci: accelerate the checkout Johannes Schindelin via GitGitGadget
2021-07-01 22:03 ` [PATCH 0/6] ci: speed-up the Windows parts of our GitHub workflow Junio C Hamano
2021-07-03 21:26 ` [PATCH v2 0/7] " Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 1/7] ci: use the new GitHub Action to download git-sdk-64-minimal Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 2/7] ci (vs-build): use `cmd` to copy the DLLs, not `powershell` Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 3/7] ci: upgrade to using actions/{up,down}load-artifacts v2 Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 4/7] ci(windows): transfer also the Git-tracked files to the test jobs Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 5/7] artifacts-tar: respect NO_GETTEXT Johannes Schindelin via GitGitGadget
2021-07-04  8:30     ` Ævar Arnfjörð Bjarmason [this message]
2021-07-04 22:52       ` Johannes Schindelin
2021-07-05  6:33         ` Ævar Arnfjörð Bjarmason
2021-07-03 21:26   ` [PATCH v2 6/7] ci(vs-build): build with NO_GETTEXT Dennis Ameling via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 7/7] ci: accelerate the checkout Johannes Schindelin via GitGitGadget
2021-07-04  8:54     ` Ævar Arnfjörð Bjarmason
2021-07-04 22:37       ` Johannes Schindelin
2021-07-04 22:55   ` [PATCH v3 0/7] ci: speed-up the Windows parts of our GitHub workflow Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 1/7] ci: use the new GitHub Action to download git-sdk-64-minimal Johannes Schindelin via GitGitGadget
2021-07-06 19:16       ` Junio C Hamano
2021-07-04 22:55     ` [PATCH v3 2/7] ci (vs-build): use `cmd` to copy the DLLs, not `powershell` Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 3/7] ci: upgrade to using actions/{up,down}load-artifacts v2 Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 4/7] ci(windows): transfer also the Git-tracked files to the test jobs Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 5/7] artifacts-tar: respect NO_GETTEXT Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 6/7] ci(vs-build): build with NO_GETTEXT Dennis Ameling via GitGitGadget
2021-07-05  6:45       ` Ævar Arnfjörð Bjarmason
2021-07-05 12:44         ` Johannes Schindelin
2021-07-06 19:19       ` Junio C Hamano
2021-07-14  8:47         ` Johannes Schindelin
2021-07-04 22:55     ` [PATCH v3 7/7] ci: accelerate the checkout Johannes Schindelin via GitGitGadget
2021-07-06 23:20     ` [PATCH v3 0/7] ci: speed-up the Windows parts of our GitHub workflow Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=878s2m8ns9.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).