We don't want comments to be present in the potfile, unless
they're specifically aimed at translators.

To achieve this, we pass the --add-comments=TRANSLATORS: option
to xgettext. However, we also use the 'glib' preset, which
contains the --add-comments option.

This should work fine, as later options override earlier ones,
but there's a problem: until 0.60, meson would not correctly
preserve the order of options, and so whether or not the potfile
would contain comments we don't care about was entirely down to
chance.

Add a check that will make the test suite fail if unwanted
comments have been added to the potfile.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 build-aux/syntax-check.mk | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 6664763faf..b14e2e57cc 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1503,6 +1503,17 @@ sc_po_check:
 	  rm -f $@-1 $@-2;						\
 	fi
 
+pot_file = $(top_srcdir)/po/libvirt.pot
+
+# Before 0.60, meson would sometimes pass options to xgettext in the
+# wrong order, resulting in unwanted comments showing up in the
+# potfile after it was refreshed
+sc_pot_comments:
+	@if $(GREP) -E '^#\. ' $(pot_file) | $(GREP) -Ev 'TRANSLATORS:'; then \
+	  echo "Spurious comments in $(pot_file)" 1>&1; \
+	  exit 1; \
+	fi
+
 # #if WITH_... will evaluate to false for any non numeric string.
 # That would be flagged by using -Wundef, however gnulib currently
 # tests many undefined macros, and so we can't enable that option.
-- 
2.35.3

On Tue, May 17, 2022 at 10:32:12 +0200, Andrea Bolognani wrote:
> We don't want comments to be present in the potfile, unless
> they're specifically aimed at translators.
> 
> To achieve this, we pass the --add-comments=TRANSLATORS: option
> to xgettext. However, we also use the 'glib' preset, which
> contains the --add-comments option.
> 
> This should work fine, as later options override earlier ones,
> but there's a problem: until 0.60, meson would not correctly
> preserve the order of options, and so whether or not the potfile
> would contain comments we don't care about was entirely down to
> chance.
> 
> Add a check that will make the test suite fail if unwanted
> comments have been added to the potfile.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  build-aux/syntax-check.mk | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
> index 6664763faf..b14e2e57cc 100644
> --- a/build-aux/syntax-check.mk
> +++ b/build-aux/syntax-check.mk
> @@ -1503,6 +1503,17 @@ sc_po_check:
>  	  rm -f $@-1 $@-2;						\
>  	fi
>  
> +pot_file = $(top_srcdir)/po/libvirt.pot
> +
> +# Before 0.60, meson would sometimes pass options to xgettext in the
> +# wrong order, resulting in unwanted comments showing up in the
> +# potfile after it was refreshed
> +sc_pot_comments:
> +	@if $(GREP) -E '^#\. ' $(pot_file) | $(GREP) -Ev 'TRANSLATORS:'; then \
> +	  echo "Spurious comments in $(pot_file)" 1>&1; \
> +	  exit 1; \
> +	fi

Ewww. This doesn't feel like a systemic fix. If meson can't generate
them properly, we should make sure to call the tool properly rahter than
just check whether it's broken.

On Tue, May 17, 2022 at 11:12:47AM +0200, Peter Krempa wrote:
> On Tue, May 17, 2022 at 10:32:12 +0200, Andrea Bolognani wrote:
> > +# Before 0.60, meson would sometimes pass options to xgettext in the
> > +# wrong order, resulting in unwanted comments showing up in the
> > +# potfile after it was refreshed
> > +sc_pot_comments:
> > +	@if $(GREP) -E '^#\. ' $(pot_file) | $(GREP) -Ev 'TRANSLATORS:'; then \
> > +	  echo "Spurious comments in $(pot_file)" 1>&1; \
> > +	  exit 1; \
> > +	fi
>
> Ewww. This doesn't feel like a systemic fix. If meson can't generate
> them properly, we should make sure to call the tool properly rahter than
> just check whether it's broken.

Whether meson generates them properly or not is based on a coin flip,
which is why it took me so damn long to get to the bottom of it :)

meson 0.60 and newer always generate the file correctly. Once all
platforms we target have a new enough version we can decide to drop
the check; in the meantime, having it will prevent further cruft from
sneaking in undetected.

Reimplementing the logic ourselves just because of this bug doesn't
sound like a good idea. It would add significant long-term
maintenance burden.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Tue, May 17, 2022 at 02:23:45AM -0700, Andrea Bolognani wrote:
> On Tue, May 17, 2022 at 11:12:47AM +0200, Peter Krempa wrote:
> > On Tue, May 17, 2022 at 10:32:12 +0200, Andrea Bolognani wrote:
> > > +# Before 0.60, meson would sometimes pass options to xgettext in the
> > > +# wrong order, resulting in unwanted comments showing up in the
> > > +# potfile after it was refreshed
> > > +sc_pot_comments:
> > > +	@if $(GREP) -E '^#\. ' $(pot_file) | $(GREP) -Ev 'TRANSLATORS:'; then \
> > > +	  echo "Spurious comments in $(pot_file)" 1>&1; \
> > > +	  exit 1; \
> > > +	fi
> >
> > Ewww. This doesn't feel like a systemic fix. If meson can't generate
> > them properly, we should make sure to call the tool properly rahter than
> > just check whether it's broken.
> 
> Whether meson generates them properly or not is based on a coin flip,
> which is why it took me so damn long to get to the bottom of it :)
> 
> meson 0.60 and newer always generate the file correctly. Once all
> platforms we target have a new enough version we can decide to drop
> the check; in the meantime, having it will prevent further cruft from
> sneaking in undetected.

Can you say what has changed in the  xgettext command line that meson
is running between the fixed & broken release.  I'm not seing any
obvious difference in git logs for 0.60 that could cause this.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, May 17, 2022 at 10:49:47AM +0100, Daniel P. Berrangé wrote:
> On Tue, May 17, 2022 at 02:23:45AM -0700, Andrea Bolognani wrote:
> > On Tue, May 17, 2022 at 11:12:47AM +0200, Peter Krempa wrote:
> > > On Tue, May 17, 2022 at 10:32:12 +0200, Andrea Bolognani wrote:
> > > > +# Before 0.60, meson would sometimes pass options to xgettext in the
> > > > +# wrong order, resulting in unwanted comments showing up in the
> > > > +# potfile after it was refreshed
> > > > +sc_pot_comments:
> > > > +	@if $(GREP) -E '^#\. ' $(pot_file) | $(GREP) -Ev 'TRANSLATORS:'; then \
> > > > +	  echo "Spurious comments in $(pot_file)" 1>&1; \
> > > > +	  exit 1; \
> > > > +	fi
> > >
> > > Ewww. This doesn't feel like a systemic fix. If meson can't generate
> > > them properly, we should make sure to call the tool properly rahter than
> > > just check whether it's broken.
> >
> > Whether meson generates them properly or not is based on a coin flip,
> > which is why it took me so damn long to get to the bottom of it :)
> >
> > meson 0.60 and newer always generate the file correctly. Once all
> > platforms we target have a new enough version we can decide to drop
> > the check; in the meantime, having it will prevent further cruft from
> > sneaking in undetected.
>
> Can you say what has changed in the  xgettext command line that meson
> is running between the fixed & broken release.  I'm not seing any
> obvious difference in git logs for 0.60 that could cause this.

This is the relevant change:

  https://github.com/mesonbuild/meson/commit/bd2fcb268b9ff48797bebb6a2ef94d2741234191#diff-e9d99e08d876e79e48b22f23823c3fa4b1ba48d852daa2186982a480ffa4e8c3R242

Before 0.60, the xgettext arguments coming from the glib preset were
added to a set along with the ones that we provide ourselves, and so
there was no guaranteed ordering when they were later passed to the
command. The commit above switched from a standard Python set to a
mesonlib.OrderedSet, so the ordering is now guaranteed to be always
the expected one.

-- 
Andrea Bolognani / Red Hat / Virtualization