launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> 
> === renamed file 'lib/lp/archivepublisher/uefi.py' => 'lib/lp/archivepublisher/signing.py'
> --- lib/lp/archivepublisher/uefi.py	2016-05-03 09:30:51 +0000
> +++ lib/lp/archivepublisher/signing.py	2016-05-06 09:18:56 +0000
> @@ -59,32 +59,48 @@
>          return bits[0], bits[1], bits[2].split(".")[0]
>  
>      def setComponents(self, tarfile_path):
> -        self.loader_type, self.version, self.arch = self.parsePath(
> +        self.package, self.version, self.arch = self.parsePath(
>              tarfile_path)
>  
>      def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
> -        if pubconf.uefiroot is None:
> +        if pubconf.signingroot is None:
>              if self.logger is not None:
> -                self.logger.warning("No UEFI root configured for this archive")
> +                self.logger.warning(
> +                    "No SIGNING root configured for this archive")

UEFI was being used as an acronym there rather than as SHOUTYCAPS, so I'd just spell that "signing".

>              self.key = None
>              self.cert = None
>              self.autokey = False
>          else:
> -            self.key = os.path.join(pubconf.uefiroot, "uefi.key")
> -            self.cert = os.path.join(pubconf.uefiroot, "uefi.crt")
> -            self.autokey = pubconf.uefiautokey
> +            self.key = os.path.join(pubconf.signingroot, "uefi.key")
> +            self.cert = os.path.join(pubconf.signingroot, "uefi.crt")
> +            self.autokey = pubconf.signingautokey
>  
>          self.setComponents(tarfile_path)
> +
> +        # Ensure we expose the results via uefi and signed in dists.
> +        # For compatibility if efi already exists create a symlink
> +        # to it from signed.  Otherwise create the link the other way.
> +        dists_signing = os.path.join(
> +            pubconf.archiveroot, "dists", distroseries, "main", "signed")

This should be dists_signed, not dists_signing.

> +        dists_uefi = os.path.join(
> +            pubconf.archiveroot, "dists", distroseries, "main", "uefi")
> +        if not os.path.exists(dists_signing):
> +            if os.path.isdir(dists_uefi):
> +                os.symlink("uefi", dists_signing)
> +            else:
> +                os.makedirs(dists_signing, 0o755)
> +                os.symlink("signed", dists_uefi)

It's generally better to end up in a single consistent state.  Could we instead do:

  if not exists(signed):
      if exists(uefi):
          mv(uefi, signed)
  if not exists(uefi):
      if not exists(signed):
          makedirs(signed)
      symlink(signed, uefi)

> +
> +        # Extract into the "signed" path regardless of linking.
>          self.targetdir = os.path.join(
> -            pubconf.archiveroot, "dists", distroseries, "main", "uefi",
> -            "%s-%s" % (self.loader_type, self.arch))
> +            dists_signing, "%s-%s" % (self.package, self.arch))
>          self.archiveroot = pubconf.archiveroot
>  
>      @classmethod
>      def getSeriesKey(cls, tarfile_path):
>          try:
> -            loader_type, _, arch = cls.parsePath(tarfile_path)
> -            return loader_type, arch
> +            package, _, arch = cls.parsePath(tarfile_path)
> +            return package, arch
>          except ValueError:
>              return None
>  
> 
> === modified file 'lib/lp/archiveuploader/tests/test_nascentuploadfile.py'
> --- lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2015-03-04 13:06:02 +0000
> +++ lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2016-05-06 09:18:56 +0000
> @@ -161,10 +161,16 @@
>              "bla.txt", "data", "main/raw-installer", "extra")
>          self.assertTrue(uploadfile.autoApprove())
>  
> +    def test_uefi_not_auto_approved_compat(self):
> +        # UEFI uploads are auto-approved.
> +        uploadfile = self.createCustomUploadFile(
> +            "bla.txt", "data", "main/raw-uefi", "extra")
> +        self.assertFalse(uploadfile.autoApprove())
> +
>      def test_uefi_not_auto_approved(self):
>          # UEFI uploads are auto-approved.
>          uploadfile = self.createCustomUploadFile(
> -            "bla.txt", "data", "main/raw-uefi", "extra")
> +            "bla.txt", "data", "main/raw-signing", "extra")
>          self.assertFalse(uploadfile.autoApprove())

Maybe just test_uefi_not_auto_approved and test_signing_not_auto_approved.

>  
>  
> 
> === modified file 'lib/lp/archiveuploader/tests/test_uploadpolicy.py'
> --- lib/lp/archiveuploader/tests/test_uploadpolicy.py	2013-08-01 14:09:45 +0000
> +++ lib/lp/archiveuploader/tests/test_uploadpolicy.py	2016-05-06 09:18:56 +0000
> @@ -304,6 +304,17 @@
>          upload.changes = FakeChangesFile(custom_files=[uploadfile])
>          self.assertFalse(buildd_policy.autoApprove(upload))
>  
> +    def test_buildd_does_not_approve_signed(self):

s/signed/signing/

> +        # Uploads to the primary archive containing files for signing are
> +        # not approved.
> +        buildd_policy = findPolicyByName("buildd")
> +        uploadfile = CustomUploadFile(
> +            "uefi.tar.gz", None, 0, "main/raw-signing", "extra", buildd_policy,
> +            None)
> +        upload = make_fake_upload(binaryful=True)
> +        upload.changes = FakeChangesFile(custom_files=[uploadfile])
> +        self.assertFalse(buildd_policy.autoApprove(upload))
> +
>      def test_buildd_approves_uefi_ppa(self):
>          # Uploads to PPAs containing UEFI custom files are auto-approved.
>          buildd_policy = findPolicyByName("buildd")
> 
> === modified file 'lib/lp/soyuz/browser/queue.py'
> --- lib/lp/soyuz/browser/queue.py	2015-07-09 20:06:17 +0000
> +++ lib/lp/soyuz/browser/queue.py	2016-05-06 09:18:56 +0000
> @@ -574,7 +574,7 @@
>              (self.contains_installer, ("Installer", 'ubuntu-icon')),
>              (self.contains_upgrader, ("Upgrader", 'ubuntu-icon')),
>              (self.contains_ddtp, (ddtp, 'ubuntu-icon')),
> -            (self.contains_uefi, ("Signed UEFI boot loader", 'ubuntu-icon')),
> +            (self.contains_signing, ("Signing", 'ubuntu-icon')),

I think perhaps "Objects for signing".

>              ]
>          return [
>              self.composeIcon(*details)
> 
> === modified file 'lib/lp/soyuz/interfaces/queue.py'
> --- lib/lp/soyuz/interfaces/queue.py	2015-09-03 15:14:07 +0000
> +++ lib/lp/soyuz/interfaces/queue.py	2016-05-06 09:18:56 +0000
> @@ -264,8 +264,11 @@
>          "whether or not this upload contains upgrader images")
>      contains_ddtp = Attribute(
>          "whether or not this upload contains DDTP images")
> +    contains_signing = Attribute(
> +        "whether or not this upload contains signing images")
>      contains_uefi = Attribute(
> -        "whether or not this upload contains a signed UEFI boot loader image")
> +        "whether or not this upload contains a signed UEFI boot loader image" +

No need for the "+"; adjacent literal strings are concatenated automatically.

> +        " (deprecated)")
>      isPPA = Attribute(
>          "Return True if this PackageUpload is a PPA upload.")
>  


-- 
https://code.launchpad.net/~apw/launchpad/generify-uefi-signing/+merge/293802
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.