launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

This is going to block any binary copies to series (or distros!) with fewer arches, but otherwise seems fine.

Diff comments:

> === modified file 'lib/lp/soyuz/configure.zcml'
> --- lib/lp/soyuz/configure.zcml	2014-02-18 11:40:52 +0000
> +++ lib/lp/soyuz/configure.zcml	2014-05-30 12:14:46 +0000
> @@ -1,4 +1,4 @@
> -<!-- Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +<!-- Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
>       GNU Affero General Public License version 3 (see the file LICENSE).
>  -->
>  
> @@ -199,7 +199,6 @@
>          <require
>              permission="launchpad.Edit"
>              attributes="
> -                setNew
>                  setUnapproved
>                  setRejected
>                  setAccepted
> 
> === modified file 'lib/lp/soyuz/model/packagecopyjob.py'
> --- lib/lp/soyuz/model/packagecopyjob.py	2014-05-15 08:49:44 +0000
> +++ lib/lp/soyuz/model/packagecopyjob.py	2014-05-30 12:14:46 +0000
> @@ -515,8 +515,7 @@
>              raise CannotCopy("Package %r %r not found." % (name, version))
>          return source_package
>  
> -    def _checkPolicies(self, source_name, source_component=None,
> -                       auto_approve=False):
> +    def _checkPolicies(self, source_name, source_package, auto_approve=False):
>          # This helper will only return if it's safe to carry on with the
>          # copy, otherwise it raises SuspendJobException to tell the job
>          # runner to suspend the job.
> @@ -532,7 +531,8 @@
>              # metadata.
>              defaults = UnknownOverridePolicy().calculateSourceOverrides(
>                  self.target_archive, self.target_distroseries,
> -                self.target_pocket, [source_name], source_component)
> +                self.target_pocket, [source_name],
> +                source_package.sourcepackagerelease.component)
>              self.addSourceOverride(defaults[0])
>              if auto_approve:
>                  auto_approve = self.target_archive.canAdministerQueue(
> @@ -556,6 +556,29 @@
>                      self.requester, self.getSourceOverride().component,
>                      self.target_pocket, self.target_distroseries)
>  
> +        # Go through the same routine for binary overrides if necessary.  We
> +        # assume that the above permission check on the source component is
> +        # sufficient.
> +        if self.include_binaries:
> +            binaries = source_package.getBuiltBinaries()
> +            bpn_archtag = [(
> +                bpph.binarypackagerelease.binarypackagename,
> +                bpph.distroarchseries.architecturetag) for bpph in binaries]
> +            ancestry = override_policy.calculateBinaryOverrides(
> +                self.target_archive, self.target_distroseries,
> +                self.target_pocket, bpn_archtag)
> +            if len(ancestry) != len(binaries):

This doesn't consider that not all of the binaries may end up being copied. That's probably only going to happen when there are fewer architectures in the destination, so it will block every arch-dep copy into a derivative with fewer arches.

> +                # XXX cjwatson bug=1079577: Binary overrides for Ubuntu
> +                # should be calculated using the Ubuntu override policy, and
> +                # they should be saved in the metadata with an API for
> +                # overriding them.
> +                approve_new = auto_approve or copy_policy.autoApproveNew(
> +                    self.target_archive, self.target_distroseries,
> +                    self.target_pocket)
> +                if not approve_new:
> +                    self._createPackageUpload()
> +                    raise SuspendJobException
> +
>          # The package is not new (it has ancestry) so check the copy
>          # policy for existing packages.
>          approve_existing = auto_approve or copy_policy.autoApprove(
> @@ -643,9 +666,7 @@
>              [self.context.id]).any()
>          if pu is None:
>              source_name = getUtility(ISourcePackageNameSet)[self.package_name]
> -            self._checkPolicies(
> -                source_name, source_package.sourcepackagerelease.component,
> -                self.auto_approve)
> +            self._checkPolicies(source_name, source_package, self.auto_approve)
>  
>          # The package is free to go right in, so just copy it now.
>          ancestry = self.target_archive.getPublishedSources(
> 
> === modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
> --- lib/lp/soyuz/tests/test_packagecopyjob.py	2014-05-15 09:06:24 +0000
> +++ lib/lp/soyuz/tests/test_packagecopyjob.py	2014-05-30 12:14:46 +0000
> @@ -18,6 +18,7 @@
>  from zope.security.proxy import removeSecurityProxy
>  
>  from lp.bugs.interfaces.bugtask import BugTaskStatus
> +from lp.buildmaster.enums import BuildStatus
>  from lp.registry.interfaces.pocket import PackagePublishingPocket
>  from lp.registry.interfaces.series import SeriesStatus
>  from lp.registry.model.distroseriesdifferencecomment import (
> @@ -1003,6 +1004,40 @@
>              [removeSecurityProxy(job).context.id]).one()
>          self.assertEqual(PackageUploadStatus.NEW, pu.status)
>  
> +    def test_copying_new_binaries(self):
> +        # A copy involving source and new binaries requires NEW queue approval.
> +        target_archive = self.factory.makeArchive(
> +            self.distroseries.distribution, purpose=ArchivePurpose.PRIMARY)
> +        source_archive = self.factory.makeArchive()
> +        old_spph = self.publisher.getPubSource(
> +            distroseries=self.distroseries, sourcename="copyme", version="1.0",
> +            status=PackagePublishingStatus.PUBLISHED, archive=target_archive)
> +        self.publisher.getPubBinaries(
> +            binaryname="copyme", pub_source=old_spph,
> +            distroseries=self.distroseries,
> +            status=PackagePublishingStatus.PUBLISHED)
> +        new_spph = self.publisher.getPubSource(
> +            distroseries=self.distroseries, sourcename="copyme", version="1.1",
> +            status=PackagePublishingStatus.PUBLISHED, archive=source_archive)
> +        for build in new_spph.createMissingBuilds():
> +            build.updateStatus(BuildStatus.FULLYBUILT)
> +            for binaryname in ("copyme", "libcopyme-dev"):
> +                bpr = self.publisher.uploadBinaryForBuild(build, binaryname)
> +                self.publisher.publishBinaryInArchive(
> +                    bpr, new_spph.archive,
> +                    status=PackagePublishingStatus.PUBLISHED)
> +            pu = self.publisher.addPackageUpload(
> +                new_spph.archive, self.distroseries,
> +                changes_file_name="copyme_1.1_%s.changes" % build.arch_tag)
> +            pu.addBuild(build)
> +        job = self.createCopyJobForSPPH(
> +            new_spph, source_archive, target_archive, include_binaries=True)
> +        self.runJob(job)
> +        self.assertEqual(JobStatus.SUSPENDED, job.status)
> +        pu = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
> +            [removeSecurityProxy(job).context.id]).one()
> +        self.assertEqual(PackageUploadStatus.NEW, pu.status)
> +
>      def test_copying_to_main_archive_unapproved(self):
>          # Uploading to a series that is in a state that precludes auto
>          # approval will cause the job to suspend and a packageupload
> @@ -1302,6 +1337,10 @@
>              distroseries=self.distroseries, sourcename="copyme",
>              version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
>              component='multiverse', section='web', archive=target_archive)
> +        self.publisher.getPubBinaries(
> +            binaryname="copyme", pub_source=old_spph,
> +            distroseries=self.distroseries,
> +            status=PackagePublishingStatus.PUBLISHED)
>          old_spr = old_spph.sourcepackagerelease
>          diff_file = self.publisher.addMockFile("diff_file", restricted=True)
>          package_diff = old_spr.requestDiffTo(target_archive.owner, spr)
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/copies-respect-new/+merge/221529
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.