dh_translations doesn't strip .desktop files when more than 1 pot target with meson

Attached please find an idea which addresses all the packages mentioned above.

In addition to ignoring help-* domains, it makes dh_translations ignore a hardcoded list of known domains. In case of g-c-c we could get rid of all the workarounds in debian/control, and libgweather would only need to build libgweather-locations.pot from debian/rules (since dh_translations don't build more than one POT file).

Thanks for the patch. I'm of the opinion that this isn't bionic-critical, since these are packages which have never worked with dh_translations + meson, do you agree?

Here's my review:

+ # known domains to be ignored in this context
+ my %ignores = (
+ 'gnome-control-center-2.0-timezones' => 1,
+ 'libgweather-locations' => 1,
+ );

I don't like this - it puts the information in a hard to find place that's remote from the packages it refers to. I think this should be in the rules file for the affected projects, maybe

  $ dh_translations --ignore-domain=gnome-control-center-2.0-timezones

and documented in the manpage. Ideally you'd be able to specify that option multiple times.

+ # delete if found in %ignores or begins with 'help-'
+ for my $d (keys %domains) {
+ delete $domains{$d} if $ignores{$d} or $d =~ /^help-/;
         }

I feel like you could end up accidentally ignoring too many things here. The documentation says

  This also creates two targets for translations help-$project-update-po and help-$project-pot.

So AFAIK we don't need to guess at the domain to ignore since you can do something like

  $ meson introspect . --projectinfo | jq -r '.name'

to get $project.

+ ($domain) = keys %domains;

I'm not a great perl expert, but is this a normal idiom? I guess it gets the first element - is it preferable to using [0]? Just reads a bit confusing to me but if it's normal perl then that's OK.

I've attached a perl file that shows how to get the difference of two arrays with only one map and getting an array out at the end - it might be useful for this bug.

Thanks again!

Thanks for your review!

On 2018-04-11 10:51, Iain Lane wrote:
> Thanks for the patch. I'm of the opinion that this isn't bionic-
> critical, since these are packages which have never worked with
> dh_translations + meson, do you agree?
Not quite. To the extent packages, which dh_translations previously dealt with, have been converted to meson recently, it appears to be a regression. At the same time, for several packages the issue has been worked around for now in debian/rules, so hopefully we have taken care of the most important cases.

> I don't like this - it puts the information in a hard to find place
> that's remote from the packages it refers to. I think this should be
> in the rules file for the affected projects, maybe
>
> $ dh_translations --ignore-domain=gnome-control-center-2.0-timezones
>
> and documented in the manpage. Ideally you'd be able to specify that
> option multiple times.
Yeah, calling dh_translations with options is the other way. To be honest I started to think about that (bug #1762728) but couldn't figure out how to do it. Can you give a hint about how to add an option with arguments to a dh_* script? (I know about @ARGV, but in this case things happen behind the scenes which I don't understand.)

If we figure out how to add an option, I think it would be much easier to state the domain we want instead of a list of domains to ignore:

   $ dh_translations --domain=gnome-control-center-2.0

So as regards your concern about ignoring too much by only looking for 'help-', I'd suggest we leave that for now. With an option as above, we can drop 'the ignore approach'.

> + ($domain) = keys %domains;
>
> I'm not a great perl expert, but is this a normal idiom? I guess it gets
> the first element - is it preferable to using [0]? Just reads a bit
> confusing to me but if it's normal perl then that's OK.
I replaced the array with a hash. [0] is for arrays, not hashes. The parentheses around $domain gives you list context (otherwise you would have got the number of keys, i.e. 1).

> I've attached a perl file that shows how to get the difference of two
> arrays with only one map and getting an array out at the end - it might
> be useful for this bug.
That's a useful tip, thanks. I switched to a hash since I found it easier to delete items that way, but that file indeed shows a nice way to do it also with an array.

On Wed, Apr 11, 2018 at 10:37:11AM -0000, Gunnar Hjalmarsson wrote:
> > I don't like this - it puts the information in a hard to find place
> > that's remote from the packages it refers to. I think this should be
> > in the rules file for the affected projects, maybe
> >
> > $ dh_translations --ignore-domain=gnome-control-center-2.0-timezones
> >
> > and documented in the manpage. Ideally you'd be able to specify that
> > option multiple times.
> Yeah, calling dh_translations with options is the other way. To be honest I started to think about that (bug #1762728) but couldn't figure out how to do it. Can you give a hint about how to add an option with arguments to a dh_* script? (I know about @ARGV, but in this case things happen behind the scenes which I don't understand.)

Sure, you can look at other dh_* for examples of this. For example
dh_strip has a --dbgsym-migration option. See the init() call.

> If we figure out how to add an option, I think it would be much easier
> to state the domain we want instead of a list of domains to ignore:
>
> $ dh_translations --domain=gnome-control-center-2.0
>
> So as regards your concern about ignoring too much by only looking for
> 'help-', I'd suggest we leave that for now. With an option as above, we
> can drop 'the ignore approach'.

It'd be best if dh_translations was able to work automatically most of
the time.

It would appear that a normal case is to have a foo-po and a
help-$project-po target. It should be possible, using the method I
outlined, to handle this out of the box without requiring an override in
d/rules.

For the rest, I suppose it doesn't really matter if you handle outliers
with a --domain or a --ignore option, so I'll leave that bit up to you.

Cheers,

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Thanks Laney! I think I got enough guidance to prepare a patch for this.

I think the attached debdiff changes dh_translations in accordance with the above discussion.

With all the workarounds in debian/rules dropped, g-c-c fails as expected. But with this:

override_dh_translations:
 dh_translations --domain=gnome-control-center-2.0

both the POT creation and the rest work fine.

gnome-calculator creates the POT and tweaks the .desktop file nicely without any overrides.

I took this opportunity to propose that also Icon and Keywords translations are stripped. Hope you agree that that makes sense.

@Laney: I kept using %domains instead of @domains. ;) Given these adjustments I think that a hash is more suitable.

Thanks for the patch!

On Thu, Apr 12, 2018 at 05:57:01PM -0000, Gunnar Hjalmarsson wrote:
> @Laney: I kept using %domains instead of @domains. ;) Given these
> adjustments I think that a hash is more suitable.

Not reviewing yet, but why? If the question is just asking one time
whether a string is in an array, you can use `if (grep { $_ eq string }
@array)' and I think it's clerarer than all of this. And similar for
removing the help domain except using `ne' and assigning the result back
to @domains. My main concern here is keeping the code readable. I don't
think we have to optimise away legibility for 1 or 2 lookups in a tiny
array.

OK, a slight review, it's confusing that --domain only works for meson;
is there any reason not to make that option cover other build systems
too?

Cheers,

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Okay, I'll switch to an array. I'm used to hashes, and personally find it just as readable, but maybe not.. And efficiency is indeed not an issue for this non-runtime code.

On 2018-04-13 10:42, Iain Lane wrote:
> OK, a slight review, it's confusing that --domain only works for
> meson; is there any reason not to make that option cover other build
> systems too?

My thought was that the purpose of adding it was to offer a way to compensate for the ambiguity wrt some Meson packages. So the way it's currently done, you can only set a domain from debian/rules which the introspect command finds. Eliminates the risk that a package maintainer screws it up. ;)

If we'd switch to a true override, where you can set any domain from debian/rules, it would make sense to include other build systems. That might be useful for cases where a gettext domain exists, but the dh_translations code fails to find it for some reason.

Should I change it to a true override, so to say?

In response to Laney's comment #8, and after some clarifications on IRC, attached please find a new attempt.

Comment #6 still applies. In addition to that, g-c-c now fails to build if you add a value via --domain which is unknown to Meson.

As regards non-Meson packages, --domain is a fallback, i.e. using that option makes a difference only for cases where dh_translations fails to find the domain by itself. I haven't tested that detail, since I'm not aware of any package where dh_translations tries and fails to find the domain. The latest issue of the kind I know of was fixed via this change:

http://launchpadlibrarian.net/356105290/pkgbinarymangler_131_132.diff.gz

Big oops. I believe my suggestion in my original bug report was wrong.

I compared gnome-chess and gnome-mines (two simple games in Ubuntu main). gnome-mahjongg in its current release still uses autotools, but gnome-chess uses meson.

The build produces gnome-mahjongg_3.22.0-3_amd64_translations.tar.gz
In there is a gnome-mahjongg.pot in the source/help/ directory.

gnome-chess doesn't have a .pot in its source/help/, but one can be made easily by running the help-gnome-chess-pot target.

Both of these builds are also interesting compared to some of the other packages we've looked at because they produce a second tarball like gnome-mahjongg_3.22.0-3_static_translations.tar.gz containing the pre-built help.

By the Way
==========
We don't actually allow translators to translate the help files for gnome-mahjongg. I don't know how complicated that would be to support.

https://translations.launchpad.net/ubuntu/+source/gnome-mahjongg

Revised suggestion
==================
We should run all the -pot targets offered by meson.

This will do the right thing for all the apps that provide help files.
It will do the right thing for libgweather (which has a second "locations" pot).

I guess we do need to be smart enough to know which one is the main gettext domain though or at least the one to insert into the .desktop files that we strip.

Note about gnome-control-center
==============================
My suggestion will break for gnome-control-center where the gnome-control-center-2.0-timezones-pot meson target isn't runnable. But gnome-control-center is an unusual case. I am guessing that those translations are somehow built in to the gnome-control-center binary.

See https://gitlab.gnome.org/GNOME/gnome-control-center/blob/master/panels/datetime/po-timezones/README

I think the existence of panels/datetime/po-timezones/meson.build is wrong and that file should be removed. When it's removed, we'll no longer have the broken gettext domain meson target.

Bionic
======
Once we fix this bug, I think we should rebuild all the meson apps in main so that the .desktop files will be translatable. We should be able to drop all of our manual dh_translations overrides at the same time (although I believe they are harmless). These rebuilds can be done as SRUs as this isn't an urgent issue because the .desktop files should already be translated by GNOME for popular languages.

Final Note
==========
Gunnar, I apologize for all the time you spent on this bug. I think there is no known reason we would need to specify the gettext domain for either autotools or meson (once we handle gnome-control-center).

Appendix
=========
$ ninja -C obj-x86_64-linux-gnu/ gnome-control-center-2.0-timezones-pot
ninja: Entering directory `obj-x86_64-linux-gnu/'
[0/1] Running external command gnome-control-center-2.0-timezones-pot.
Could not find file POTFILES in gnome-control-center/panels/datetime/po-timezones
FAILED: meson-gnome-control-center-2.0-timezones-pot
/usr/bin/python3 /usr/bin/meson --internal commandrunner gnome-control-center gnome-control-center/obj-x86_64-linux-gnu panels/datetime/po-timezones /usr/bin/python3 /usr/bin/meson /usr/bin/python3 /usr/bin/meson --internal ge...

It's true that dh_translations builds a POT file for the help files in gnome-mahjongg. It's this code in dh_translations:

    # try to build help POT
    if (-e 'help/Makefile') {
        chdir 'help';
        system ('make', 'pot');
        chdir '..';
    }

Can't help wondering why that code is there.

And yes, it would be possible to make dh_translations build help-gnome-mines.pot etc.

But do we want to make use of such help file templates for LP translations? Personally I'd say no. We are not using LP for gnome-user-docs, but rely on the upstream translations. In the light of that, why would we want to spend considerable time with exporting translations and putting them into source for a bunch of packages with only a few help pages each?

And if you agree that we don't want them, why would we let dh_translations build them? To avoid a command in libgweather's debian/rules for building libgweather-locations.pot? ;)

On 2018-04-14 00:45, Jeremy Bicha wrote:
> I guess we do need to be smart enough to know which one is the
> main gettext domain though or at least the one to insert into the
> .desktop files that we strip.

The --domain option serves that purpose, and it's useful whether we build the help file POTs or not. Currently I'm aware of two packages where --domain is needed: g-c-c and libgweather.

Jeremy, I don't think your original suggestion was wrong at all. The latest patch addresses both that and provides a way to pick the right one in case of multiple non-help domains.

( Yes, I'm biased. :) )

It sounds like you're saying that if any translator wants translated help for GNOME apps, they need to do that in GNOME directly. Therefore, why bother building the help pot files since we're just ignoring them anyway right now?

We don't patch the help for GNOME apps (excluding ubuntu-docs), so the factor of Ubuntu-specific strings isn't relevant here.

On 2018-04-14 14:22, Jeremy Bicha wrote:
> It sounds like you're saying that if any translator wants translated
> help for GNOME apps, they need to do that in GNOME directly.

Yes. That's what I already (since 17.10) have told the translators as regards gnome-user-docs, and I think it would be consistent to be able to give them the same message for app specific help pages.

> Therefore, why bother building the help pot files since we're just
> ignoring them anyway right now?
>
> We don't patch the help for GNOME apps (excluding ubuntu-docs), so
> the factor of Ubuntu-specific strings isn't relevant here.

Right, that's the essence in what I wrote. Does it make sense?

I guess I don't have a big problem with it, but we should probably document why we are discarding the help-* domains.

Documenting the reasons makes sense. If it is to be documented in the dh_translations man page, I suppose we should drop the code snippet I included in comment #12 to be consistent.

@Laney, @Sebastien: Any thoughts on this?

Added the last(?) patch, with the following changes:

* Dropped the code for building help-* templates in accordance with the above discussion, and clarified this in the docs.

* Changed the --domain option for non-Meson packages to be an override instead of fallback. The issue reported in bug #1767549 illustrates that dh_translations may pick an incorrect domain, and we should have an easy way to compensate for that.

I've uploaded that now, thanks. Please keep a close eye on Cosmic uploads to watch out for any issues.

This bug was fixed in the package pkgbinarymangler - 139

---------------
pkgbinarymangler (139) cosmic; urgency=medium

  * dh_translations:
    - Prevent domain confusion for Meson packages when a help-* domain
      is present (LP: #1762889).
    - Don't build help-*.pot. Those templates are used to build static
      localized XML pages, and we don't want to create LP templates and
      deal with exported LP translations for such pages anyway.
    - Allow the domain to be passed to dh_translations via an option in
      debian/rules.
    - Strip also Icon and Keywords translations in .desktop files.

 -- Gunnar Hjalmarsson <email address hidden> Mon, 21 May 2018 12:51:24 +0100

Gunnar, this fix assumes that the translation templates will match each other and will match the meson project id.

polari 3.30.1's project id is 'polari'. Its 2 pot targets are polari-pot and help-org.gnome.Polari-pot.

The project id needs to be 'polari' since that determines the tarball name when ninja dist is run (polari-3.30.1.tar.xz).

So the easy workaround for polari is to simply use polari-pot and help-polari-pot

Since this is the only case I've found like this so far, I'm going to go ahead and propose this to the Polari developers. I just wanted to mention this case to you.

Gunnar pointed out to me that this would be a good case for the --domain option that was added. If my proposal upstream is rejected, I'll use the --domain option for Ubuntu's polari package.

I am concerned about the implementation in the package in the queue, which is applying its own heuristics to determine which build system should be used. Why is this not leveraging the debhelper Buildsystem module, which would ensure dh_translations would be consistent with dh(1) - and, in particular, honor --buildsystem passed in debian/rules?

Hi Steve,

Since I'm the formal uploader of this SRU proposal, I'd better say something, even if I think that e.g. Laney and Didier are better prepared to comment on the approach.

For several cycles dh_translations has used its own, pretty simplistic, heuristic to detemine both the gettext domain and the build system. The introduction of Meson added some complexity:

- Not as straightforward to find the domain
- A need to figure out the build directory

So even if the actually used build system could be detected more easily (I'm not able to tell), we'd still need code to figure out the domain and the build directory.

The proposed upload is in substance just a backport of the version of the dh_translations script used in Cosmic, where it has proved to be helpful AFAIK. If there is room for improvements, can it wait until the 19.04 cycle, so we don't delay this SRU?

I guess dh_translations was written by Martin long before dh(1) came in, and that's why it had its own euristic to determine the current build system. This has never been revisited because the maintainance on this package is low.

I agree though that it should have both systems:
- one for dh(1) packages, using that parameters
- then, the fallback for debhelper < 9 packages, cbds and so on. I'm unsure we still have some in main using those, but that's to be checked.

Unfortunately, I don't see that analyze and changes coming as an official item to work on this cycle in the desktop team in the current queue.

The only packages in main that I am aware of that use cdbs and dh-translations are these: (These use dh-translations via gnome-pkg-tools. I've been keeping track because once these are converted to dh, we can drop part of our diff from Debian in the cdbs packaging.):

* gst-plugins-base1.0
* gst-plugins-good1.0
* gstreamer1.0

And here's the full list of packages that build-depend on dh-translations (so not using gnome-pkg-tools). I assume they are using dh but I didn't actully check them.
* a11y-profile-manager
* activity-log-manager
* apport
* aptdaemon
* banshee
* compiz
* geonames
* gnome-shell-extension-gsconnect
* gnome-shell-extension-ubuntu-dock
* gtk+3.0
* ibus
* indicator-application
* indicator-application-gtk2
* indicator-appmenu
* indicator-bluetooth
* indicator-datetime
* indicator-keyboard
* indicator-messages
* indicator-power
* indicator-printers
* indicator-session
* indicator-sound
* language-selector
* lightdm
* network-manager-applet
* pasaffe
* policykit-1-gnome
* software-properties
* ubuntu-kylin-sso-client
* ubuntu-settings
* ubuntu-system-service
* unity
* unity-china-music-scope
* unity-china-photo-scope
* unity-china-video-scope
* unity-lens-applications
* unity-lens-files
* unity-lens-music
* unity-lens-photos
* unity-lens-video
* url-dispatcher
* usb-creator
* ushare
* whoopsie-preferences
* xdiagnose

Oh, I see that gnome.mk is provided directly by cdbs so we'll need to do more investigation to figure out which cdbs-using packages use gnome.mk. There are 67 packages in main using cdbs and around 2380 in total using cdbs. The vast majority of those wouldn't be using Ubuntu language packs but I don't know a good way to search the archive like we can with https://codesearch.debian.net/

Packages like firefox and thunderbird still use cdbs in Ubuntu (those 2 use dh in Debian) and don't directly include gnome.mk but still get their translations stripped for language packs.

Hello Jeremy, or anyone else affected,

Accepted pkgbinarymangler into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/pkgbinarymangler/138.18.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Verified the test case using dh-translations 138.18.04.1 from bionic-proposed.

This bug was fixed in the package pkgbinarymangler - 138.18.04.1

---------------
pkgbinarymangler (138.18.04.1) bionic-proposed; urgency=medium

  [ Gunnar Hjalmarsson ]
  * dh_translations:
    - Prevent domain confusion for Meson packages when a help-* domain
      is present (LP: #1762889).
    - Don't build help-*.pot. Those templates are used to build static
      localized XML pages, and we don't want to create LP templates and
      deal with exported LP translations for such pages anyway.
    - Allow the domain to be passed to dh_translations via an option in
      debian/rules.

  [ Didier Roche ]
  * dh_translations, fix meson being picked even if not used as the build
    system (causing FTBFS on n-m-applet):
    - detect if a ninja build directory was found, otherwise, discard meson
      findings and let other build systems be tested.
    - follow the same code logic for setting meson as preferred, but only
      if domain wasn't found, instead of unconditionally overriding.
    - still prefer po/Makefile as first source
    - let people override it with dh{domain}

 -- Gunnar Hjalmarsson <email address hidden> Mon, 08 Oct 2018 19:12:00 +0200

The verification of the Stable Release Update for pkgbinarymangler has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.