launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04252
[Merge] lp:~jtv/launchpad/bug-809211 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-809211 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #809211 in Launchpad itself: "Move generate-contents-files' <dist>-contents into archive root"
https://bugs.launchpad.net/launchpad/+bug/809211
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-809211/+merge/67731
= Summary =
The contents files we generate for our package archives have been stored in separate directories outside the trees for their respective distributions. It's been decided that they should live inside their distributions' distro roots instead. This was harder in the past, when shell scripts and python code needed to share their ideas of what goes where. Nowadays we have a fully-pythonized generate-contents-files script that we're hoping to put into production soon, and the old shell script is broken.
== Proposed fix ==
Most of this was already abstracted away, so the change is relatively small.
One complication however is transition. Rather than requiring admins to move the existing directories on dogfood and production, I had the script move the data from the old location to the new as a one-off. We can take that code out later, along with the configuration item that determines where the contents files go.
== Pre-implementation notes ==
Discussed in detail with Julian, including the built-in migration code.
A happy side effect of the change is that at least one directory no longer needs to have a name that includes the distro name; its new location is isolated from data for other distros.
== Implementation details ==
There may be concurrency hazards if we run the publish-distro and generate-contents-files scripts concurrently on the same archive, as the contents directory may be yanked away. I'm not sure this is really a risk; I believe publish-ftpmaster also runs generate-contents-files and so it's possible that we end up with two identical directories anyway. I'm emailing Julian about this.
== Tests ==
{{{
./bin/tests -vvc lp.archivepublisher/tests/test_generate_contents_files
}}}
== Demo and Q/A ==
A qualified Soyuz engineer will have to run the python generate-contents-files.py script on dogfood. The contents directory /srv/launchpad.net/ubuntu-contents will turn into /srv/launchpad.net/ubuntu-archive/contents-generation and the script will log an INFO notice about this.
The notice will go away on subsequent runs, and the directory will stay in its new place.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/archivepublisher/tests/test_generate_contents_files.py
lib/lp/archivepublisher/scripts/generate_contents_files.py
--
https://code.launchpad.net/~jtv/launchpad/bug-809211/+merge/67731
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-809211 into lp:launchpad.
=== modified file 'lib/lp/archivepublisher/scripts/generate_contents_files.py'
--- lib/lp/archivepublisher/scripts/generate_contents_files.py 2011-04-27 10:48:28 +0000
+++ lib/lp/archivepublisher/scripts/generate_contents_files.py 2011-07-12 17:07:27 +0000
@@ -292,6 +292,40 @@
for arch in archs:
self.updateContentsFile(suite, arch)
+ def updateLegacyContentArchiveRoot(self):
+ """Replace content archive root with new contents location.
+
+ If there is a legacy content_archive_root, move it to the new
+ location.
+
+ This is a temporary migration tool for the fix to bug 809211.
+ After this has run at least once on each system that needs it, for
+ each distribution archive, this method can go away.
+ """
+ content_archive_root = getattr(
+ config.archivepublisher, 'content_archive_root', None)
+ if content_archive_root is None:
+ # No legacy configuration; nothing to do.
+ return
+ old_content_dir = os.path.join(
+ content_archive_root, self.distribution.name + "-contents")
+ if not file_exists(old_content_dir):
+ # The data has already been moved (or didn't exist at all).
+ return
+ if file_exists(self.content_archive):
+ # The new contents directory has already been created.
+ # Confusing!
+ self.logger.warn(
+ "New contents directory %s has been created, "
+ "but legacy directory %s still exists. Ignoring the latter.",
+ self.content_archive, old_content_dir)
+ return
+
+ self.logger.info(
+ "Moving legacy contents directory %s into %s.",
+ old_content_dir, self.content_archive)
+ os.rename(old_content_dir, self.content_archive)
+
def setUp(self):
"""Prepare configuration and filesystem state for the script's work.
@@ -302,8 +336,8 @@
self.processOptions()
self.config = getPubConfig(self.distribution.main_archive)
self.content_archive = os.path.join(
- config.archivepublisher.content_archive_root,
- self.distribution.name + "-contents")
+ self.config.distroroot, "contents-generation")
+ self.updateLegacyContentArchiveRoot()
self.setUpContentArchive()
def main(self):
=== modified file 'lib/lp/archivepublisher/tests/test_generate_contents_files.py'
--- lib/lp/archivepublisher/tests/test_generate_contents_files.py 2011-04-27 10:48:28 +0000
+++ lib/lp/archivepublisher/tests/test_generate_contents_files.py 2011-07-12 17:07:27 +0000
@@ -6,7 +6,8 @@
__metaclass__ = type
from optparse import OptionValueError
-import os.path
+import os
+from testtools.matchers import StartsWith
from textwrap import dedent
from canonical.config import config
@@ -127,8 +128,8 @@
layer = LaunchpadZopelessLayer
- def makeContentArchive(self):
- """Prepare a "content archive" directory for script tests."""
+ def makeLegacyContentArchive(self):
+ """Prepare a legacy "content archive" directory."""
content_archive = self.makeTemporaryDirectory()
config.push("content-archive", dedent("""\
[archivepublisher]
@@ -146,15 +147,35 @@
return self.factory.makeDistribution(
publish_root_dir=unicode(self.makeTemporaryDirectory()))
- def makeScript(self, distribution=None):
+ def makeScript(self, distribution=None, run_setup=True):
"""Create a script for testing."""
if distribution is None:
distribution = self.makeDistro()
script = GenerateContentsFiles(test_args=['-d', distribution.name])
script.logger = DevNullLogger()
- script.setUp()
+ if run_setup:
+ script.setUp()
+ else:
+ script.distribution = distribution
return script
+ def writeMarkerFile(self, file_path):
+ """Create a marker file at location `file_path`.
+
+ An arbitrary string is written to the file, and flushed to the
+ filesystem. Any surrounding directories are created as needed.
+
+ :param file_path: Full path to a file: optional directory prefix
+ followed by required file name.
+ :return: The arbitrary string that is also in the file.
+ """
+ marker_contents = self.factory.getUniqueString()
+ dir_name = os.path.dirname(file_path)
+ if not file_exists(dir_name):
+ os.makedirs(dir_name)
+ write_file(file_path, marker_contents)
+ return marker_contents
+
def test_name_is_consistent(self):
# Script instances for the same distro get the same name.
distro = self.factory.makeDistribution()
@@ -235,7 +256,6 @@
# writeAptContentsConf writes apt-contents.conf. At a minimum
# this will include a header based on apt_conf_header.template,
# with the right distribution name interpolated.
- self.makeContentArchive()
distro = self.makeDistro()
script = self.makeScript(distro)
script.writeAptContentsConf([], [])
@@ -249,9 +269,9 @@
# writeAptContentsConf adds sections based on
# apt_conf_dist.template for every suite, with certain
# parameters interpolated.
- content_archive = self.makeContentArchive()
distro = self.makeDistro()
script = self.makeScript(distro)
+ content_archive = script.content_archive
suite = self.factory.getUniqueString('suite')
arch = self.factory.getUniqueString('arch')
script.writeAptContentsConf([suite], [arch])
@@ -260,27 +280,85 @@
% (script.content_archive, distro.name)).read()
self.assertIn('tree "dists/%s"\n' % suite, apt_contents_conf)
overrides_path = os.path.join(
- content_archive, distro.name + "-contents",
- distro.name + "-overrides")
+ content_archive, distro.name + "-overrides")
self.assertIn('FileList "%s' % overrides_path, apt_contents_conf)
self.assertIn('Architectures "%s source";' % arch, apt_contents_conf)
def test_writeContentsTop(self):
# writeContentsTop writes a Contents.top file based on a
# standard template, with the distribution's title interpolated.
- content_archive = self.makeContentArchive()
distro = self.makeDistro()
script = self.makeScript(distro)
+ content_archive = script.content_archive
script.writeContentsTop()
+
contents_top = file(
- "%s/%s-contents/%s-misc/Contents.top"
- % (content_archive, distro.name, distro.name)).read()
+ "%s/%s-misc/Contents.top" % (content_archive, distro.name)).read()
+
self.assertIn("This file maps", contents_top)
self.assertIn(distro.title, contents_top)
+ def test_updateLegacyContentArchiveRoot_moves_legacy_contents(self):
+ # If updateLegacyContentArchiveRoot finds entries in the legacy
+ # content-archive directory, it moves them into their new
+ # location inside distroroot.
+ distro = self.makeDistro()
+ script = self.makeScript(distro, run_setup=False)
+ content_archive = self.makeLegacyContentArchive()
+ script.content_archive = os.path.join(
+ self.makeTemporaryDirectory(), "contents-generation")
+ old_contents_dir = os.path.join(
+ content_archive, "%s-contents" % distro.name)
+
+ marker_contents = self.writeMarkerFile(os.path.join(
+ old_contents_dir, "%s-misc" % distro.name, "Contents.top"))
+
+ script.updateLegacyContentArchiveRoot()
+
+ self.assertFalse(file_exists(old_contents_dir))
+ new_path = os.path.join(
+ script.content_archive, "%s-misc" % distro.name, "Contents.top")
+ self.assertEqual(marker_contents, file(new_path).read())
+
+ def test_updateLegacyContentArchiveRoot_is_harmless_without_config(self):
+ # If no legacy content-archive root directory is configured,
+ # that's fine. It means that updateLegacyContentArchiveRoot has
+ # nothing to do.
+ script = self.makeScript()
+ script.updateLegacyContentArchiveRoot()
+ self.assertTrue(file_exists(script.content_archive))
+ self.assertThat(
+ script.content_archive, StartsWith(script.config.distroroot))
+
+ def test_updateLegacyContentArchiveRoot_is_harmless_without_legacy(self):
+ # If a legacy content-archive root directory is configured but
+ # it does not actually exist, updateLegacyContentArchiveRoot
+ # does nothing.
+ self.makeLegacyContentArchive()
+ script = self.makeScript()
+ script.updateLegacyContentArchiveRoot()
+ self.assertTrue(file_exists(script.content_archive))
+
+ def test_setUp_moves_legacy_content_archive(self):
+ # setUp calls updateLegacyContentArchiveRoot, which moves any
+ # legacy content-archive contents to their new location.
+ content_archive = self.makeLegacyContentArchive()
+ distro = self.makeDistro()
+ marker_dir = os.path.join(
+ content_archive, "%s-contents" % distro.name,
+ "%s-misc" % distro.name)
+ self.writeMarkerFile(os.path.join(marker_dir, "Contents.top"))
+ self.makeScript(distro, run_setup=True)
+ self.assertFalse(file_exists(marker_dir))
+
+ def test_setUp_places_content_archive_in_distroroot(self):
+ # The contents files are kept in subdirectories of distroroot.
+ script = self.makeScript()
+ self.assertThat(
+ script.content_archive, StartsWith(script.config.distroroot))
+
def test_main(self):
# If run end-to-end, the script generates Contents.gz files.
- self.makeContentArchive()
distro = self.makeDistro()
distroseries = self.factory.makeDistroSeries(distribution=distro)
processor = self.factory.makeProcessor()
