Code review comment for lp:~michael.nelson/launchpad/567922-binarypackagebuild-new-table-2

On Tue, May 4, 2010 at 7:52 PM, Brad Crittenden <email address hidden> wrote:
> Review: Approve code
> Hi Michael,
>
> Man I know you're going to be happy to get these changes all wrapped
> up.  Thanks for doing a very tedious series of branches.
>
> I'm glad to see soyuzvariablenames expanded.  thank_you.
>
> --bac

Thanks for the review bac, a few comments below.

>
>
>> === modified file 'lib/lp/buildmaster/interfaces/packagebuild.py'
>> --- lib/lp/buildmaster/interfaces/packagebuild.py     2010-05-04 12:07:35 +0000
>> +++ lib/lp/buildmaster/interfaces/packagebuild.py     2010-05-04 15:29:42 +0000
>> @@ -151,6 +151,13 @@
>>          :param slave_status: A dict as returned by IBuilder.slaveStatus
>>          """
>>
>> +    def queueBuild(suspended=False):
>> +        """Create a BuildQueue entry for this build.
>> +
>> +        :param suspended: Whether the associated `Job` instance should be
>> +            created in a suspended state.
>> +        """
>
> So interface method specifications leave off the 'self' argument but
> as a staticmethod there is no self, so the first argument 'build' is
> dropped?  That's no good.

It's not a static method on PackageBuild... the
PackageBuild.queueBuild() method calls BuildBase.queueBuild() which is
a static method (so the implementation can be shared in the transition
by old BuildBase-based classes and the new PackageBuild classes).

Let me know if that was obvious to you and there is still something
you'd like to see changed?

>
>>
>>  class IPackageBuildSource(Interface):
>>      """A utility of this interface used to create _things_."""
>
>> === modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
>> --- lib/lp/buildmaster/model/buildfarmjob.py  2010-05-04 13:42:25 +0000
>> +++ lib/lp/buildmaster/model/buildfarmjob.py  2010-05-04 15:29:42 +0000
>> @@ -189,7 +189,7 @@
>>      def __init__(self, job_type, status=BuildStatus.NEEDSBUILD,
>>                   processor=None, virtualized=None):
>>          super(BuildFarmJob, self).__init__()
>> -        self.job_type, self.status, self.process, self.virtualized = (
>> +        self.job_type, self.status, self.processor, self.virtualized = (
>>              job_type,
>>              status,
>>              processor,
>
>> === modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
>> --- lib/lp/soyuz/model/binarypackagebuild.py  2010-05-04 09:45:26 +0000
>> +++ lib/lp/soyuz/model/binarypackagebuild.py  2010-05-04 15:29:42 +0000
>
>> @@ -778,7 +791,7 @@
>>
>>          # This code MUST match the logic in the Build security adapter,
>>          # otherwise users are likely to get 403 errors, or worse.
>
> If it MUST match then perhaps it should be refactored and shared?
> (Not a requirement for this branch.)

Yeah, I thought the same thing when I moved that comment.

>
>> === renamed file 'lib/lp/soyuz/tests/test_build.py' => 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
>> --- lib/lp/soyuz/tests/test_build.py  2010-04-29 08:28:30 +0000
>> +++ lib/lp/soyuz/tests/test_binarypackagebuild.py     2010-05-04 15:29:42 +0000
>> @@ -7,6 +7,7 @@
>>
>>  from storm.store import Store
>>  from zope.component import getUtility
>> +from zope.security.proxy import removeSecurityProxy
>>
>>  from canonical.database.constants import UTC_NOW
>>  from canonical.testing import LaunchpadZopelessLayer
>> @@ -81,7 +82,7 @@
>>
>>          [depwait_build] = depwait_source.createMissingBuilds()
>>          depwait_build.buildstate = BuildStatus.MANUALDEPWAIT
>> -        depwait_build.dependencies = 'dep-bin'
>> +        depwait_build.dependencies = u'dep-bin'
>
> Why did you need to convert these strings to unicode?

I meant to mention that on the MP with a question - without the
unicode string I see:

{{{
Error in test testBuildqueueRemoval
(lp.soyuz.tests.test_binarypackagebuild.TestBuildUpdateDependencies)
Traceback (most recent call last):
_StringException: Text attachment: traceback
------------
Traceback (most recent call last):
  File "/home/michael/canonical/lp-sourcedeps/eggs/testtools-0.9.2-py2.5.egg/testtools/runtest.py",
line 128, in _run_user
    return fn(*args)
  File "/home/michael/canonical/lp-sourcedeps/eggs/testtools-0.9.2-py2.5.egg/testtools/testcase.py",
line 368, in _run_test_method
    testMethod()
  File "/home/michael/canonical/lp-branches/current_work/lib/lp/soyuz/tests/test_binarypackagebuild.py",
line 96, in testBuildqueueRemoval
    depwait_build = self._setupSimpleDepwaitContext()
  File "/home/michael/canonical/lp-branches/current_work/lib/lp/soyuz/tests/test_binarypackagebuild.py",
line 85, in _setupSimpleDepwaitContext
    depwait_build.dependencies = 'dep-bin'
  File "/home/michael/canonical/lp-sourcedeps/eggs/lazr.delegates-1.1.0-py2.5.egg/lazr/delegates/_delegates.py",
line 123, in __set__
    setattr(getattr(inst, self.contextvar), self.name, value)
  File "/home/michael/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-x86_64.egg/storm/properties.py",
line 67, in __set__
    obj_info.variables[column].set(value)
  File "/home/michael/canonical/lp-sourcedeps/eggs/storm-0.15danilo_storm_launchpad_r342-py2.5-linux-x86_64.egg/storm/variables.py",
line 394, in parse_set
    % (type(value), value))
TypeError: Expected unicode, found <type 'str'>: 'dep-bin'
------------
}}}

I checked the db table definitions, and the old build.dependencies
column was simply text, as is the new, so I wasn't certain why this
was required. Any ideas? (my only guess is that new tables/columns are
now defaulting to unicode).

Thanks,
Michael