Hi Jeroen,

Thanks for the nice agrarian branch.

Running your test command (was that just 'make check' in disguise?) I
got a failure on test_dispatchBuildToSlave.

Jeroen I tried on two different occasions to follow your instructions
in order to exercise the new UI.  Both times were very time consuming
and ultimately failed.  I give up.  I approve the code but not the UI
-- you needed to get a separate UI review anyway but as a code
reviewer I like to have a once-over.  

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py	2010-03-12 08:59:36 +0000
> +++ lib/canonical/launchpad/security.py	2010-03-16 11:13:21 +0000
> @@ -1505,6 +1508,41 @@
>          return auth_spr.checkUnauthenticated()
>  
>  
> +class ViewBuildFarmJob(AuthorizationBase):
> +    """Permission to view an `IBuildFarmJob`.
> +
> +    This permission is based entirely on permission to view the
> +    associated `IBuild` and/or `IBranch`.
> +    """
> +    permission = 'launchpad.View'
> +    usedfor = IBuildFarmJob
> +
> +    def _getBuildPermission(self):
> +        return ViewBuildRecord(self.obj.build)
> +
> +    def _checkBranchVisibility(self, user=None):
> +        """Is the user free to view any branches associated with this job?"""
> +        if not IBuildFarmBranchJob.providedBy(self.obj):
> +            return True
> +        else:
> +            return self.obj.branch.visibleByUser(user)

I don't like the inverted test (our coding standards recommend against
having a 'not' test if both paths are present) and the fact a return
value of True has dual meanings.

> +    def checkAuthenticated(self, user):
> +        if not self._checkBranchVisibility(user):
> +            return False
> +
> +        if IBuildFarmBuildJob.providedBy(self.obj):
> +            if not self._getBuildPermission().checkAuthenticated(user):
> +                return False
> +
> +        return True

Could you write a single method that provides checkAuthenticated and
checkUnauthenticated for your two possiblities?  A nice little wrapper
would make the real methods more readable.

> +    def checkUnauthenticated(self):
> +        if not self._checkBranchVisibility():
> +            return False
> +        return self._getBuildPermission().checkUnauthenticated()

Again I find this implementation confusing.

> +
>  class AdminQuestion(AdminByAdminsTeam):
>      permission = 'launchpad.Admin'
>      usedfor = IQuestion

> === modified file 'lib/canonical/launchpad/webapp/tales.py'
> --- lib/canonical/launchpad/webapp/tales.py	2010-03-10 12:50:18 +0000
> +++ lib/canonical/launchpad/webapp/tales.py	2010-03-16 11:13:21 +0000
> @@ -1514,6 +1514,33 @@
>          return url
>  
>  
> +class BuildBaseFormatterAPI(ObjectFormatterAPI):
> +    """Adapter providing fmt support for `IBuildBase` objects."""

You have plenty of room so please spell out formatter.

> +    def _composeArchiveReference(self, archive):
> +        if archive.is_ppa:
> +            return " [%s/%s]" % (
> +                cgi.escape(archive.owner.name), cgi.escape(archive.name))
> +        else:
> +            return ""
> +
> +    def icon(self, view_name):
> +        if not check_permission('launchpad.View', self._context):
> +            return '<img src="/@@/processing" alt="[build]" />'
> +
> +        return BuildImageDisplayAPI(self._context).icon()
> +
> +    def link(self, view_name, rootsite=None):
> +        icon = self.icon(view_name)
> +        build = self._context
> +        if not check_permission('launchpad.View', build):
> +            return '%s private source' % icon
> +
> +        url = self.url(view_name=view_name, rootsite=rootsite)
> +        title = cgi.escape(build.title)
> +        archive = self._composeArchiveReference(build.archive)
> +        return '<a href="%s">%s%s</a>%s' % (url, icon, title, archive)
> +
> +
>  class CodeImportMachineFormatterAPI(CustomizableFormatter):
>      """Adapter providing fmt support for CodeImport objects"""
>  

> === added file 'lib/lp/buildmaster/interfaces/buildfarmbranchjob.py'
> --- lib/lp/buildmaster/interfaces/buildfarmbranchjob.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/buildmaster/interfaces/buildfarmbranchjob.py	2010-03-16 11:13:21 +0000
> @@ -0,0 +1,21 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Interface."""

c-n-p leftover?

> +__metaclass__ = type
> +__all__ = [
> +    'IBuildFarmBranchJob'
> +    ]
> +
> +from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
> +from lp.code.interfaces.branchjob import IBranchJob
> +
> +
> +class IBuildFarmBranchJob(IBuildFarmJob, IBranchJob):
> +    """An `IBuildFarmJob` that's also an `IBranchJob`.
> +
> +    Use this interface for `IBuildFarmJob` implementations that do not
> +    have a "build" attribute but do implement `IBranchJob`, so that the
> +    UI can render appropriate status information.
> +    """

> === modified file 'lib/lp/buildmaster/interfaces/buildqueue.py'
> --- lib/lp/buildmaster/interfaces/buildqueue.py	2010-03-05 13:52:32 +0000
> +++ lib/lp/buildmaster/interfaces/buildqueue.py	2010-03-16 11:13:21 +0000
> @@ -72,6 +72,10 @@
>          title=_("Estimated Job Duration"), required=True,
>          description=_("Estimated job duration interval."))
>  
> +    current_build_duration = Timedelta(
> +        title=_("Current build duration"), required=False,
> +        description=_("Time spent building so far."))
> +
>      def manualScore(value):
>          """Manually set a score value to a queue item and lock it."""
>  

> === modified file 'lib/lp/buildmaster/model/buildqueue.py'
> --- lib/lp/buildmaster/model/buildqueue.py	2010-03-08 12:53:59 +0000
> +++ lib/lp/buildmaster/model/buildqueue.py	2010-03-16 11:13:21 +0000
> @@ -14,6 +14,7 @@
>  from collections import defaultdict
>  from datetime import datetime, timedelta
>  import logging
> +from pytz import timezone

   import pytz

>  
>  from sqlobject import (
>      StringCol, ForeignKey, BoolCol, IntCol, IntervalCol, SQLObjectNotFound)
> @@ -37,6 +38,9 @@
>  from lp.soyuz.model.buildpackagejob import BuildPackageJob
>  
>  
> +UTC = timezone('UTC')

One of my Quixotic missions is to get people to just use pytz.UTC.
Stuart was oh so helpful in providing that symbol for us I think we
should take advantage of it.  Won't you give it a try?  If so, this
statement can be removed. 

> +
>  def normalize_virtualization(virtualized):
>      """Jobs with NULL virtualization settings should be treated the
>         same way as virtualized jobs."""
> @@ -118,6 +122,15 @@
>          """See `IBuildQueue`."""
>          return self.job.date_started
>  
> +    @property
> +    def current_build_duration(self):
> +        """See `IBuildQueue`."""
> +        date_started = self.date_started

Why the local copy, which you don't even use later?

> +        if date_started is None:
> +            return None
> +        else:
> +            return self._now() - self.date_started
> +
>      def destroySelf(self):
>          """Remove this record and associated job/specific_job."""
>          job = self.job
> @@ -464,8 +477,8 @@
>  
>      @staticmethod
>      def _now():
> -        """Provide utcnow() while allowing test code to monkey-patch this."""
> -        return datetime.utcnow()
> +        """Return current time (UTC).  Overridable for test purposes."""
> +        return datetime.now(UTC)


        return datetime.now(pytz.UTC)

Thanks for the fix here and the explanation.  I didn't realize
there was a difference.  YLSNED.
  	   	 
>  
>  class BuildQueueSet(object):

> === modified file 'lib/lp/buildmaster/tests/test_buildqueue.py'
> --- lib/lp/buildmaster/tests/test_buildqueue.py	2010-03-10 21:30:57 +0000
> +++ lib/lp/buildmaster/tests/test_buildqueue.py	2010-03-16 11:13:21 +0000
> @@ -6,13 +6,14 @@
>  
>  from datetime import datetime, timedelta
>  from pytz import utc
> +from unittest import TestLoader
>  
>  from zope.component import getUtility
>  from zope.interface.verify import verifyObject
>  
>  from canonical.launchpad.webapp.interfaces import (
>      IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
> -from canonical.testing import LaunchpadZopelessLayer
> +from canonical.testing import LaunchpadZopelessLayer, ZopelessDatabaseLayer
>  
>  from lp.buildmaster.interfaces.buildbase import BuildStatus
>  from lp.buildmaster.interfaces.builder import IBuilderSet
> @@ -28,6 +29,7 @@
>  from lp.soyuz.model.build import Build
>  from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
>  from lp.testing import TestCaseWithFactory
> +from lp.testing.fakemethod import FakeMethod
>  
>  
>  def find_job(test, name, processor='386'):
> @@ -140,10 +142,12 @@
>      This avoids spurious test failures.
>      """
>      # Use the date/time the job started if available.
> -    time_stamp = buildqueue.job.date_started
> -    if not time_stamp:
> -        time_stamp = datetime.now(utc)
> -    buildqueue._now = lambda: time_stamp
> +    if buildqueue.job.date_started:
> +        time_stamp = buildqueue.job.date_started
> +    else:
> +        time_stamp = buildqueue._now()
> +
> +    buildqueue._now = FakeMethod(result=time_stamp)
>      return time_stamp


Other tests in this module don't use _now() but use the real,
unmonkeypatched time.  Would you give a look at them and see if they
should be updated?
>  
> @@ -209,7 +213,6 @@
>              "An active build job must have a builder.")
>  
>  
> -
>  class TestBuildQueueBase(TestCaseWithFactory):
>      """Setup the test publisher and some builders."""
>  
> @@ -605,6 +608,7 @@
>          # fact that no builders capable of running the job are available.
>          check_mintime_to_builder(self, job, 0)
>  
> +
>  class MultiArchBuildsBase(TestBuildQueueBase):
>      """Set up a test environment with builds and multiple processors."""
>      def setUp(self):
> @@ -805,6 +809,27 @@
>          check_mintime_to_builder(self, apg_job, 0)
>  
>  
> +class TestBuildQueueDuration(TestCaseWithFactory):
> +    layer = ZopelessDatabaseLayer
> +
> +    def _makeBuildQueue(self):
> +        """Produce a `BuildQueue` object to test."""
> +        return self.factory.makeSourcePackageRecipeBuildJob()
> +
> +    def test_current_build_duration_not_started(self):
> +        buildqueue = self._makeBuildQueue()
> +        self.assertEqual(None, buildqueue.current_build_duration)
> +
> +    def test_current_build_duration(self):
> +        buildqueue = self._makeBuildQueue()
> +        now = buildqueue._now()
> +        buildqueue._now = FakeMethod(result=now)
> +        age = timedelta(minutes=3)
> +        buildqueue.job.date_started = now - age
> +
> +        self.assertEqual(age, buildqueue.current_build_duration)
> +
> +
>  class TestJobClasses(TestCaseWithFactory):
>      """Tests covering build farm job type classes."""
>      layer = LaunchpadZopelessLayer
> @@ -1301,3 +1326,7 @@
>          assign_to_builder(self, 'xxr-daptup', 1, None)
>          postgres_build, postgres_job = find_job(self, 'postgres', '386')
>          check_estimate(self, postgres_job, 120)
> +
> +
> +def test_suite():
> +    return TestLoader().loadTestsFromName(__name__)

> === modified file 'lib/lp/soyuz/browser/build.py'
> --- lib/lp/soyuz/browser/build.py	2010-03-10 13:28:55 +0000
> +++ lib/lp/soyuz/browser/build.py	2010-03-16 11:13:21 +0000
> @@ -525,4 +525,3 @@
>      @property
>      def no_results(self):
>          return self.form_submitted and not self.complete_builds
> -

> === modified file 'lib/lp/soyuz/browser/configure.zcml'
> --- lib/lp/soyuz/browser/configure.zcml	2010-03-08 05:30:13 +0000
> +++ lib/lp/soyuz/browser/configure.zcml	2010-03-16 11:13:21 +0000
> @@ -1,4 +1,4 @@
> -<!-- Copyright 2009 Canonical Ltd.  This software is licensed under the
> +<!-- Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
>       GNU Affero General Public License version 3 (see the file LICENSE).
>  -->
>  
> @@ -798,4 +798,34 @@
>          path_expression="string:+dependency/${dependency/id}"
>          attribute_to_parent="archive"
>          />
> +
> +    <!-- XXX JeroenVermeulen 2010-03-13 bug=539395: This is
> buildmaster, not soyuz -->

Thanks for the XXX.

> +    <browser:page
> +        for="lp.buildmaster.interfaces.buildqueue.IBuildQueue"
> +        name="+current"
> +        class="canonical.launchpad.webapp.publisher.LaunchpadView"
> +        facet="overview"
> +        permission="launchpad.View"
> +        template="../templates/buildqueue-current.pt"/>
> +    <browser:page
> +        for="lp.buildmaster.interfaces.buildqueue.IBuildFarmJob"
> +        name="+current"
> +        class="canonical.launchpad.webapp.publisher.LaunchpadView"
> +        facet="overview"
> +        permission="launchpad.View"
> +        template="../templates/buildfarmjob-current.pt"/>
> +    <browser:page
> +        for="lp.buildmaster.interfaces.buildfarmbranchjob.IBuildFarmBranchJob"
> +        name="+current"
> +        class="canonical.launchpad.webapp.publisher.LaunchpadView"
> +        facet="overview"
> +        permission="launchpad.View"
> +        template="../templates/buildfarmbranchjob-current.pt"/>
> +    <browser:page
> +        for="lp.soyuz.interfaces.buildfarmbuildjob.IBuildFarmBuildJob"
> +        name="+current"
> +        class="canonical.launchpad.webapp.publisher.LaunchpadView"
> +        facet="overview"
> +        permission="launchpad.View"
> +        template="../templates/buildfarmbuildjob-current.pt"/>
>  </configure>

> === modified file 'lib/lp/soyuz/doc/build-estimated-dispatch-time.txt'
> --- lib/lp/soyuz/doc/build-estimated-dispatch-time.txt	2010-03-06 04:57:40 +0000
> +++ lib/lp/soyuz/doc/build-estimated-dispatch-time.txt	2010-03-16 11:13:21 +0000
> @@ -89,7 +89,7 @@
>  The estimated start time for the pending job is either now or lies
>  in the future.
>  
> -    >>> now = datetime.utcnow()
> +    >>> now = datetime.now(UTC)

As above.

>      >>> def job_start_estimate(build):
>      ...     return build.buildqueue_record.getEstimatedJobStartTime()
>      >>> estimate = job_start_estimate(alsa_build)
> @@ -147,7 +147,7 @@
>  Since the 'iceweasel' build has a higher score (666) than the 'pmount'
>  build (66) its estimated dispatch time is essentially "now".
>  
> -    >>> now = datetime.utcnow()
> +    >>> now = datetime.now(UTC)
>      >>> estimate = job_start_estimate(iceweasel_build)
>      >>> estimate > now
>      True

> === added file 'lib/lp/soyuz/templates/buildfarmbranchjob-current.pt'
> --- lib/lp/soyuz/templates/buildfarmbranchjob-current.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/templates/buildfarmbranchjob-current.pt	2010-03-16 11:13:21 +0000
> @@ -0,0 +1,11 @@
> +<tal:root
> +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +  omit-tag="">
> +
> +  <img src="/@@/processing" alt="[building]" />

Can you not use your new formatter here?

> +  Working on
> +  <tal:jobtype replace="context/__class__/__name__" />
> +  for branch <tal:branch replace="structure context/branch/fmt:link" />.
> +</tal:root>

> === added file 'lib/lp/soyuz/templates/buildfarmbuildjob-current.pt'
> --- lib/lp/soyuz/templates/buildfarmbuildjob-current.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/templates/buildfarmbuildjob-current.pt	2010-03-16 11:13:21 +0000
> @@ -0,0 +1,8 @@
> +<tal:root
> +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +  omit-tag="">
> +
> +  Building <tal:build replace="structure context/build/fmt:link" />
> +</tal:root>

> === added file 'lib/lp/soyuz/templates/buildfarmjob-current.pt'
> --- lib/lp/soyuz/templates/buildfarmjob-current.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/templates/buildfarmjob-current.pt	2010-03-16 11:13:21 +0000
> @@ -0,0 +1,10 @@
> +<tal:root
> +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +  omit-tag="">
> +
> +  <img src="/@@/processing" alt="[building]" />

And here.

> +  Working on
> +  <tal:jobtype replace="context/__class__/__name__" />.
> +</tal:root>