Code review comment for lp:~jml/launchpad/process-upload-checks

On Fri, Jan 22, 2010 at 3:11 AM, Julian Edwards
<email address hidden> wrote:
> Review: Needs Information code
> Thanks for fixing things!  Here's my review:
>

Hey Julian,

Thanks for the review.

As a meta-comment, I'd really appreciate it if you could leave the
filenames from the diff in your reply. It makes it much, much faster
to find the relevant code in my editor.

>> + # XXX: JonathanLange 2010-01-15: This shouldn't instantiate policy
>> + # types. They should instead have names as class variables.
>
> The XXX should reference a bug :)  Or better still, fix it, it's an easy change.
>

Bug filed. It's an easy change, but the branch is big enough as-is.
I'll make the change in a separate branch.

>> + # XXX: JonathanLange 2010-01-15: This has to be exactly the same
>> + # string as the one in SourcePackageRecipeBuild.policy_name. Factor
>> + # out a shared constant.
>
> Same here.
>

And same reply.

>> class SourcePackageRecipeUploadPolicy(BuildDaemonUploadPolicy):
>
> Did you double check the inherited policy options for this?  I think it looks sane but I have to ask ...
>

        self.unsigned_changes_ok = True
        self.unsigned_dsc_ok = True
        self.can_upload_source = False
        self.can_upload_mixed = False

I don't know if those bits are good. Your call.

>> + # XXX: JonathanLange 2010-01-15: We should not be re-using magical
>> + # string literals. Zombie Dijkstra will come and kill us in our sleep.
>
> bug number *cough* :)
>

Added.

>> - buildqueue_record = Attribute("Corespondent BuildQueue record")
>> + buildqueue_record = Attribute("Corresponding BuildQueue record")
>
> I know you're just fixing the text, but we can surely do better than "Attribute".
>

Changed to Object(...)

>> + policy_name = Attribute(
>> +     "The upload policy to use for handling these builds.")
>
> And here!
>

Changed to TextLine(...)

>> +     uploader_argv = list(config.builddmaster.uploader.split())
>
> Can you rename this variable to something like "uploader_command".  The "argv" bit made me look twice since I was expecting only parameters to the command but it's actually the base command from the config.

Ahh ok. FWIW, I used "argv" specifically because it corresponds to
argv arrays in C convention by including the argument.

Changed to uploader_command.

>
>> + # XXX: Duplicated from getUploaderCommand
>>   uploader_logfilename = os.path.join(upload_dir, 'uploader.log')
>
> Why not turn it into a property on the class then?
>

Because it's parametrized -- it depends on upload_leaf. I've made it
an argument to getUploaderCommand.

>> - classProvides(IBuildFarmCandidateJobSelection, ISpecificBuildFarmJobClass)
>> + classProvides(ISpecificBuildFarmJobClass)
>
> Why did you remove that?  I note that it's there in db-devel but not devel.
>

_Probably_ a bad merge. Restored.

>> + # XXX: BuildBase is supposed to implement IBuildBase, but doesn't atm.
>> + # Since it's not the focus of the branch, we'll postpone the work.
>
>
>From the code I just looked at, it does!  If there's a genuine problem then we need a bug please - tag wellington of course.  This is becoming known as the Wellington project I think!

It didn't when I submitted this branch last week, and when I
re-enabled the test, it failed. The problem is genuine, so I've filed
a bug and added it to the comment.

>
> FWIW, I think all classes should have basic doctests, I really don't like this unit testing for their basic features.
>

OK. I haven't added any doctests in this branch.

> What layer is TestBuildBase going to run in if it's not explicitly specified?
>

No layer at all. It doesn't need to be run in a layer. We want more
tests like this, because they are fast :)

>> +class TestBuildBaseHarder(TestCaseWithFactory):
>
> Crappy name alert!  Perhaps "TestBuildBaseWithDatabase" ?
>

Changed.

> Or maybe, s/TestBuildBase/TestBuildBase<layer>/ where <layer> is whatever it's really using (and I think it should be explicit) and then s/TestBuildBaseHarder/TestBuildBase/.  Your call.
>
>> + <adapter factory="lp.soyuz.model.recipebuilder.RecipeBuildBehavior"
>> +     permission="zope.Public" />
>
> You can fill a gap in my knowledge here, what does this do when declared without the "provides" and "for" ?
>

I forwarded you an email on the subject from the Canonical thread.

http://www.muthukadan.net/docs/zca.html#zcml has some other notes.

>> + build_log_url = exported(
>
> Why is this moved from BuildBase to Build?
>

When I was trying to get the assertProvides(BuildBase, IBuildBase)
thing to pass, it turned out that BuildBase didn't implement this, so
I moved the declaration.

>> + # JRV 2010-01-15: The database table really should have a pocket
>> + # column, although this is not a big problem at the moment as recipe
>> + # builds only happen for PPA's (so far). (bug 507307)
>
> Can you XXX this in the usual format please.
>
> I guess I should tell Jelmer how this works too :)
>

Yes. Changed.

>> + distroseries.newArch(
>> +     '386', processorfamily, True, self.factory.makePerson())
>
> Nitpick, but s/386/i386/ for the architecture tag.
>

Fixed.

> Looking good!  Can you answer my queries above please and then we'll get this puppy landed.
>

Cool.

I look forward to hearing back from you.

jml