GNU bug report logs - #47882
[PATCH] Makefile: Reimplement download-po target

Previous Next

Full log

Message #8 received at 47882 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 47882 <at> debbugs.gnu.org
Subject: Re: bug#47882: [PATCH] Makefile: Reimplement download-po target
Date: Fri, 30 Apr 2021 21:57:05 +0200

Hi!

Julien Lepiller <julien <at> lepiller.eu> skribis:

> The attached patch reimplements the download-po target in Makefile.am.
> The previous implementation was based on the implementation for the TP
> and would use the API of Weblate to get files.
>
> However, the rate-limit is very close to the number of files we need to
> download, which was a big limitation, and this target would only
> download existing files, ignoring new translations.

OK.

> To document the new process:
>
> make download-po
> for any new file in po/guix and po/packages, add the language in a
> single line in po/guix/LINGUAS or po/packages/LINGUAS.
> for any new file in po/doc, add the file name to po/doc/local.mk, in
> DOC_PO_FILES or DOC_COOKBOOK_PO_FILES depending on the translation
> type. Add the texi file name to doc/local.mk too, in info_TEXINFOS.

Alright.  This looks like a nice improvement!  Some nitpicking follows,
but it LGTM overall.

>>From 5c14506d6af24b5307e03604cabf8ca10af56067 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien <at> lepiller.eu>
> Date: Sun, 18 Apr 2021 23:56:48 +0200
> Subject: [PATCH] Makefile: Reimplement `download-po` target.
>
> The weblate API rate limit is very close to the number of files we need
> to download.  The previous implementation did not add new translations.
>
> * Makefile.am (download-po): Update target.
> (make-download-po, make-check-po): Remove functions.

Should be: (make-download-po-rule, make-check-po-rule)

> -	    rm "$(top_srcdir)/$(2)/$(3)$$$$lang.po.tmp" ;			\
> -	    mv "$(top_srcdir)/$(2)/$(3)$$$$lang.po"{.tmp2,} ;			\

That was kinda crazy, I’m glad you divided the number of dollars by
two.  :-)

> +WEBLATE_REPO="https://framagit.org/tyreunom/guix-translations"

Since it’s not shell, rather:

  WEBLATE_REPO = https://framagit.org/tyreunom/guix-translations

BTW, I vaguely remember we asked for a Savannah repo but I’m not sure
what the outcome was; did it eventually stall for some obscure reason?

> +# shallow clone the git repository behind weblate and copy files from it if
> +# they contain at least one translation, and they are well-formed (scheme format
> +# only), warn otherwise.  Copied files are converted to a canonical form.

Please capitalize sentences and proper names.

> +download-po:
> +	dir=$$(mktemp -d); \
> +	git clone --depth 1 "$(WEBLATE_REPO)" "$$dir/translations"; \
> +	for domain in po/doc po/guix po/packages; do \
> +		for po in "$$dir/translations/$$domain"/*.po; do \
> +			translated=$$(LANG=en_US.UTF-8 msgfmt --statistics "$$po" 2>&1 | cut -f1 -d' '); \
> +			target=$$(basename "$$po"); \
> +			target="$$domain/$$target"; \
> +			if msgfmt -c "$$po" && [ "$$translated" != "0" ]; then \
> +				msgfilter --no-wrap -i "$$po" cat > "$$target"; \

Maybe write to $$target.tmp and then do “mv $$target.tmp $$target” so
it’s atomic and we don’t end up with corrupt files if ‘msgfilter’ fails.

I would use spaces rather than tabs to indent the ‘for’ loops.

Thanks!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.