launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/ppa-pub-skip into lp:launchpad.

Commit message:
Fix a couple of issues causing deleted PPAs to get their directories recreated on disk.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/ppa-pub-skip/+merge/150264

Fix a couple of issues causing deleted PPAs to get their directories recreated on disk. One or the other fix is sufficient, but one is the sensible fix and the other has been bugging me for years.

 - Calling getPublisher creates the archive directory tree on disk. This is rather counterintuitive, since the returned Publisher object has methods for deleting the archive and other similar things. I moved it to a Publisher.setupArchiveDirs method that's called in the relevant places.

 - Distribution.getPendingPublicationPPAs assumed that a PPA had pending deletions as long as scheduleddeletiondate was unset, when in fact both scheduleddeletiondate and dateremoved need to be unset. It could be argued that scheduleddeletiondate should be set whenever dateremoved is, but sometimes (mostly PPA deletion) the deletion is executed immediately. This should also fix some performance issues in the PPA publisher.
-- 
https://code.launchpad.net/~wgrant/launchpad/ppa-pub-skip/+merge/150264
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/ppa-pub-skip into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py	2012-08-17 11:15:35 +0000
+++ lib/lp/archivepublisher/domination.py	2013-02-25 04:45:26 +0000
@@ -517,6 +517,7 @@
             binarypackagepublishinghistory.distroarchseries =
                 distroarchseries.id AND
             binarypackagepublishinghistory.scheduleddeletiondate IS NULL AND
+            binarypackagepublishinghistory.dateremoved IS NULL AND
             binarypackagepublishinghistory.archive = %s AND
             binarypackagebuild.source_package_release = %s AND
             distroarchseries.distroseries = %s AND
@@ -786,7 +787,8 @@
             sourcepackagepublishinghistory.archive = %s AND
             sourcepackagepublishinghistory.pocket = %s AND
             sourcepackagepublishinghistory.status IN %s AND
-            sourcepackagepublishinghistory.scheduleddeletiondate is NULL
+            sourcepackagepublishinghistory.scheduleddeletiondate is NULL AND
+            sourcepackagepublishinghistory.dateremoved is NULL
             """ % sqlvalues(
                 distroseries, self.archive, pocket,
                 inactive_publishing_status))
@@ -798,7 +800,8 @@
             binarypackagepublishinghistory.archive = %s AND
             binarypackagepublishinghistory.pocket = %s AND
             binarypackagepublishinghistory.status IN %s AND
-            binarypackagepublishinghistory.scheduleddeletiondate is NULL
+            binarypackagepublishinghistory.scheduleddeletiondate is NULL AND
+            binarypackagepublishinghistory.dateremoved is NULL
             """ % sqlvalues(
                 distroseries, self.archive, pocket,
                 inactive_publishing_status),

=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py	2013-02-20 04:02:11 +0000
+++ lib/lp/archivepublisher/publishing.py	2013-02-25 04:45:26 +0000
@@ -96,9 +96,6 @@
     It ensures the given archive location matches the minimal structure
     required.
     """
-    log.debug("Making directories as needed.")
-    pubconf.setupArchiveDirs()
-
     log.debug("Preparing on-disk pool representation.")
     dp = DiskPool(pubconf.poolroot, pubconf.temproot,
                   logging.getLogger("DiskPool"))
@@ -145,8 +142,6 @@
 
     disk_pool = _getDiskPool(pubconf, log)
 
-    _setupHtaccess(archive, pubconf, log)
-
     if distsroot is not None:
         log.debug("Overriding dists root with %s." % distsroot)
         pubconf.distsroot = distsroot
@@ -197,9 +192,6 @@
         self.archive = archive
         self.allowed_suites = allowed_suites
 
-        if not os.path.isdir(config.poolroot):
-            raise ValueError("Root %s is not a directory or does "
-                             "not exist" % config.poolroot)
         self._diskpool = diskpool
 
         if library is None:
@@ -217,6 +209,11 @@
         # This is a set of tuples in the form (distroseries.name, pocket)
         self.release_files_needed = set()
 
+    def setupArchiveDirs(self):
+        self.log.debug("Setting up archive directories.")
+        self._config.setupArchiveDirs()
+        _setupHtaccess(self.archive, self._config, self.log)
+
     def isDirty(self, distroseries, pocket):
         """True if a publication has happened in this release and pocket."""
         return (distroseries.name, pocket) in self.dirty_pockets

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2013-02-20 04:02:11 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2013-02-25 04:45:26 +0000
@@ -167,6 +167,7 @@
         dead_bpph.supersede()
 
         publisher = getPublisher(test_archive, None, self.logger)
+        publisher.setupArchiveDirs()
 
         self.assertTrue(os.path.exists(publisher._config.archiveroot))
 
@@ -212,6 +213,7 @@
             purpose=ArchivePurpose.PPA)
         logger = BufferLogger()
         publisher = getPublisher(test_archive, None, logger)
+        publisher.setupArchiveDirs()
 
         self.assertTrue(os.path.exists(publisher._config.archiveroot))
 
@@ -1134,7 +1136,7 @@
 
         # Set up the publisher for it and publish its repository.
         # 'getPublisher' is what actually configures the htaccess file.
-        getPublisher(ppa, [], self.logger)
+        getPublisher(ppa, [], self.logger).setupArchiveDirs()
         pubconf = getPubConfig(ppa)
         htaccess_path = os.path.join(pubconf.htaccessroot, ".htaccess")
         self.assertTrue(os.path.exists(htaccess_path))

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2013-02-06 09:15:36 +0000
+++ lib/lp/registry/model/distribution.py	2013-02-25 04:45:26 +0000
@@ -1275,7 +1275,8 @@
         Archive.purpose = %s AND
         Archive.distribution = %s AND
         SourcePackagePublishingHistory.archive = archive.id AND
-        SourcePackagePublishingHistory.scheduleddeletiondate is null AND
+        SourcePackagePublishingHistory.scheduleddeletiondate IS NULL AND
+        SourcePackagePublishingHistory.dateremoved IS NULL AND
         SourcePackagePublishingHistory.status IN (%s, %s)
          """ % sqlvalues(ArchivePurpose.PPA, self,
                          PackagePublishingStatus.PENDING,
@@ -1289,7 +1290,8 @@
         Archive.purpose = %s AND
         Archive.distribution = %s AND
         BinaryPackagePublishingHistory.archive = archive.id AND
-        BinaryPackagePublishingHistory.scheduleddeletiondate is null AND
+        BinaryPackagePublishingHistory.scheduleddeletiondate IS NULL AND
+        BinaryPackagePublishingHistory.dateremoved IS NULL AND
         BinaryPackagePublishingHistory.status IN (%s, %s)
         """ % sqlvalues(ArchivePurpose.PPA, self,
                         PackagePublishingStatus.PENDING,

=== modified file 'lib/lp/soyuz/doc/distribution.txt'
--- lib/lp/soyuz/doc/distribution.txt	2012-12-05 16:16:20 +0000
+++ lib/lp/soyuz/doc/distribution.txt	2013-02-25 04:45:26 +0000
@@ -236,11 +236,20 @@
     >>> pending_ppa.id == cprov.archive.id
     True
 
+If scheduleddeletiondate or dateremoved are set then the PPA is no
+longer pending. process-death-row will do the rest.
+
     >>> login('mark@xxxxxxxxxxx')
     >>> from lp.services.database.constants import UTC_NOW
     >>> src_pub.scheduleddeletiondate = UTC_NOW
     >>> ubuntu.getPendingPublicationPPAs().count()
     0
+    >>> src_pub.scheduleddeletiondate = None
+    >>> ubuntu.getPendingPublicationPPAs().count()
+    1
+    >>> src_pub.dateremoved = UTC_NOW
+    >>> ubuntu.getPendingPublicationPPAs().count()
+    0
 
 A binary pending publication also moves a PPA to the pending-publication
 state. In order to test this behaviour we will copy some binaries within
@@ -285,6 +294,12 @@
     >>> bin_pub.scheduleddeletiondate = UTC_NOW
     >>> ubuntu.getPendingPublicationPPAs().count()
     0
+    >>> bin_pub.scheduleddeletiondate = None
+    >>> ubuntu.getPendingPublicationPPAs().count()
+    1
+    >>> bin_pub.dateremoved = UTC_NOW
+    >>> ubuntu.getPendingPublicationPPAs().count()
+    0
 
 
 Distribution Archives

=== modified file 'lib/lp/soyuz/scripts/publishdistro.py'
--- lib/lp/soyuz/scripts/publishdistro.py	2012-06-29 08:40:05 +0000
+++ lib/lp/soyuz/scripts/publishdistro.py	2013-02-25 04:45:26 +0000
@@ -300,6 +300,7 @@
 
         Commits transactions along the way.
         """
+        publisher.setupArchiveDirs()
         publisher.A_publish(self.isCareful(self.options.careful_publishing))
         self.txn.commit()