launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

getSupportedSeries is probably not an ideal name, given that SeriesStatus.SUPPORTED exists.

Also, we might want to make the method itself less nonsensical. Experimental is an active series status, so it probably wants Contents files.

Diff comments:

> === modified file 'lib/lp/archivepublisher/scripts/generate_contents_files.py'
> --- lib/lp/archivepublisher/scripts/generate_contents_files.py	2012-11-10 02:25:07 +0000
> +++ lib/lp/archivepublisher/scripts/generate_contents_files.py	2015-02-12 17:31:49 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Archive Contents files generator."""
> @@ -16,7 +16,6 @@
>  from lp.archivepublisher.config import getPubConfig
>  from lp.registry.interfaces.distribution import IDistributionSet
>  from lp.registry.interfaces.pocket import PackagePublishingPocket
> -from lp.registry.interfaces.series import SeriesStatus
>  from lp.services.command_spawner import (
>      CommandSpawner,
>      OutputLineHandler,
> @@ -132,20 +131,9 @@
>              if not file_exists(path):
>                  os.makedirs(path)
>  
> -    def getSupportedSeries(self):
> -        """Return suites that are supported in this distribution.
> -
> -        "Supported" means not EXPERIMENTAL or OBSOLETE.
> -        """
> -        unsupported_status = (SeriesStatus.EXPERIMENTAL,
> -                              SeriesStatus.OBSOLETE)
> -        for series in self.distribution:
> -            if series.status not in unsupported_status:
> -                yield series
> -
>      def getSuites(self):
>          """Return suites that are actually supported in this distribution."""
> -        for series in self.getSupportedSeries():
> +        for series in self.distribution.getSupportedSeries():
>              for pocket in PackagePublishingPocket.items:
>                  suite = series.getSuite(pocket)
>                  if file_exists(os.path.join(self.config.distsroot, suite)):
> @@ -274,7 +262,7 @@

There is an "Installing new Contents file" log message just above. It is now a lie.

>              new_contents = os.path.join(
>                  contents_dir, "%s.gz" % contents_filename)
>              contents_dest = os.path.join(
> -                self.config.distsroot, suite, "%s.gz" % contents_filename)
> +                contents_dir, ".%s.gz" % contents_filename)

I'm not entirely convinced that a hidden file is the best naming scheme.

>  
>              os.rename(current_contents, last_contents)
>              os.rename(new_contents, contents_dest)
> 
> === modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
> --- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2014-08-09 19:45:00 +0000
> +++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2015-02-12 17:31:49 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Master distro publishing script."""
> @@ -10,6 +10,7 @@
>  
>  from datetime import datetime
>  import os
> +import shutil
>  
>  from pytz import utc
>  from zope.component import getUtility
> @@ -156,6 +157,19 @@
>          for purpose, config in candidates if config is not None)
>  
>  
> +def newer_mtime(one_file, other_file):
> +    """Is one_file newer than other_file, or is other_file missing?"""
> +    try:
> +        one_mtime = os.stat(one_file).st_mtime
> +    except OSError:
> +        return False
> +    try:
> +        other_mtime = os.stat(other_file).st_mtime
> +    except OSError:
> +        return True
> +    return one_mtime > other_mtime
> +
> +
>  class PublishFTPMaster(LaunchpadCronScript):
>      """Publish a distro (update).
>  
> @@ -539,6 +553,32 @@
>                  # most of its time.
>                  self.publishDistroArchive(distribution, archive)
>  
> +    def updateContentsFile(self, distribution, suite, arch):
> +        """Update a single Contents file if necessary."""
> +        config = self.configs[distribution][ArchivePurpose.PRIMARY]
> +        backup_dists = get_backup_dists(config)
> +        content_dists = os.path.join(
> +            config.distroroot, "contents-generation", distribution.name,
> +            "dists")
> +        contents_filename = "Contents-%s" % arch.architecturetag
> +        current_contents = os.path.join(
> +            backup_dists, suite, "%s.gz" % contents_filename)
> +        new_contents = os.path.join(
> +            content_dists, suite, ".%s.gz" % contents_filename)
> +        if newer_mtime(new_contents, current_contents):
> +            self.logger.debug(
> +                "Installing new Contents file for %s/%s.", suite,
> +                arch.architecturetag)
> +            shutil.copy2(new_contents, current_contents)
> +
> +    def updateContentsFiles(self, distribution):
> +        """Pick up updated Contents files if necessary."""
> +        for series in distribution.getSupportedSeries():
> +            for pocket in PackagePublishingPocket.items:
> +                suite = series.getSuite(pocket)
> +                for arch in series.enabled_architectures:
> +                    self.updateContentsFile(distribution, suite, arch)
> +
>      def publish(self, distribution, security_only=False):
>          """Do the main publishing work.
>  
> @@ -558,6 +598,8 @@
>                  # Let's assume the main archive is always modified
>                  has_published = True
>  
> +            self.updateContentsFiles(distribution)
> +
>              # Swizzle the now-updated backup dists and the current dists
>              # around.
>              self.installDists(distribution)
> 
> === modified file 'lib/lp/archivepublisher/tests/test_generate_contents_files.py'
> --- lib/lp/archivepublisher/tests/test_generate_contents_files.py	2013-09-11 08:17:34 +0000
> +++ lib/lp/archivepublisher/tests/test_generate_contents_files.py	2015-02-12 17:31:49 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Test for the `generate-contents-files` script."""
> @@ -15,6 +15,7 @@
>      execute,
>      GenerateContentsFiles,
>      )
> +from lp.archivepublisher.scripts.publish_ftpmaster import PublishFTPMaster
>  from lp.registry.interfaces.pocket import PackagePublishingPocket
>  from lp.services.log.logger import DevNullLogger
>  from lp.services.osutils import write_file
> @@ -183,14 +184,6 @@
>          self.assertEqual(
>              [das.architecturetag], script.getArchs(distroseries.name))
>  
> -    def test_getSupportedSeries(self):
> -        # getSupportedSeries returns the supported distroseries in the
> -        # distribution.
> -        script = self.makeScript()
> -        distroseries = self.factory.makeDistroSeries(
> -            distribution=script.distribution)
> -        self.assertIn(distroseries, script.getSupportedSeries())
> -
>      def test_getSuites(self):
>          # getSuites returns the full names (distroseries-pocket) of the
>          # pockets that have packages to publish.
> @@ -268,7 +261,8 @@
>              script.content_archive, StartsWith(script.config.distroroot))
>  
>      def test_main(self):
> -        # If run end-to-end, the script generates Contents.gz files.
> +        # If run end-to-end, the script generates Contents.gz files, and a
> +        # following publisher run will put those files in their final place.
>          distro = self.makeDistro()
>          distroseries = self.factory.makeDistroSeries(distribution=distro)
>          processor = self.factory.makeProcessor()
> @@ -287,6 +281,13 @@
>          fake_overrides(script, distroseries)
>          script.process()
>          self.assertTrue(file_exists(os.path.join(
> +            script.content_archive, distro.name, "dists", suite,
> +            ".Contents-%s.gz" % das.architecturetag)))
> +        publisher_script = PublishFTPMaster(test_args=["-d", distro.name])
> +        publisher_script.txn = self.layer.txn
> +        publisher_script.logger = DevNullLogger()
> +        publisher_script.main()
> +        self.assertTrue(file_exists(os.path.join(
>              script.config.distsroot, suite,
>              "Contents-%s.gz" % das.architecturetag)))
>  
> 
> === modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
> --- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2014-04-15 16:32:03 +0000
> +++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2015-02-12 17:31:49 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Test publish-ftpmaster cron script."""
> @@ -8,6 +8,7 @@
>  import logging
>  import os
>  from textwrap import dedent
> +import time
>  
>  from apt_pkg import TagFile
>  from testtools.matchers import (
> @@ -24,6 +25,7 @@
>      compose_shell_boolean,
>      find_run_parts_dir,
>      get_working_dists,
> +    newer_mtime,
>      PublishFTPMaster,
>      shell_quote,
>      )
> @@ -38,6 +40,7 @@
>      BufferLogger,
>      DevNullLogger,
>      )
> +from lp.services.osutils import write_file
>  from lp.services.scripts.base import LaunchpadScriptFailure
>  from lp.services.utils import file_exists
>  from lp.soyuz.enums import (
> @@ -208,6 +211,46 @@
>          self.assertEqual("no", compose_shell_boolean(False))
>  
>  
> +class TestNewerMtime(TestCase):
> +
> +    def setUp(self):
> +        super(TestCase, self).setUp()
> +        tempdir = self.useTempDir()
> +        self.a = os.path.join(tempdir, "a")
> +        self.b = os.path.join(tempdir, "b")
> +
> +    def test_both_missing(self):
> +        self.assertFalse(newer_mtime(self.a, self.b))
> +
> +    def test_one_missing(self):
> +        write_file(self.b, "")
> +        self.assertFalse(newer_mtime(self.a, self.b))
> +
> +    def test_other_missing(self):
> +        write_file(self.a, "")
> +        self.assertTrue(newer_mtime(self.a, self.b))
> +
> +    def test_older(self):
> +        write_file(self.a, "")
> +        os.utime(self.a, (0, 0))
> +        write_file(self.b, "")
> +        self.assertFalse(newer_mtime(self.a, self.b))
> +
> +    def test_equal(self):
> +        now = time.time()
> +        write_file(self.a, "")
> +        os.utime(self.a, (now, now))
> +        write_file(self.b, "")
> +        os.utime(self.b, (now, now))
> +        self.assertFalse(newer_mtime(self.a, self.b))
> +
> +    def test_newer(self):
> +        write_file(self.a, "")
> +        write_file(self.b, "")
> +        os.utime(self.b, (0, 0))
> +        self.assertTrue(newer_mtime(self.a, self.b))
> +
> +
>  class TestFindRunPartsDir(TestCaseWithFactory, HelpersMixin):
>      layer = ZopelessDatabaseLayer
>  
> @@ -796,6 +839,31 @@
>                  "Did not find expected marker for %s."
>                  % archive.purpose.title)
>  
> +    def test_updateContentsFile_installs_changed(self):
> +        distro = self.makeDistroWithPublishDirectory()
> +        distroseries = self.factory.makeDistroSeries(distribution=distro)
> +        das = self.factory.makeDistroArchSeries(distroseries=distroseries)
> +        script = self.makeScript(distro)
> +        script.setUp()
> +        script.setUpDirs()
> +        archive_config = getPubConfig(distro.main_archive)
> +        contents_filename = "Contents-%s.gz" % das.architecturetag
> +        backup_suite = os.path.join(
> +            archive_config.archiveroot + "-distscopy", "dists",
> +            distroseries.name)
> +        os.makedirs(backup_suite)
> +        write_marker_file([backup_suite, contents_filename], "Old Contents")
> +        os.utime(os.path.join(backup_suite, contents_filename), (0, 0))
> +        content_suite = os.path.join(
> +            archive_config.distroroot, "contents-generation", distro.name,
> +            "dists", distroseries.name)
> +        os.makedirs(content_suite)
> +        write_marker_file(
> +            [content_suite, ".%s" % contents_filename], "Contents")
> +        script.updateContentsFile(distro, distroseries.name, das)
> +        self.assertEqual(
> +            "Contents", read_marker_file([backup_suite, contents_filename]))
> +
>      def test_publish_always_returns_true_for_primary(self):
>          script = self.makeScript()
>          script.publishDistroUploads = FakeMethod()
> 
> === modified file 'lib/lp/registry/interfaces/distribution.py'
> --- lib/lp/registry/interfaces/distribution.py	2014-11-17 18:36:16 +0000
> +++ lib/lp/registry/interfaces/distribution.py	2015-02-12 17:31:49 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Interfaces including and related to IDistribution."""
> @@ -408,6 +408,12 @@
>      def getDevelopmentSeries():
>          """Return the DistroSeries which are marked as in development."""
>  
> +    def getSupportedSeries():
> +        """Return the DistroSeries that are supported in this distribution.
> +
> +        "Supported" means not EXPERIMENTAL or OBSOLETE.
> +        """
> +
>      def resolveSeriesAlias(name):
>          """Resolve a series alias.
>  
> 
> === modified file 'lib/lp/registry/model/distribution.py'
> --- lib/lp/registry/model/distribution.py	2014-12-08 04:20:17 +0000
> +++ lib/lp/registry/model/distribution.py	2015-02-12 17:31:49 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Database classes for implementing distribution items."""
> @@ -785,6 +785,14 @@
>              distribution=self,
>              status=SeriesStatus.DEVELOPMENT)
>  
> +    def getSupportedSeries(self):
> +        """See `IDistribution`."""
> +        unsupported_status = (SeriesStatus.EXPERIMENTAL,
> +                              SeriesStatus.OBSOLETE)
> +        for series in self.series:
> +            if series.status not in unsupported_status:
> +                yield series
> +
>      def getMilestone(self, name):
>          """See `IDistribution`."""
>          return Milestone.selectOne("""
> 
> === modified file 'lib/lp/registry/tests/test_distribution.py'
> --- lib/lp/registry/tests/test_distribution.py	2013-08-01 14:09:45 +0000
> +++ lib/lp/registry/tests/test_distribution.py	2015-02-12 17:31:49 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Tests for Distribution."""
> @@ -383,7 +383,7 @@
>  
>  
>  class SeriesTests(TestCaseWithFactory):
> -    """Test IDistribution.getSeries().
> +    """Test IDistribution.getSeries() and friends.
>      """
>  
>      layer = LaunchpadFunctionalLayer
> @@ -416,6 +416,19 @@
>          self.assertEqual(
>              series, distro.getSeries("devel", follow_aliases=True))
>  
> +    def test_getSupportedSeries(self):
> +        distro = self.factory.makeDistribution()
> +        self.factory.makeDistroSeries(
> +            distribution=distro, status=SeriesStatus.OBSOLETE)
> +        current = self.factory.makeDistroSeries(
> +            distribution=distro, status=SeriesStatus.CURRENT)
> +        development = self.factory.makeDistroSeries(
> +            distribution=distro, status=SeriesStatus.DEVELOPMENT)
> +        self.factory.makeDistroSeries(
> +            distribution=distro, status=SeriesStatus.EXPERIMENTAL)
> +        self.assertContentEqual(
> +            [current, development], list(distro.getSupportedSeries()))
> +
>  
>  class DerivativesTests(TestCaseWithFactory):
>      """Test IDistribution.derivatives.
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/contents-race/+merge/249547
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.