launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/translationmerger-always-merge-all into lp:launchpad.

Commit message:
TranslationMerger.mergePackagingTemplates now uses POTemplate.getSharingSubsets to determine the templates to merge.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/translationmerger-always-merge-all/+merge/232042

TranslationMerger.mergePackagingTemplates no longer tries to be over-smart. It now just uses the ends of the relationship to determine a sharing subset and then merges all templates in that subset, rather than assuming that the only targets for merging are on the immediate other side of the relationship. We're about to make things more complicated by sharing between distributions.

This doesn't seem to adversely affect performance to any obvious level.

I also made mergePackagingTemplates use mergeAll rather than just mergePOTMsgSets. There's no obvious reason for it to have behaviour different from mergeModifiedTemplates, and that only makes it a bit slower to do work that would have been done on the first POTemplate IObjectModifiedEvent anyway.
-- 
https://code.launchpad.net/~wgrant/launchpad/translationmerger-always-merge-all/+merge/232042
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/translationmerger-always-merge-all into lp:launchpad.

=== modified file 'lib/lp/translations/scripts/tests/test_merge_existing_packagings.py'
--- lib/lp/translations/scripts/tests/test_merge_existing_packagings.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/scripts/tests/test_merge_existing_packagings.py	2014-08-25 08:57:00 +0000
@@ -30,20 +30,19 @@
             with person_logged_in(packaging.owner):
                 packaging.destroySelf()
         job = make_translation_merge_job(self.factory)
-        packaging = self.factory.makePackagingLink(job.productseries,
-                job.sourcepackagename, job.distroseries)
         self.assertEqual(2, count_translations(job))
         transaction.commit()
         retcode, stdout, stderr = run_script(
             'scripts/rosetta/merge-existing-packagings.py', [],
             expect_returncode=0)
         merge_message = 'INFO    Merging %s/%s and %s/%s.\n' % (
-            packaging.productseries.product.name,
-            packaging.productseries.name,
-            packaging.sourcepackagename.name, packaging.distroseries.name)
+            job.productseries.product.name, job.productseries.name,
+            job.sourcepackagename.name, job.distroseries.name)
         self.assertEqual(
             merge_message +
-            'INFO    Deleted POTMsgSets: 1.  TranslationMessages: 1.\n',
+            'INFO    Deleted POTMsgSets: 1.  TranslationMessages: 1.\n'
+            'INFO    Merging template 1/2.\n'
+            'INFO    Merging template 2/2.\n',
             stderr)
         self.assertEqual('', stdout)
         self.assertEqual(1, count_translations(job))

=== modified file 'lib/lp/translations/scripts/tests/test_packaging_translations.py'
--- lib/lp/translations/scripts/tests/test_packaging_translations.py	2013-07-04 07:26:53 +0000
+++ lib/lp/translations/scripts/tests/test_packaging_translations.py	2014-08-25 08:57:00 +0000
@@ -10,7 +10,10 @@
 import transaction
 
 from lp.services.scripts.tests import run_script
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    admin_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import ZopelessAppServerLayer
 from lp.translations.model.translationpackagingjob import TranslationSplitJob
 from lp.translations.tests.test_translationpackagingjob import (
@@ -24,23 +27,36 @@
 
     def test_merge_translations(self):
         job = make_translation_merge_job(self.factory)
-        TranslationSplitJob.create(
-            job.productseries, job.distroseries, job.sourcepackagename)
         transaction.commit()
         retcode, stdout, stderr = run_script(
             'cronscripts/process-job-source.py',
-            ['ITranslationPackagingJobSource'],
-            expect_returncode=0)
+            ['ITranslationPackagingJobSource'], expect_returncode=0)
         matcher = MatchesRegex(dedent("""\
             INFO    Creating lockfile: /var/lock/launchpad-process-job-source-ITranslationPackagingJobSource.lock
             INFO    Running synchronously.
             INFO    Running <.*?TranslationMergeJob.*?> \(ID .*\) in status Waiting
             INFO    Merging .* and .* in Ubuntu Distroseries.*
             INFO    Deleted POTMsgSets: 1.  TranslationMessages: 1.
+            INFO    Merging template 1/2.
+            INFO    Merging template 2/2.
+            INFO    Ran 1 TranslationMergeJob jobs.
+            """))
+        self.assertThat(stderr, matcher)
+        self.assertEqual('', stdout)
+
+        with admin_logged_in():
+            job.distroseries.getSourcePackage(
+                job.sourcepackagename).deletePackaging()
+        transaction.commit()
+        retcode, stdout, stderr = run_script(
+            'cronscripts/process-job-source.py',
+            ['ITranslationPackagingJobSource'], expect_returncode=0)
+        matcher = MatchesRegex(dedent("""\
+            INFO    Creating lockfile: /var/lock/launchpad-process-job-source-ITranslationPackagingJobSource.lock
+            INFO    Running synchronously.
             INFO    Running <.*?TranslationSplitJob.*?> \(ID .*\) in status Waiting
             INFO    Splitting .* and .* in Ubuntu Distroseries.*
             INFO    1 entries split.
-            INFO    Ran 1 TranslationMergeJob jobs.
             INFO    Ran 1 TranslationSplitJob jobs.
             """))
         self.assertThat(stderr, matcher)

=== modified file 'lib/lp/translations/tests/test_translationpackagingjob.py'
--- lib/lp/translations/tests/test_translationpackagingjob.py	2012-12-26 01:32:19 +0000
+++ lib/lp/translations/tests/test_translationpackagingjob.py	2014-08-25 08:57:00 +0000
@@ -8,11 +8,13 @@
 
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
+from storm.expr import Desc
 import transaction
 from zope.component import getUtility
 from zope.event import notify
 
 from lp.registry.interfaces.packaging import IPackagingUtil
+from lp.services.database.interfaces import IStore
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import (
     IRunnableJob,
@@ -74,9 +76,17 @@
     productseries = upstream_pofile.potemplate.productseries
     distroseries = package_pofile.potemplate.distroseries
     sourcepackagename = package_pofile.potemplate.sourcepackagename
-    return TranslationMergeJob.create(
-        productseries=productseries, distroseries=distroseries,
-        sourcepackagename=sourcepackagename)
+    factory.makePackagingLink(
+        distroseries=distroseries, sourcepackagename=sourcepackagename,
+        productseries=productseries)
+    # Creating the Packaging will have created a merge job too. Return
+    # it.
+    return TranslationPackagingJob.makeSubclass(
+        IStore(TranslationSharingJob).find(
+            TranslationSharingJob,
+            distroseries=distroseries, sourcepackagename=sourcepackagename,
+            productseries=productseries).order_by(
+                Desc(TranslationSharingJob.id)).first())
 
 
 def get_msg_sets(productseries=None, distroseries=None,

=== modified file 'lib/lp/translations/translationmerger.py'
--- lib/lp/translations/translationmerger.py	2013-10-21 06:53:25 +0000
+++ lib/lp/translations/translationmerger.py	2014-08-25 08:57:00 +0000
@@ -376,18 +376,10 @@
     @classmethod
     def mergePackagingTemplates(cls, productseries, sourcepackagename,
                                 distroseries, tm):
-        template_map = dict()
-        all_templates = list(POTemplateSubset(
-            sourcepackagename=sourcepackagename,
-            distroseries=distroseries))
-        all_templates.extend(POTemplateSubset(
-            productseries=productseries))
-        for template in all_templates:
-            template_map.setdefault(template.name, []).append(template)
-        for name, templates in template_map.iteritems():
-            templates.sort(key=POTemplate.sharingKey, reverse=True)
-            merger = cls(templates, tm)
-            merger.mergePOTMsgSets()
+        subset = getUtility(IPOTemplateSet).getSharingSubset(
+            product=productseries.product)
+        for pot in POTemplateSubset(productseries=productseries):
+            cls._mergeTemplates(subset.getSharingPOTemplates(pot.name), tm)
 
     @classmethod
     def mergeModifiedTemplates(cls, potemplate, tm):
@@ -395,8 +387,12 @@
             distribution=potemplate.distribution,
             sourcepackagename=potemplate.sourcepackagename,
             product=potemplate.product)
-        templates = list(subset.getSharingPOTemplates(potemplate.name))
-        templates.sort(key=methodcaller('sharingKey'), reverse=True)
+        cls._mergeTemplates(subset.getSharingPOTemplates(potemplate.name), tm)
+
+    @classmethod
+    def _mergeTemplates(cls, templates, tm):
+        templates = list(sorted(
+            templates, key=methodcaller('sharingKey'), reverse=True))
         merger = cls(templates, tm)
         merger.mergeAll()