Merge into db-devel : 567922-binarypackagebuild-new-table-2 : Code : Launchpad itself

Status:

Merged

Approved by:

Michael Nelson on 2010-05-06

Approved revision:

no longer in the source branch.

Merged at revision:

9405

Proposed branch:

lp://staging/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-2

Merge into:

lp://staging/launchpad/db-devel

Prerequisite:

lp://staging/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-1

Diff against target:

756 lines (+188/-86) (has conflicts)

17 files modified

lib/lp/buildmaster/interfaces/buildbase.py (+7/-1)
lib/lp/buildmaster/interfaces/packagebuild.py (+7/-0)
lib/lp/buildmaster/model/buildbase.py (+6/-5)
lib/lp/buildmaster/model/buildfarmjob.py (+1/-1)
lib/lp/buildmaster/model/packagebuild.py (+11/-4)
lib/lp/code/model/sourcepackagerecipe.py (+1/-1)
lib/lp/code/tests/test_sourcepackagerecipebuild.py (+1/-1)
lib/lp/soyuz/interfaces/binarypackagebuild.py (+2/-1)
lib/lp/soyuz/model/binarypackagebuild.py (+48/-29)
lib/lp/soyuz/model/buildfarmbuildjob.py (+50/-0)
lib/lp/soyuz/model/buildpackagejob.py (+7/-7)
lib/lp/soyuz/model/publishing.py (+1/-1)
lib/lp/soyuz/model/sourcepackagerelease.py (+15/-8)
lib/lp/soyuz/tests/test_binarypackagebuild.py (+11/-9)
lib/lp/soyuz/tests/test_hasbuildrecords.py (+3/-1)
lib/lp/soyuz/tests/test_publishing.py (+14/-14)
lib/lp/soyuz/tests/test_publishing_models.py (+3/-3)

Text conflict in lib/lp/buildmaster/interfaces/buildbase.py
Text conflict in lib/lp/buildmaster/model/buildbase.py
Text conflict in lib/lp/buildmaster/tests/test_buildbase.py

To merge this branch:

bzr merge lp://staging/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-2

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Brad Crittenden (community)	code	2010-05-04	Approve on 2010-05-04
Review via email: mp+24688@code.staging.launchpad.net

Description of the change

This branch is part of a pipeline for

https://blueprints.edge.launchpad.net/soyuz/+spec/build-generalisation
https://dev.launchpad.net/LEP/GeneralBuildHistories

Overview
========
This branch continues the work to switch our BinaryPackageBuild class to the new binarypackagebuild table (using the delegated PackageBuild/BuildFarmJob).

Details
=======
Renames test_build to test_binarypackagebuild, adds BinaryPackageBuild.queueBuild (converting it temporarily to a static method so the implementation can be shared with BuildBase until BuildBase is later deleted), and basically renames lots of attributes to get the tests below to pass.

This branch is dependent on the pending schema patch in a previous branch.

To test
=======

First update the test db schema (required as the db patch still needs to be updated to remove the old build table):
psql launchpad_ftest_template -f database/schema/pending/michaeln-build-generalisation.sql
bin/py database/schema/security.py -d launchpad_ftest_template

And then:
bin/test -vv -m test_binarypackagebuild -t test_getBuildsForBuilder -t test_getBuildsForArchive -t test_providesInterface

The next branch will continue to update the BinaryPackageBuild implementation to get the rest of the tests working.

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2010-05-04:

#

Diff here: http://pastebin.ubuntu.com/427743/ (in case the scanner is still catching up).

Revision history for this message

Brad Crittenden (bac) wrote on 2010-05-04:

#

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

> === 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.

>
> 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.)

> === 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?

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

> === 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.

>  
>  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.)

> === 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?

review: Approve (code)

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2010-05-05:

#

Download full text (5.4 KiB)

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 r...

On Tue, May 4, 2010 at 7:52 PM, Brad Crittenden <bac@canonical.com> 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

Launchpad itself

Merge lp://staging/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-2 into lp://staging/launchpad/db-devel

Commit message

Description of the change

Preview Diff

Subscribers