Hi Brad, This is a really good review. Thanks for that! It does show that I was past my saturation point for this branch, doesn't it? > Running your test command (was that just 'make check' in disguise?) I > got a failure on test_dispatchBuildToSlave. Hmm... my EC2 test came through clean though, as do my manual runs. You don't happen to have a log of this failure somewhere, do you? I wouldn't normally ask, but IIRC you were the one who wrote the retest tool, so you may have a more reliable habit of keeping them. > 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. Alright. Sorry to put you to the trouble; this is indeed very hard work and it's been slowing me down no end. If you have any notes at all about what made it hard, please let me know so I can improve the documentation. I believe it's important to us to make this accessible to encourage development. > > === 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. You're right, of course. This is a missing final cleanup after various refactorings. I put the interface checks in separate "get {IBranch,IBuildBase} for this job if it has one" methods to isolate the complexity. > > + 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. TBH I still don't see why these security classes don't work like that in the first place. What could be more appropriate for unauthenticated access than having the user be None? The change does introduce an "if user is None" deeper down the call tree when it comes to delegating to another permissions object, but it's still better than keeping the methods separate all the way from top to bottom. > > + def checkUnauthenticated(self): > > + if not self._checkBranchVisibility(): > > + return False > > + return self._getBuildPermission().checkUnauthenticated() > > Again I find this implementation confusing. Right you are. It's all different now. > > === 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. This for a change is not laziness on my part; it's a reference to the "fmt" in TAL formatters. > > === 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? I think I just had too much on my plate and forgot to finish the docstring. Fixed. > > === 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. I used to use that all the time, but I remember seeing a deprecation warning about it at some point and that's why I backed away from it. If it's something we're supposed to use, I'll happily go back to it. > > @@ -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? Because it's a @property and so may be costly. I made the code further down use it. > > === 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? This test has much, much bigger problems. The long, convoluted test interactions make it impossible to see individual reasons and intentions; it's just a fossilized computation outcome. Touch-ups are to be applied with extreme care; I took a risk as it is. The cost and difficulty of test setup, as well as the buildmaster's Soyuz provenance, make it hard to break away from this testing style. The ideal setup would isolate the prioritization and estimation logic from the database, and unit-test the living daylights out of it. In practice, that may be hard to achieve. The core of the thing is in SQL, and for relatively good reasons. Tim has suggested running queue simulations instead of a pure computational approach. I think he's right (when he suggested it I assumed performance was the limitation, but he assured me it isn't). I have a feeling that the rewrite will come eventually, and that would be the perfect time to rip out these tests and replace them with more manageable ones. > > === 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. Fixed. > > === 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 @@ > > + > + 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] > > Can you not use your new formatter here? Alas, no. Different class hierarchy altogether. It's mightily, and to a large degree unnecessarily, complex: http://people.ubuntu.com/~wgrant/launchpad/buildfarm/current-build-model.png One does wonder though whether we should not be a bit more liberal with formatters: we have fmt:link and fmt:icon and so on, but why not a fmt:summary or fmt:current-build? > > === 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 @@ > > + > + 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] > > And here. Same. I had to check though. You can get lost in this forest of classes. Jeroen