Björn Tillenius wrote: > Review: Needs Information > On Mon, Jan 11, 2010 at 03:08:17AM -0000, Muharem Hrnjadovic wrote: >> === modified file 'lib/lp/buildmaster/manager.py' >> --- lib/lp/buildmaster/manager.py 2009-07-26 14:19:49 +0000 >> +++ lib/lp/buildmaster/manager.py 2010-01-11 03:08:16 +0000 >> @@ -340,17 +340,9 @@ >> transaction.commit() >> continue >> >> - candidate = builder.findBuildCandidate() >> - if candidate is None: >> - self.logger.debug( >> - "No build candidates available for builder.") >> - continue >> - >> slave = RecordingSlave(builder.name, builder.url, builder.vm_host) >> - builder.setSlaveForTesting(slave) >> - >> - builder.dispatchBuildCandidate(candidate) >> - if builder.currentjob is not None: >> + candidate = builder.findAndStartJob(buildd_slave=slave) >> + if candidate is not None: >> recording_slaves.append(slave) >> transaction.commit() > > Here you changed it to commit when no candidates are found... Good catch! I have indented the commit() line above to preserve the old behaviour. >> === modified file 'lib/lp/soyuz/scripts/buildd.py' >> --- lib/lp/soyuz/scripts/buildd.py 2009-10-26 18:40:04 +0000 >> +++ lib/lp/soyuz/scripts/buildd.py 2010-01-11 03:08:16 +0000 >> @@ -201,12 +201,10 @@ >> if not builder.is_available: >> self.logger.warn('builder is not available. Ignored.') >> continue >> - candidate = builder.findBuildCandidate() >> + >> + candidate = builder.findAndStartJob() >> if candidate is None: >> - self.logger.debug( >> - "No candidates available for builder.") >> continue >> - builder.dispatchBuildCandidate(candidate) >> self.txn.commit() > > ... but here you retain the behaviour of not commiting when no candidate > is found. Why was one place changed, but not the other? The issue above is fixed now. >> === modified file 'lib/lp/soyuz/tests/test_builder.py' >> --- lib/lp/soyuz/tests/test_builder.py 2009-12-02 15:18:46 +0000 >> +++ lib/lp/soyuz/tests/test_builder.py 2010-01-11 03:08:16 +0000 > >> @@ -34,13 +35,15 @@ >> # Create some i386 builders ready to build PPA builds. Two >> # already exist in sampledata so we'll use those first. >> self.builder1 = getUtility(IBuilderSet)['bob'] >> - self.builder2 = getUtility(IBuilderSet)['frog'] >> + self.frog_builder = removeSecurityProxy( >> + getUtility(IBuilderSet)['frog']) > > Why doesn't frog_builder have a security proxy? If it's just to call > private methods, it's better to remove the proxy when you call the > method. If you start passing frog_builder to methods in your tests, you > might get things passing in the tests, while failing in real code. > Better to be safe and only unwrap the objects when you know it's safe. Good point! Revised the code accordingly. >> self.builder3 = self.factory.makeBuilder(name='builder3') >> - self.builder4 = self.factory.makeBuilder(name='builder4') >> + self.builder4 = removeSecurityProxy( >> + self.factory.makeBuilder(name='builder4')) > > Same comment here. Yup. Fixed. >> self.builder5 = self.factory.makeBuilder(name='builder5') >> self.builders = [ >> self.builder1, >> - self.builder2, >> + self.frog_builder, >> self.builder3, >> self.builder4, >> self.builder5, >> @@ -62,7 +65,8 @@ >> self.publisher.prepareBreezyAutotest() >> >> self.bob_builder = getUtility(IBuilderSet)['bob'] >> - self.frog_builder = getUtility(IBuilderSet)['frog'] >> + self.frog_builder = removeSecurityProxy( >> + getUtility(IBuilderSet)['frog']) > > And here. Fixed as well. > review needsinformation Please see the enclosed incremental diff. Best regards -- Muharem Hrnjadovic