Code review comment for lp:~jelmer/launchpad/128259-async-1

Hi Jelmer,

a nice branch! I have just a few formal nitpicks, see below.

> === modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
> --- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2010-08-06 13:52:46 +0000
> +++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2010-08-12 21:09:28 +0000

[...]

> + def testSuccess(self):
> + # Upload a source package
> + upload_dir = self.queueUpload("bar_1.0-1")
> + self.processUpload(self.uploadprocessor, upload_dir)
> + source_pub = self.publishPackage('bar', '1.0-1')
> + [build] = source_pub.createMissingBuilds()
> +
> + # Move the source from the accepted queue.
> + [queue_item] = self.breezy.getQueueItems(
> + status=PackageUploadStatus.ACCEPTED,
> + version="1.0-1", name="bar")
> + queue_item.setDone()
> +
> + build.jobStarted()
> + build.builder = self.factory.makeBuilder()
> +
> + # Upload and accept a binary for the primary archive source.
> + shutil.rmtree(upload_dir)
> + self.layer.txn.commit()
> + leaf_name = "%d-%d" % (build.id, 60)
> + upload_dir = self.queueUpload("bar_1.0-1_binary",
> + queue_entry=leaf_name)
> + self.options.context = 'buildd'
> + self.options.builds = True
> + self.uploadprocessor.processBuildUpload(self.incoming_folder, leaf_name)
> + self.layer.txn.commit()
> + self.assertEquals(BuildStatus.FULLYBUILT, build.status)
> + log_lines = build.upload_log.read().splitlines()
> + self.assertTrue(
> + 'INFO: Processing upload bar_1.0-1_i386.changes' in log_lines)
> + self.assertTrue(
> + 'INFO: Committing the transaction and any mails associated with'
> + 'this upload.')

As already mentioned on IRC, this should be something like

    assertTrue('INFO...' in log_lines)

(And I assume that a ' ' is missing after 'with'.)

> === modified file 'lib/lp/archiveuploader/uploadprocessor.py'
> --- lib/lp/archiveuploader/uploadprocessor.py 2010-08-12 17:54:00 +0000
> +++ lib/lp/archiveuploader/uploadprocessor.py 2010-08-12 21:09:28 +0000

[...]

> @@ -184,7 +257,8 @@
> for changes_file in changes_files:
> self.log.debug("Considering changefile %s" % changes_file)
> try:
> - result = self.processChangesFile(upload_path, changes_file)
> + result = self.processChangesFile(upload_path, changes_file,
> + self.log)

Our style guide says that we should put all parameters into the second
(and following) line if they don't fit completely into the first line.

[...]

> @@ -376,11 +451,11 @@
> except UploadPolicyError, e:
> upload.reject("UploadPolicyError escaped upload.process: "
> "%s " % e)
> - self.log.debug("UploadPolicyError escaped upload.process",
> + logger.debug("UploadPolicyError escaped upload.process",
> exc_info=True)

Again, please move all parameters to the second line.

> except FatalUploadError, e:
> upload.reject("UploadError escaped upload.process: %s" % e)
> - self.log.debug("UploadError escaped upload.process",
> + logger.debug("UploadError escaped upload.process",
> exc_info=True)

...and here too

> except (KeyboardInterrupt, SystemExit):
> raise
> @@ -398,7 +473,7 @@
> # the new exception will be handled by the caller just like
> # the one we caught would have been, by failing the upload
> # with no email.
> - self.log.exception("Unhandled exception processing upload")
> + logger.exception("Unhandled exception processing upload")
> upload.reject("Unhandled exception processing upload: %s" % e)
>
> # XXX julian 2007-05-25 bug=29744:
> @@ -416,20 +491,20 @@
> successful = upload.do_accept(notify=notify)
> if not successful:
> result = UploadStatusEnum.REJECTED
> - self.log.info("Rejection during accept. "
> + logger.info("Rejection during accept. "
> "Aborting partial accept.")

...and here

> self.ztm.abort()
>
> if upload.is_rejected:
> - self.log.warn("Upload was rejected:")
> + logger.warn("Upload was rejected:")
> for msg in upload.rejections:
> - self.log.warn("\t%s" % msg)
> + logger.warn("\t%s" % msg)
>
> if self.dry_run:
> - self.log.info("Dry run, aborting transaction.")
> + logger.info("Dry run, aborting transaction.")
> self.ztm.abort()
> else:
> - self.log.info("Committing the transaction and any mails "
> + logger.info("Committing the transaction and any mails "
> "associated with this upload.")

...and here.