Merge lp:~stevenk/launchpad/populate-spr-changelogs into lp:launchpad

[Steve Kowalik]

[r=lifeless,wgrant][bug=732334] Add a garbo-hourly job that attempts to set changelogs for SPRs that don't have one set.

Add a garbo-hourly job that looks for Debian SPRs that don't have a changelog set, unpacks package, uploads the debian/changelog file to the Librarian and sets SPR.changelog.

This code is expected to only be temporary, and used as a one-shot. It will be removed once it has been determined that is no longer doing anything useful.

I have cleaned up some lint, along with actually exporting findFile in lp.archiveuploader.dscfile.

[William Grant (wgrant)]

I have a major concern with this: the script was initially written to be a single run, so it doesn't record failures so it can skip them next time. This means that each garbo run will initially go through any import failures and retry them, before getting to anything new. I don't really know how many failures there will be, but it's very possible that it will get into a situation in which it is spending the entirety of each run reattempting old failures, not making any new progress.

Apart from that, some small comments:
 - Why getCandidateSPRIDs instead of getCandidateSPRs (my fault, but your problem now)?
 - Why use the master store? Isn't the slave store good enough for the query (ditto)?
 - Why is there an archive ID hardcoded? 3 is Debian primary, but we want to do it for everything. Even if you think we don't want to do it for everything, hardcoding the ID is the not the Right Way.
 - I'd like additional tests, if only to confirm that the restrictedness check works.

review: Needs Information (code*)

[William Grant (wgrant)]

Looks good, but could you elaborate on the sampledata suckage? It's not at all obvious to anyone.

I'm also tempted to ask you to rename getCandidateSPRs back to getCandidateSPRIDs, since your change to make it return SPRs failed. But I think you might injure me, so I won't force the issue.

review: Approve (code*)

[Robert Collins (lifeless)]

I agree with wgrant on the function name. Also there is a fixture/context manager for temporary directories in fixtures - from fixtures import TempDir. That will be a little more correct than manual management of tempdir lifetime. Both changes are up to you but I heartily commend them to you.

review: Approve

[Robert Collins (lifeless)]

(particularly as your tempdir handling is buggy right now).

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-03-08 16:42:43 +0000
+++ database/schema/security.cfg	2011-03-09 23:47:00 +0000
@@ -2218,6 +2218,9 @@
 public.job                              = SELECT, INSERT, DELETE
 public.branchjob                        = SELECT, DELETE
 public.bugjob                           = SELECT, INSERT
+public.libraryfilealias                 = SELECT, INSERT
+public.libraryfilecontent               = SELECT, INSERT
+public.sourcepackagerelease             = SELECT, UPDATE
 
 [garbo_daily]
 type=user

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2011-02-17 17:02:54 +0000
+++ lib/lp/archiveuploader/dscfile.py	2011-03-09 23:47:00 +0000
@@ -10,11 +10,12 @@
 __metaclass__ = type
 
 __all__ = [
-    'SignableTagFile',
     'DSCFile',
     'DSCUploadedFile',
+    'findFile',
     'find_changelog',
     'find_copyright',
+    'SignableTagFile',
     ]
 
 from cStringIO import StringIO
@@ -28,7 +29,6 @@
 from debian.deb822 import Deb822Dict
 from zope.component import getUtility
 
-from lp.services.encoding import guess as guess_encoding
 from canonical.launchpad.interfaces.gpghandler import (
     GPGVerificationError,
     IGPGHandler,
@@ -64,6 +64,7 @@
     )
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
+from lp.services.encoding import guess as guess_encoding
 from lp.soyuz.enums import (
     ArchivePurpose,
     SourcePackageFormat,

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2011-02-23 10:28:53 +0000
+++ lib/lp/scripts/garbo.py	2011-03-09 23:47:00 +0000
@@ -13,11 +13,18 @@
     datetime,
     timedelta,
     )
+from fixtures import TempDir
+import os
+import signal
+import subprocess
 import time
 
 from psycopg2 import IntegrityError
 import pytz
+from storm.expr import LeftJoin
 from storm.locals import (
+    And,
+    Count,
     Max,
     Min,
     Select,
@@ -43,6 +50,7 @@
 from canonical.launchpad.database.oauth import OAuthNonce
 from canonical.launchpad.database.openidconsumer import OpenIDConsumerNonce
 from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
+from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.utilities.looptuner import TunableLoop
 from canonical.launchpad.webapp.interfaces import (
@@ -50,6 +58,9 @@
     MAIN_STORE,
     MASTER_FLAVOR,
     )
+from canonical.librarian.utils import copy_and_close
+from lp.archiveuploader.dscfile import findFile
+from lp.archiveuploader.nascentuploadfile import UploadError
 from lp.bugs.interfaces.bug import IBugSet
 from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugattachment import BugAttachment
@@ -70,10 +81,13 @@
 from lp.hardwaredb.model.hwdb import HWSubmission
 from lp.registry.model.person import Person
 from lp.services.job.model.job import Job
+from lp.services.memcache.interfaces import IMemcacheClient
 from lp.services.scripts.base import (
     LaunchpadCronScript,
     SilentLaunchpadScriptFailure,
     )
+from lp.soyuz.model.files import SourcePackageReleaseFile
+from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.model.potranslation import POTranslation
 
@@ -81,6 +95,10 @@
 ONE_DAY_IN_SECONDS = 24*60*60
 
 
+def subprocess_setup():
+    signal.signal(signal.SIGPIPE, signal.SIG_DFL)
+
+
 class BulkPruner(TunableLoop):
     """A abstract ITunableLoop base class for simple pruners.
 
@@ -893,6 +911,124 @@
         self.done = True
 
 
+class PopulateSPRChangelogs(TunableLoop):
+    maximum_chunk_size = 1
+
+    def __init__(self, log, abort_time=None):
+        super(PopulateSPRChangelogs, self).__init__(log, abort_time)
+        value = getUtility(IMemcacheClient).get('populate-spr-changelogs')
+        if not value:
+            self.start_at = 0
+        else:
+            self.start_at = value
+        self.finish_at = self.getCandidateSPRs(0).last()
+
+    def getCandidateSPRs(self, start_at):
+        return IMasterStore(SourcePackageRelease).using(
+            SourcePackageRelease,
+            # Find any SPRFs that have expired (LFA.content IS NULL).
+            LeftJoin(
+                SourcePackageReleaseFile,
+                SourcePackageReleaseFile.sourcepackagereleaseID ==
+                    SourcePackageRelease.id),
+            LeftJoin(
+                LibraryFileAlias,
+                And(LibraryFileAlias.id ==
+                    SourcePackageReleaseFile.libraryfileID,
+                    LibraryFileAlias.content == None)),
+            # And exclude any SPRs that have any expired SPRFs.
+            ).find(
+                SourcePackageRelease.id,
+                SourcePackageRelease.id >= start_at,
+                SourcePackageRelease.changelog == None,
+            ).group_by(SourcePackageRelease.id).having(
+                Count(LibraryFileAlias) == 0
+            ).order_by(SourcePackageRelease.id)
+
+    def isDone(self):
+        return self.start_at > self.finish_at
+
+    def __call__(self, chunk_size):
+        for sprid in self.getCandidateSPRs(self.start_at)[:chunk_size]:
+            spr = SourcePackageRelease.get(sprid)
+            with TempDir() as tmp_dir:
+                dsc_file = None
+
+                # Grab the files from the librarian into a temporary
+                # directory.
+                try:
+                    for sprf in spr.files:
+                        dest = os.path.join(
+                            tmp_dir.path, sprf.libraryfile.filename)
+                        dest_file = open(dest, 'w')
+                        sprf.libraryfile.open()
+                        copy_and_close(sprf.libraryfile, dest_file)
+                        if dest.endswith('.dsc'):
+                            dsc_file = dest
+                except LookupError:
+                    self.log.warning(
+                        'SPR %d (%s %s) has missing library files.' % (
+                            spr.id, spr.name, spr.version))
+                    continue
+
+                if dsc_file is None:
+                    self.log.warning(
+                        'SPR %d (%s %s) has no DSC.' % (
+                            spr.id, spr.name, spr.version))
+                    continue
+
+                # Extract the source package. Throw away stdout/stderr
+                # -- we only really care about the return code.
+                fnull = open('/dev/null', 'w')
+                ret = subprocess.call(
+                    ['dpkg-source', '-x', dsc_file, os.path.join(
+                        tmp_dir.path, 'extracted')],
+                        stdout=fnull, stderr=fnull,
+                        preexec_fn=subprocess_setup)
+                fnull.close()
+                if ret != 0:
+                    self.log.warning(
+                        'SPR %d (%s %s) failed to unpack: returned %d' % (
+                            spr.id, spr.name, spr.version, ret))
+                    continue
+
+                # We have an extracted source package. Let's get the
+                # changelog. findFile ensures that it's not too huge, and
+                # not a symlink.
+                try:
+                    changelog_path = findFile(
+                        tmp_dir.path, 'debian/changelog')
+                except UploadError, e:
+                    changelog_path = None
+                    self.log.warning(
+                        'SPR %d (%s %s) changelog could not be '
+                        'imported: %s' % (
+                            spr.id, spr.name, spr.version, e))
+                if changelog_path:
+                    # The LFA should be restricted only if there aren't any
+                    # public publications.
+                    restricted = not any(
+                        not a.private for a in spr.published_archives)
+                    spr.changelog = getUtility(ILibraryFileAliasSet).create(
+                        'changelog',
+                        os.stat(changelog_path).st_size,
+                        open(changelog_path, "r"),
+                        "text/x-debian-source-changelog",
+                        restricted=restricted)
+                    self.log.info('SPR %d (%s %s) changelog imported.' % (
+                        spr.id, spr.name, spr.version))
+                else:
+                    self.log.warning('SPR %d (%s %s) had no changelog.' % (
+                        spr.id, spr.name, spr.version))
+
+        self.start_at = spr.id + 1
+        result = getUtility(IMemcacheClient).set(
+            'populate-spr-changelogs', self.start_at)
+        if not result:
+            self.log.warning('Failed to set start_at in memcache.')
+        transaction.commit()
+
+
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
     script_name = None # Script name for locking and database user. Override.
@@ -977,6 +1113,7 @@
         RevisionCachePruner,
         BugHeatUpdater,
         BugWatchScheduler,
+        PopulateSPRChangelogs,
         ]
     experimental_tunable_loops = []
 

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2011-02-28 11:50:34 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2011-03-09 23:47:00 +0000
@@ -10,7 +10,9 @@
     datetime,
     timedelta,
     )
-import operator
+from fixtures import TempDir
+import os
+import subprocess
 import time
 
 from pytz import UTC
@@ -35,6 +37,7 @@
 from canonical.launchpad.database.oauth import OAuthNonce
 from canonical.launchpad.database.openidconsumer import OpenIDConsumerNonce
 from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
+from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.scripts.tests import run_script
 from canonical.launchpad.webapp.interfaces import (
@@ -48,6 +51,7 @@
     LaunchpadZopelessLayer,
     ZopelessDatabaseLayer,
     )
+from lp.archiveuploader.dscfile import findFile
 from lp.bugs.model.bugnotification import (
     BugNotification,
     BugNotificationRecipient,
@@ -64,6 +68,7 @@
     )
 from lp.code.model.codeimportevent import CodeImportEvent
 from lp.code.model.codeimportresult import CodeImportResult
+from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import (
     IPersonSet,
     PersonCreationRationale,
@@ -76,6 +81,8 @@
     )
 from lp.services.job.model.job import Job
 from lp.services.log.logger import BufferLogger
+from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 from lp.testing import (
     TestCase,
     TestCaseWithFactory,
@@ -95,6 +102,13 @@
 
     def test_hourly_script(self):
         """Ensure garbo-hourly.py actually runs."""
+        # Our sampledata doesn't contain anything that PopulateSPRChangelogs
+        # can process without errors, so it's easier to just set all of the
+        # changelogs to a random LFA. We can't just expire every LFA, since
+        # a bunch of SPRs have no SPRFs at all.
+        IMasterStore(SourcePackageRelease).find(SourcePackageRelease).set(
+            changelogID=1)
+        transaction.commit() # run_script() is a different process.
         rv, out, err = run_script(
             "cronscripts/garbo-hourly.py", ["-q"], expect_returncode=0)
         self.failIf(out.strip(), "Output to stdout: %s" % out)
@@ -513,7 +527,7 @@
         # them will have the present day as its date created, and so will not
         # be deleted, whereas the other will have a creation date far in the
         # past, so it will be deleted.
-        person = self.factory.makePerson(name='test-unlinked-person-new')
+        self.factory.makePerson(name='test-unlinked-person-new')
         person_old = self.factory.makePerson(name='test-unlinked-person-old')
         removeSecurityProxy(person_old).datecreated = datetime(
             2008, 01, 01, tzinfo=UTC)
@@ -541,7 +555,7 @@
             bugID=1,
             is_comment=True,
             date_emailed=None)
-        recipient = BugNotificationRecipient(
+        BugNotificationRecipient(
             bug_notification=notification,
             personID=1,
             reason_header='Whatever',
@@ -555,7 +569,7 @@
                 bugID=1,
                 is_comment=True,
                 date_emailed=UTC_NOW + SQL("interval '%d days'" % delta))
-            recipient = BugNotificationRecipient(
+            BugNotificationRecipient(
                 bug_notification=notification,
                 personID=1,
                 reason_header='Whatever',
@@ -609,7 +623,6 @@
         Store.of(db_branch).flush()
         branch_job = BranchUpgradeJob.create(db_branch)
         branch_job.job.date_finished = THIRTY_DAYS_AGO
-        job_id = branch_job.job.id
 
         self.assertEqual(
             store.find(
@@ -617,7 +630,7 @@
                 BranchJob.branch == db_branch.id).count(),
                 1)
 
-        collector = self.runDaily()
+        self.runDaily()
 
         LaunchpadZopelessLayer.switchDbUser('testadmin')
         self.assertEqual(
@@ -638,21 +651,16 @@
 
         branch_job = BranchUpgradeJob.create(db_branch)
         branch_job.job.date_finished = THIRTY_DAYS_AGO
-        job_id = branch_job.job.id
 
         db_branch2 = self.factory.makeAnyBranch(
             branch_format=BranchFormat.BZR_BRANCH_5,
             repository_format=RepositoryFormat.BZR_KNIT_1)
-        branch_job2 = BranchUpgradeJob.create(db_branch2)
-        job_id_newer = branch_job2.job.id
+        BranchUpgradeJob.create(db_branch2)
 
-        collector = self.runDaily()
+        self.runDaily()
 
         LaunchpadZopelessLayer.switchDbUser('testadmin')
-        self.assertEqual(
-            store.find(
-                BranchJob).count(),
-            1)
+        self.assertEqual(store.find(BranchJob).count(), 1)
 
     def test_ObsoleteBugAttachmentDeleter(self):
         # Bug attachments without a LibraryFileContent record are removed.
@@ -710,3 +718,60 @@
             """ % sqlbase.quote(template.id)).get_one()
 
         self.assertEqual(1, count)
+
+    def upload_to_debian(self, restricted=False):
+        sid = getUtility(IDistributionSet)['debian']['sid']
+        spn = self.factory.makeSourcePackageName('9wm')
+        spr = self.factory.makeSourcePackageRelease(
+            sourcepackagename=spn, version='1.2-7', distroseries=sid)
+        archive = sid.main_archive
+        if restricted:
+            archive = self.factory.makeArchive(
+                distribution=sid.distribution, private=True)
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagerelease=spr, archive=archive,
+            status=PackagePublishingStatus.PUBLISHED)
+        for name in (
+            '9wm_1.2-7.diff.gz', '9wm_1.2.orig.tar.gz', '9wm_1.2-7.dsc'):
+            path = os.path.join(
+                'lib/lp/soyuz/scripts/tests/gina_test_archive/pool/main/9',
+                '9wm', name)
+            lfa = getUtility(ILibraryFileAliasSet).create(
+                name, os.stat(path).st_size, open(path, 'r'),
+                'application/octet-stream', restricted=restricted)
+            spr.addFile(lfa)
+        with TempDir() as tmp_dir:
+            fnull = open('/dev/null', 'w')
+            ret = subprocess.call(
+                ['dpkg-source', '-x', path, os.path.join(
+                    tmp_dir.path, 'extracted')],
+                    stdout=fnull, stderr=fnull)
+            fnull.close()
+            self.assertEqual(0, ret)
+            changelog_path = findFile(tmp_dir.path, 'debian/changelog')
+            changelog = open(changelog_path, 'r').read()
+        transaction.commit() # .runHourly() switches dbuser.
+        return (spr, changelog)
+
+    def test_populateSPRChangelogs(self):
+        # We set SPR.changelog for imported records from Debian.
+        LaunchpadZopelessLayer.switchDbUser('testadmin')
+        spr, changelog = self.upload_to_debian()
+        collector = self.runHourly()
+        log = collector.logger.getLogBuffer()
+        self.assertTrue(
+            'SPR %d (9wm 1.2-7) changelog imported.' % spr.id in log)
+        self.assertFalse(spr.changelog == None)
+        self.assertFalse(spr.changelog.restricted)
+        self.assertEqual(changelog, spr.changelog.read())
+
+    def test_populateSPRChangelogs_restricted_sprf(self):
+        LaunchpadZopelessLayer.switchDbUser('testadmin')
+        spr, changelog = self.upload_to_debian(restricted=True)
+        collector = self.runHourly()
+        log = collector.logger.getLogBuffer()
+        self.assertTrue(
+            'SPR %d (9wm 1.2-7) changelog imported.' % spr.id in log)
+        self.assertFalse(spr.changelog == None)
+        self.assertTrue(spr.changelog.restricted)
+        self.assertEqual(changelog, spr.changelog.read())