launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1058310 into lp:launchpad.

Commit message:
Rework BinaryPackageBuild scoring: language packs now use the archive base score, packageset scores no longer apply to PPAs, and age-based scoring is gone.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #690834 in Launchpad itself: "Language packages in P3As are also scored 0"
  https://bugs.launchpad.net/launchpad/+bug/690834
  Bug #810716 in Launchpad itself: "Translations section builds have highest priority in copy builds"
  https://bugs.launchpad.net/launchpad/+bug/810716
  Bug #1058310 in Launchpad itself: "PPA builds get packageset build score bump"
  https://bugs.launchpad.net/launchpad/+bug/1058310

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1058310/+merge/127414

This branch reworks BinaryPackageBuild scoring a little.

Language packs now get the base archive score, rather than always 0 (for the primary archive and public PPAs they'll still be 0, but those in private PPAs and copy archives will now be correctly scored up and down respectively).

Packageset-based score adjustments no longer apply to PPAs, just primary/partner/copy. It's possible we'll also want to adjust copy archives later, but it's not critical either way for now.

I also took the opportunity to eliminate age-based scoring. It's counter-intuitive, the tiebreaker is id (a rather good proxy for age) anyway, and we don't run queue-builder any more so the score is never calculated except at age=0.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1058310/+merge/127414
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1058310 into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py	2012-07-20 08:29:06 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py	2012-10-02 04:50:25 +0000
@@ -7,9 +7,6 @@
     ]
 
 
-from datetime import datetime
-
-import pytz
 from storm.locals import (
     Int,
     Reference,
@@ -76,25 +73,22 @@
 
     def score(self):
         """See `IBuildPackageJob`."""
-        # Define a table we'll use to calculate the score based on the time
-        # in the build queue.  The table is a sorted list of (upper time
-        # limit in seconds, score) tuples.
-        queue_time_scores = [
-            (14400, 100),
-            (7200, 50),
-            (3600, 20),
-            (1800, 15),
-            (900, 10),
-            (300, 5),
-        ]
-
-        # Please note: the score for language packs is to be zero because
-        # they unduly delay the building of packages in the main component
-        # otherwise.
+        score = 0
+
+        # Private builds get uber score.
+        if self.build.archive.private:
+            score += PRIVATE_ARCHIVE_SCORE_BONUS
+
+        if self.build.archive.is_copy:
+            score -= COPY_ARCHIVE_SCORE_PENALTY
+
+        score += self.build.archive.relative_build_score
+
+        # Language packs don't get any of the usual package-specific
+        # score bumps, as they unduly delay the building of packages in
+        # the main component otherwise.
         if self.build.source_package_release.section.name == 'translations':
-            return 0
-
-        score = 0
+            return score
 
         # Calculates the urgency-related part of the score.
         score += SCORE_BY_URGENCY[self.build.source_package_release.urgency]
@@ -110,29 +104,9 @@
         package_sets = getUtility(IPackagesetSet).setsIncludingSource(
             self.build.source_package_release.name,
             distroseries=self.build.distro_series)
-        if not package_sets.is_empty():
+        if not self.build.archive.is_ppa and not package_sets.is_empty():
             score += package_sets.max(Packageset.relative_build_score)
 
-        # Calculates the build queue time component of the score.
-        right_now = datetime.now(pytz.timezone('UTC'))
-        eta = right_now - self.job.date_created
-        for limit, dep_score in queue_time_scores:
-            if eta.seconds > limit:
-                score += dep_score
-                break
-
-        # Private builds get uber score.
-        if self.build.archive.private:
-            score += PRIVATE_ARCHIVE_SCORE_BONUS
-
-        if self.build.archive.is_copy:
-            score -= COPY_ARCHIVE_SCORE_PENALTY
-
-        # Lastly, apply the archive score delta.  This is to boost
-        # or retard build scores for any build in a particular
-        # archive.
-        score += self.build.archive.relative_build_score
-
         return score
 
     def getLogFileName(self):

=== modified file 'lib/lp/soyuz/tests/test_buildpackagejob.py'
--- lib/lp/soyuz/tests/test_buildpackagejob.py	2012-09-28 06:25:44 +0000
+++ lib/lp/soyuz/tests/test_buildpackagejob.py	2012-10-02 04:50:25 +0000
@@ -246,23 +246,20 @@
     layer = DatabaseFunctionalLayer
 
     def makeBuildJob(self, purpose=None, private=False, component="main",
-                     urgency="high", pocket="RELEASE", age=None):
+                     urgency="high", pocket="RELEASE", section_name=None):
         if purpose is not None or private:
             archive = self.factory.makeArchive(
                 purpose=purpose, private=private)
         else:
             archive = None
         spph = self.factory.makeSourcePackagePublishingHistory(
-            archive=archive, component=component, urgency=urgency)
+            archive=archive, component=component, urgency=urgency,
+            section_name=section_name)
         naked_spph = removeSecurityProxy(spph)  # needed for private archives
         build = self.factory.makeBinaryPackageBuild(
             source_package_release=naked_spph.sourcepackagerelease,
             pocket=pocket)
-        job = removeSecurityProxy(build).makeJob()
-        if age is not None:
-            removeSecurityProxy(job).job.date_created = (
-                datetime.now(pytz.timezone("UTC")) - timedelta(seconds=age))
-        return job
+        return removeSecurityProxy(build).makeJob()
 
     # The defaults for pocket, component, and urgency here match those in
     # makeBuildJob.
@@ -320,32 +317,29 @@
             job, "RELEASE", "main", "high", PRIVATE_ARCHIVE_SCORE_BONUS)
 
     def test_main_release_low_recent_score(self):
-        # Builds created less than five minutes ago get no bonus.
-        job = self.makeBuildJob(component="main", urgency="low", age=290)
+        # 1500 (RELEASE) + 1000 (main) + 5 (low) = 2505.
+        job = self.makeBuildJob(component="main", urgency="low")
         self.assertCorrectScore(job, "RELEASE", "main", "low")
 
     def test_universe_release_high_five_minutes_score(self):
-        # 1500 (RELEASE) + 250 (universe) + 15 (high) + 5 (>300s) = 1770.
-        job = self.makeBuildJob(component="universe", urgency="high", age=310)
-        self.assertCorrectScore(job, "RELEASE", "universe", "high", 5)
+        # 1500 (RELEASE) + 250 (universe) + 15 (high) = 1765.
+        job = self.makeBuildJob(component="universe", urgency="high")
+        self.assertCorrectScore(job, "RELEASE", "universe", "high")
 
     def test_multiverse_release_medium_fifteen_minutes_score(self):
-        # 1500 (RELEASE) + 0 (multiverse) + 10 (medium) + 10 (>900s) = 1520.
-        job = self.makeBuildJob(
-            component="multiverse", urgency="medium", age=1000)
-        self.assertCorrectScore(job, "RELEASE", "multiverse", "medium", 10)
+        # 1500 (RELEASE) + 0 (multiverse) + 10 (medium) = 1510.
+        job = self.makeBuildJob(component="multiverse", urgency="medium")
+        self.assertCorrectScore(job, "RELEASE", "multiverse", "medium")
 
     def test_main_release_emergency_thirty_minutes_score(self):
-        # 1500 (RELEASE) + 1000 (main) + 20 (emergency) + 15 (>1800s) = 2535.
-        job = self.makeBuildJob(
-            component="main", urgency="emergency", age=1801)
-        self.assertCorrectScore(job, "RELEASE", "main", "emergency", 15)
+        # 1500 (RELEASE) + 1000 (main) + 20 (emergency) = 2520.
+        job = self.makeBuildJob(component="main", urgency="emergency")
+        self.assertCorrectScore(job, "RELEASE", "main", "emergency")
 
     def test_restricted_release_low_one_hour_score(self):
-        # 1500 (RELEASE) + 750 (restricted) + 5 (low) + 20 (>3600s) = 2275.
-        job = self.makeBuildJob(
-            component="restricted", urgency="low", age=4000)
-        self.assertCorrectScore(job, "RELEASE", "restricted", "low", 20)
+        # 1500 (RELEASE) + 750 (restricted) + 5 (low) = 2255.
+        job = self.makeBuildJob(component="restricted", urgency="low")
+        self.assertCorrectScore(job, "RELEASE", "restricted", "low")
 
     def test_backports_score(self):
         # BACKPORTS is the lowest-priority pocket.
@@ -374,7 +368,10 @@
         self.assertCorrectScore(job, "SECURITY")
 
     def test_score_packageset(self):
-        job = self.makeBuildJob(component="main", urgency="low")
+        # Package sets alter the score of official packages for their
+        # series.
+        job = self.makeBuildJob(
+            component="main", urgency="low", purpose=ArchivePurpose.PRIMARY)
         packageset = self.factory.makePackageset(
             distroseries=job.build.distro_series)
         removeSecurityProxy(packageset).add(
@@ -382,6 +379,25 @@
         removeSecurityProxy(packageset).relative_build_score = 100
         self.assertCorrectScore(job, "RELEASE", "main", "low", 100)
 
+    def test_score_packageset_in_ppa(self):
+        # Package set score boosts don't affect PPA packages.
+        job = self.makeBuildJob(
+            component="main", urgency="low", purpose=ArchivePurpose.PPA)
+        packageset = self.factory.makePackageset(
+            distroseries=job.build.distro_series)
+        removeSecurityProxy(packageset).add(
+            [job.build.source_package_release.sourcepackagename])
+        removeSecurityProxy(packageset).relative_build_score = 100
+        self.assertCorrectScore(job, "RELEASE", "main", "low", 0)
+
+    def test_translations_score(self):
+        # Language packs (the translations section) don't get any
+        # package-specific score bumps. They always have the archive's
+        # base score.
+        job = self.makeBuildJob(section_name='translations')
+        removeSecurityProxy(job.build.archive).relative_build_score = 666
+        self.assertEqual(666, job.score())
+
     def assertScoreReadableByAnyone(self, obj):
         """An object's build score is readable by anyone."""
         with person_logged_in(obj.owner):