launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/ppa-metaroot-only-ubuntu into lp:launchpad.

Commit message:
Ignore META_DATA custom uploads for anything except Ubuntu PPAs, as the directory in which we publish them is only defined for Ubuntu PPAs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/ppa-metaroot-only-ubuntu/+merge/225781

META_DATA custom uploads are stored in a separate directory outside the archive root so Ubuntu Software Center can get some data from P3As without accessing the P3A itself. But the metadata hierarchy doesn't include the distribution name, so it conflicts for PPAs with the same owner and name. META_DATA uploads are only used by a few PPAs, and only by USC, so we leave metaroot unset and ignore the uploads for anything except Ubuntu PPAs.
-- 
https://code.launchpad.net/~wgrant/launchpad/ppa-metaroot-only-ubuntu/+merge/225781
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/ppa-metaroot-only-ubuntu into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/config.py'
--- lib/lp/archivepublisher/config.py	2014-06-09 09:32:21 +0000
+++ lib/lp/archivepublisher/config.py	2014-07-07 08:12:52 +0000
@@ -11,6 +11,7 @@
 from zope.component import getUtility
 
 from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
+from lp.registry.interfaces.distribution import IDistributionSet
 from lp.services.config import config
 from lp.soyuz.enums import (
     archive_suffixes,
@@ -91,10 +92,21 @@
     pubconf.poolroot = os.path.join(pubconf.archiveroot, 'pool')
     pubconf.distsroot = os.path.join(pubconf.archiveroot, 'dists')
 
-    meta_root = os.path.join(
-        pubconf.distroroot, archive.owner.name)
-    pubconf.metaroot = os.path.join(
-        meta_root, "meta", archive.name)
+    # META_DATA custom uploads are stored in a separate directory
+    # outside the archive root so Ubuntu Software Center can get some
+    # data from P3As without accessing the P3A itself. But the metadata
+    # hierarchy doesn't include the distribution name, so it conflicts
+    # for PPAs with the same owner and name. META_DATA uploads are only used
+    # by a few PPAs, and only by USC, so we leave metaroot unset and
+    # ignore the uploads for anything except Ubuntu PPAs.
+    ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
+    if archive.is_ppa and archive.distribution == ubuntu:
+        meta_root = os.path.join(
+            pubconf.distroroot, archive.owner.name)
+        pubconf.metaroot = os.path.join(
+            meta_root, "meta", archive.name)
+    else:
+        pubconf.metaroot = None
 
     return pubconf
 

=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py	2014-07-03 04:51:30 +0000
+++ lib/lp/archivepublisher/publishing.py	2014-07-07 08:12:52 +0000
@@ -886,7 +886,7 @@
 
         # XXX wgrant 2014-07-03: Needs checking for multi-distro sanity.
         for directory in (root_dir, self._config.metaroot):
-            if not os.path.exists(directory):
+            if directory is None or not os.path.exists(directory):
                 continue
             try:
                 shutil.rmtree(directory)

=== modified file 'lib/lp/archivepublisher/tests/publishing-meta-data-files.txt'
--- lib/lp/archivepublisher/tests/publishing-meta-data-files.txt	2011-12-22 05:37:22 +0000
+++ lib/lp/archivepublisher/tests/publishing-meta-data-files.txt	2014-07-07 08:12:52 +0000
@@ -18,9 +18,9 @@
     >>> from lp.soyuz.interfaces.publishing import PackagePublishingPocket
     >>> from lp.soyuz.model.queue import PackageUploadCustom
 
-    >>> bat = getUtility(IDistributionSet)['ubuntutest']['breezy-autotest']
-    >>> ppa = factory.makeArchive(distribution=bat.distribution)
-    >>> package_upload = bat.createQueueEntry(
+    >>> hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
+    >>> ppa = factory.makeArchive(distribution=hoary.distribution)
+    >>> package_upload = hoary.createQueueEntry(
     ...     pocket=PackagePublishingPocket.RELEASE, changesfilename="test",
     ...     changesfilecontent="test",
     ...     archive=ppa)
@@ -39,7 +39,7 @@
 Now we can publish the custom upload:
 
     >>> custom_upload.publish(logger=FakeLogger())
-    DEBUG Publishing custom testmeta to ubuntutest/breezy-autotest
+    DEBUG Publishing custom testmeta to ubuntu/hoary
 
 The custom file is just called "testmeta" with the contents "test".  It will
 be published in a location of the scheme:
@@ -63,3 +63,26 @@
 
     >>> print content
     test
+
+The meta-data directory is currently only defined for Ubuntu PPAs, so
+publishing a meta-data upload in a non-Ubuntu PPA is a no-op.
+
+    >>> bat = getUtility(IDistributionSet)['ubuntutest']['breezy-autotest']
+    >>> ppa = factory.makeArchive(distribution=bat.distribution)
+    >>> package_upload = bat.createQueueEntry(
+    ...     pocket=PackagePublishingPocket.RELEASE, changesfilename="test",
+    ...     changesfilecontent="test",
+    ...     archive=ppa)
+    >>> custom_upload = PackageUploadCustom()
+    >>> custom_upload.customformat = PackageUploadCustomFormat.META_DATA
+    >>> custom_upload.packageupload = package_upload
+    >>> custom_upload.libraryfilealias = test_file
+
+    >>> custom_upload.publish(logger=FakeLogger())
+    DEBUG Publishing custom testmeta to ubuntutest/breezy-autotest
+    DEBUG Skipping meta-data for archive without metaroot.
+
+    >>> final_destination = os.path.join(
+    ...     ppa_root, ppa.owner.name, "meta", ppa.name)
+    >>> os.path.exists(final_destination)
+    False

=== modified file 'lib/lp/archivepublisher/tests/test_config.py'
--- lib/lp/archivepublisher/tests/test_config.py	2014-06-09 09:32:21 +0000
+++ lib/lp/archivepublisher/tests/test_config.py	2014-07-07 08:12:52 +0000
@@ -47,6 +47,7 @@
         self.assertEqual(
             self.root + "/ubuntutest-temp", primary_config.temproot)
         self.assertEqual(archiveroot + "-uefi", primary_config.uefiroot)
+        self.assertIs(None, primary_config.metaroot)
 
     def test_partner_config(self):
         # Partner archive configuration is correct.
@@ -68,6 +69,7 @@
         self.assertEqual(
             self.root + "/ubuntutest-temp", partner_config.temproot)
         self.assertEqual(archiveroot + "-uefi", partner_config.uefiroot)
+        self.assertIs(None, partner_config.metaroot)
 
     def test_copy_config(self):
         # In the case of copy archives (used for rebuild testing) the
@@ -88,6 +90,7 @@
         self.assertEqual(archiveroot + "-misc", copy_config.miscroot)
         self.assertEqual(archiveroot + "-temp", copy_config.temproot)
         self.assertIsNone(copy_config.uefiroot)
+        self.assertIs(None, copy_config.metaroot)
 
 
 class TestGetPubConfigPPA(TestCaseWithFactory):
@@ -125,6 +128,7 @@
         uefiroot = "/var/tmp/ppa-signing-keys.test/uefi/%s/%s" % (
             self.ppa.owner.name, self.ppa.name)
         self.assertEqual(uefiroot, self.ppa_config.uefiroot)
+        self.assertIs(None, self.ppa_config.metaroot)
 
     def test_private_ppa_separate_root(self):
         # Private PPAs are published to a different location.
@@ -157,3 +161,18 @@
         uefiroot = "/var/tmp/ppa-signing-keys.test/uefi/%s/%s" % (
             p3a.owner.name, p3a.name)
         self.assertEqual(uefiroot, p3a_config.uefiroot)
+        self.assertIs(None, p3a_config.metaroot)
+
+    def test_metaroot(self):
+        # The metadata directory structure doesn't include a distro
+        # name, and it's only use by Ubuntu Software Center, so only
+        # Ubuntu PPAs have a metaroot.
+        ubuntu_ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        test_ppa = self.factory.makeArchive(
+            distribution=self.ubuntutest, purpose=ArchivePurpose.PPA)
+
+        self.assertEqual(
+            "/var/tmp/ppa.test/%s/meta/%s" % (
+                ubuntu_ppa.owner.name, ubuntu_ppa.name),
+            getPubConfig(ubuntu_ppa).metaroot)
+        self.assertIs(None, getPubConfig(test_ppa).metaroot)

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2013-08-29 10:29:01 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2014-07-07 08:12:52 +0000
@@ -83,6 +83,7 @@
         cprov = getUtility(IPersonSet).getByName('cprov')
         naked_archive = removeSecurityProxy(cprov.archive)
         naked_archive.distribution = self.ubuntutest
+        self.ubuntu = getUtility(IDistributionSet)['ubuntu']
 
 
 class TestPublisher(TestPublisherBase):
@@ -147,7 +148,7 @@
         """Test deleting a PPA"""
         ubuntu_team = getUtility(IPersonSet).getByName('ubuntu-team')
         test_archive = getUtility(IArchiveSet).new(
-            distribution=self.ubuntutest, owner=ubuntu_team,
+            distribution=self.ubuntu, owner=ubuntu_team,
             purpose=ArchivePurpose.PPA, name='testing')
 
         # Create some source and binary publications, including an
@@ -210,7 +211,7 @@
     def testDeletingPPAWithoutMetaData(self):
         ubuntu_team = getUtility(IPersonSet).getByName('ubuntu-team')
         test_archive = getUtility(IArchiveSet).new(
-            distribution=self.ubuntutest, owner=ubuntu_team,
+            distribution=self.ubuntu, owner=ubuntu_team,
             purpose=ArchivePurpose.PPA)
         logger = BufferLogger()
         publisher = getPublisher(test_archive, None, logger)
@@ -230,6 +231,52 @@
         self.assertNotIn('WARNING', logger.getLogBuffer())
         self.assertNotIn('ERROR', logger.getLogBuffer())
 
+    def testDeletingPPAThatCannotHaveMetaData(self):
+        # Due to conflicts in the directory structure only Ubuntu PPAs
+        # have a metadata directory. PPAs with the same name for
+        # different distros can coexist, and only deleting the Ubuntu
+        # one will remove the metadata.
+        ubuntu_team = getUtility(IPersonSet).getByName('ubuntu-team')
+        ubuntu_ppa = getUtility(IArchiveSet).new(
+            distribution=self.ubuntu, owner=ubuntu_team,
+            purpose=ArchivePurpose.PPA, name='ppa')
+        test_ppa = getUtility(IArchiveSet).new(
+            distribution=self.ubuntutest, owner=ubuntu_team,
+            purpose=ArchivePurpose.PPA, name='ppa')
+        logger = BufferLogger()
+        ubuntu_publisher = getPublisher(ubuntu_ppa, None, logger)
+        ubuntu_publisher.setupArchiveDirs()
+        test_publisher = getPublisher(test_ppa, None, logger)
+        test_publisher.setupArchiveDirs()
+
+        self.assertTrue(os.path.exists(ubuntu_publisher._config.archiveroot))
+        self.assertTrue(os.path.exists(test_publisher._config.archiveroot))
+
+        open(os.path.join(
+            ubuntu_publisher._config.archiveroot, 'test_file'), 'w').close()
+        open(os.path.join(
+            test_publisher._config.archiveroot, 'test_file'), 'w').close()
+
+        # Add a meta file for the Ubuntu PPA
+        os.makedirs(ubuntu_publisher._config.metaroot)
+        open(os.path.join(
+            ubuntu_publisher._config.metaroot, 'test'), 'w').close()
+        self.assertIs(None, test_publisher._config.metaroot)
+
+        test_publisher.deleteArchive()
+        self.assertFalse(os.path.exists(test_publisher._config.archiveroot))
+        self.assertTrue(os.path.exists(ubuntu_publisher._config.metaroot))
+        # XXX wgrant 2014-07-07 bug=1338439: deleteArchive() currently
+        # kills all PPAs with the same name and owner.
+        #self.assertTrue(os.path.exists(ubuntu_publisher._config.archiveroot))
+
+        ubuntu_publisher.deleteArchive()
+        self.assertFalse(os.path.exists(ubuntu_publisher._config.metaroot))
+        self.assertFalse(os.path.exists(ubuntu_publisher._config.archiveroot))
+
+        self.assertNotIn('WARNING', logger.getLogBuffer())
+        self.assertNotIn('ERROR', logger.getLogBuffer())
+
     def testDeletingPPARename(self):
         a1 = self.factory.makeArchive(purpose=ArchivePurpose.PPA, name='test')
         getPublisher(a1, None, self.logger).deleteArchive()

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2014-04-09 12:32:50 +0000
+++ lib/lp/soyuz/model/queue.py	2014-07-07 08:12:52 +0000
@@ -1462,9 +1462,12 @@
         # complicated for our needs right now.  Also, the existing code
         # assumes that everything is a tarball and tries to unpack it.
 
-        archive = self.packageupload.archive
         # See the XXX near the import for getPubConfig.
-        archive_config = getPubConfig(archive)
+        archive_config = getPubConfig(self.packageupload.archive)
+        if archive_config.metaroot is None:
+            debug(logger, "Skipping meta-data for archive without metaroot.")
+            return
+
         dest_file = os.path.join(
             archive_config.metaroot, self.libraryfilealias.filename)
         if not os.path.isdir(archive_config.metaroot):