launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05261
[Merge] lp:~jtv/launchpad/bug-873421 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-873421 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #873421 in Launchpad itself: "soyuz publisher crash when opening new series"
https://bugs.launchpad.net/launchpad/+bug/873421
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-873421/+merge/79662
= Summary =
See bug 873421: publish-ftpmaster failed while trying to write Release files for a newly-created Ubuntu release series. The directory where the file was supposed to go did not exist.
== Proposed fix ==
Create the directory at the point where it's needed. I believe I removed the code that did this at one point, probably in converting the script from shell to python, thinking it was redundant. As far as I've been able to make out, we didn't create the directory in the place where you'd expect (the publish-distro script that actually needs the directory) and so it was probably too far removed from its use to be obvious.
== Pre-implementation notes ==
The real question is why we didn't notice this before. The main answer seems to be: because we didn't create our test series in Frozen status. That's what we do for Ubuntu series in production, but a new series is created in Experimental status by default. It's even conceivable that a "regular" publisher run while the series is not Frozen might create the Release directories and so hide the problem.
== Implementation details ==
I extracted the publisher code that writes the Release file into a new unit-testable method, _writeReleaseFile. After testing for the problem, I added a simple "if not file_exists(location): os.makedirs(location)." That takes only two lines, but it's what fixes the bug. The rest of the diff is tests and fluff.
== Tests ==
{{{
./bin/test -vvc lp.archivepublisher.tests.test_publisher
}}}
If you're still being hit by the Oneiric Librarian bug, run just the new tests (which don't use the Librarian):
{{{
./bin/test -vvc lp.archivepublisher.tests.test_publisher -t TestPublisherLite
}}}
== Demo and Q/A ==
Open a new Ubuntu series. Set its status to Frozen. Run publish-ftpmaster.
Actually I'm already doing this, so Q/A is just about finished: without this branch it crashed consistently, and with this branch merged it didn't. I'm just waiting to see if perhaps it goes wrong at any later point.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/archivepublisher/publishing.py
lib/lp/archivepublisher/tests/test_publisher.py
--
https://code.launchpad.net/~jtv/launchpad/bug-873421/+merge/79662
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-873421 into lp:launchpad.
=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py 2011-10-07 17:10:39 +0000
+++ lib/lp/archivepublisher/publishing.py 2011-10-18 09:39:27 +0000
@@ -17,8 +17,8 @@
import shutil
from debian.deb822 import (
+ _multivalued,
Release,
- _multivalued,
)
from canonical.database.sqlbase import sqlvalues
@@ -41,6 +41,7 @@
RepositoryIndexFile,
)
from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.utils import file_exists
from lp.soyuz.enums import (
ArchivePurpose,
ArchiveStatus,
@@ -237,17 +238,19 @@
for distroseries in self.distro.series:
for pocket in self.archive.getPockets():
- if (self.allowed_suites and not (distroseries.name, pocket) in
- self.allowed_suites):
+ allowed = (
+ not self.allowed_suites or
+ (distroseries.name, pocket) in self.allowed_suites)
+ if allowed:
+ more_dirt = distroseries.publish(
+ self._diskpool, self.log, self.archive, pocket,
+ is_careful=force_publishing)
+
+ self.dirty_pockets.update(more_dirt)
+
+ else:
self.log.debug(
- "* Skipping %s/%s" % (distroseries.name, pocket.name))
- continue
-
- more_dirt = distroseries.publish(
- self._diskpool, self.log, self.archive, pocket,
- is_careful=force_publishing)
-
- self.dirty_pockets.update(more_dirt)
+ "* Skipping %s/%s", distroseries.name, pocket.name)
def A2_markPocketsWithDeletionsDirty(self):
"""An intermediate step in publishing to detect deleted packages.
@@ -481,6 +484,20 @@
return self.distro.displayname
return "LP-PPA-%s" % get_ppa_reference(self.archive)
+ def _writeReleaseFile(self, suite, release_data):
+ """Write a Release file to the archive.
+
+ :param suite: The name of the suite whose Release file is to be
+ written.
+ :param release_data: A `debian.deb822.Release` object to write
+ to the filesystem.
+ """
+ location = os.path.join(self._config.distsroot, suite)
+ if not file_exists(location):
+ os.makedirs(location)
+ with open(os.path.join(location, "Release"), "w") as release_file:
+ release_data.dump(release_file, "utf-8")
+
def _writeSuite(self, distroseries, pocket):
"""Write out the Release files for the provided suite."""
# XXX: kiko 2006-08-24: Untested method.
@@ -552,12 +569,7 @@
"name": filename,
"size": len(entry)})
- f = open(os.path.join(
- self._config.distsroot, suite, "Release"), "w")
- try:
- release_file.dump(f, "utf-8")
- finally:
- f.close()
+ self._writeReleaseFile(suite, release_file)
# Skip signature if the archive signing key is undefined.
if self.archive.signing_key is None:
=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py 2011-10-07 17:07:52 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py 2011-10-18 09:39:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for publisher class."""
@@ -17,7 +17,6 @@
from textwrap import dedent
from debian.deb822 import Release
-
import transaction
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
@@ -26,7 +25,7 @@
from canonical.database.constants import UTC_NOW
from canonical.launchpad.ftests.keys_for_tests import gpgkeysdir
from canonical.launchpad.interfaces.gpghandler import IGPGHandler
-from lp.testing.keyserver import KeyServerTac
+from canonical.testing.layers import ZopelessDatabaseLayer
from lp.archivepublisher.config import getPubConfig
from lp.archivepublisher.diskpool import DiskPool
from lp.archivepublisher.interfaces.archivesigningkey import (
@@ -45,17 +44,21 @@
pocketsuffix,
)
from lp.registry.interfaces.series import SeriesStatus
-from lp.services.log.logger import BufferLogger
+from lp.services.log.logger import (
+ BufferLogger,
+ DevNullLogger,
+ )
+from lp.services.utils import file_exists
from lp.soyuz.enums import (
ArchivePurpose,
ArchiveStatus,
BinaryPackageFormat,
PackagePublishingStatus,
)
-from lp.soyuz.interfaces.archive import (
- IArchiveSet,
- )
+from lp.soyuz.interfaces.archive import IArchiveSet
from lp.soyuz.tests.test_publishing import TestNativePublishingBase
+from lp.testing import TestCaseWithFactory
+from lp.testing.keyserver import KeyServerTac
RELEASE = PackagePublishingPocket.RELEASE
@@ -817,8 +820,8 @@
publisher.A_publish(False)
publisher.C_doFTPArchive(False)
- self.assertTrue(
- ('breezy-autotest', PackagePublishingPocket.RELEASE) in
+ self.assertIn(
+ ('breezy-autotest', PackagePublishingPocket.RELEASE),
publisher.release_files_needed)
publisher.D_writeReleaseFiles(False)
@@ -877,7 +880,8 @@
archive_publisher.D_writeReleaseFiles(False)
release = self.parseRelease(os.path.join(
- archive_publisher._config.distsroot, 'breezy-autotest', 'Release'))
+ archive_publisher._config.distsroot, 'breezy-autotest',
+ 'Release'))
self.assertEqual('LP-PPA-cprov', release['origin'])
# The Label: field should be set to the archive displayname
@@ -925,7 +929,8 @@
# Check the distinct Origin: field content in the main Release file
# and the component specific one.
release = self.parseRelease(os.path.join(
- archive_publisher._config.distsroot, 'breezy-autotest', 'Release'))
+ archive_publisher._config.distsroot, 'breezy-autotest',
+ 'Release'))
self.assertEqual('LP-PPA-cprov-testing', release['origin'])
arch_release = self.parseRelease(os.path.join(
@@ -1381,3 +1386,82 @@
# All done, turn test-keyserver off.
tac.tearDown()
+
+
+class TestPublisherLite(TestCaseWithFactory):
+ """Lightweight unit tests for the publisher."""
+
+ layer = ZopelessDatabaseLayer
+
+ def makePublishableSeries(self, root_dir):
+ """Create a `DistroSeries` ready for publishing.
+
+ :param root_dir: A temporary directory for use as an archive root.
+ """
+ distro = self.factory.makeDistribution(publish_root_dir=root_dir)
+ return self.factory.makeDistroSeries(
+ distribution=distro, status=SeriesStatus.FROZEN)
+
+ def getReleaseFileDir(self, root, distroseries, suite):
+ """Locate the directory where a Release file should be.
+
+ :param root: Archive root directory.
+ :param distroseries: Published distroseries.
+ :param suite: Published suite.
+ """
+ return os.path.join(
+ root, distroseries.distribution.name, 'dists', suite)
+
+ def makePublishablePackage(self, series):
+ """Create a source publication ready for publishing."""
+ return self.factory.makeSourcePackagePublishingHistory(
+ distroseries=series, status=PackagePublishingStatus.PENDING)
+
+ def makePublisher(self, series):
+ """Create a publisher for a given distroseries."""
+ return getPublisher(series.main_archive, None, DevNullLogger())
+
+ def makeFakeReleaseData(self):
+ """Create a fake `debian.deb822.Release`.
+
+ The object's dump method will write arbitrary text. For testing
+ purposes, the fake object will compare equal to a string holding
+ this same text, encoded in the requested encoding.
+ """
+ class FakeReleaseData(unicode):
+ def dump(self, output_file, encoding):
+ output_file.write(self.encode(encoding))
+
+ return FakeReleaseData(self.factory.getUniqueUnicode())
+
+ def test_writeReleaseFile_dumps_release_file(self):
+ # _writeReleaseFile writes a Release file for a suite.
+ root = unicode(self.makeTemporaryDirectory())
+ series = self.makePublishableSeries(root)
+ spph = self.makePublishablePackage(series)
+ suite = series.name + pocketsuffix[spph.pocket]
+ releases_dir = self.getReleaseFileDir(root, series, suite)
+ os.makedirs(releases_dir)
+ release_data = self.makeFakeReleaseData()
+ release_path = os.path.join(releases_dir, "Release")
+
+ self.makePublisher(series)._writeReleaseFile(suite, release_data)
+
+ self.assertTrue(file_exists(release_path))
+ self.assertEqual(
+ release_data.encode('utf-8'), file(release_path).read())
+
+ def test_writeReleaseFile_creates_directory_if_necessary(self):
+ # If the suite is new and its release directory does not exist
+ # yet, _writeReleaseFile will create it.
+ root = unicode(self.makeTemporaryDirectory())
+ series = self.makePublishableSeries(root)
+ spph = self.makePublishablePackage(series)
+ suite = series.name + pocketsuffix[spph.pocket]
+ release_data = self.makeFakeReleaseData()
+ release_path = os.path.join(
+ self.getReleaseFileDir(root, series, suite), "Release")
+
+ self.makePublisher(series)._writeReleaseFile(suite, release_data)
+
+ self.assertTrue(file_exists(release_path))
