repo: add RepoDownloadsFromChangeSource renderable

Suggestion from Tom to put this away from the step code.
In cunjunction with FlattenList, this works well.

+ some more minor review handling

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>

master/buildbot/steps/source/repo.py

@@ -18,20 +18,21 @@
 
 from twisted.internet import defer, reactor
 
+from buildbot import util
 from buildbot.process import buildstep
 from buildbot.steps.source.base import Source
 from buildbot.interfaces import IRenderable
 from zope.interface import implements
 
 
-class RepoDownloadsFromProperties(object):
+class RepoDownloadsFromProperties(util.ComparableMixin, object):
     implements(IRenderable)
     parse_download_re = (re.compile(r"repo download ([^ ]+) ([0-9]+/[0-9]+)"),
                          re.compile(r"([^ ]+) ([0-9]+/[0-9]+)"),
                          re.compile(r"([^ ]+)/([0-9]+/[0-9]+)"),
                          )
 
-    compare_attrs = ('names')
+    compare_attrs = ('names',)
 
     def __init__(self, names):
         self.names = names
@@ -64,12 +65,34 @@ def parseDownloadProperty(self, s):
         return ret
 
 
+class RepoDownloadsFromChangeSource(util.ComparableMixin, object):
+    implements(IRenderable)
+    compare_attrs = ('codebase',)
+
+    def __init__(self, codebase=None):
+        self.codebase = codebase
+
+    def getRenderingFor(self, props):
+        downloads = []
+        if self.codebase is None:
+            changes = props.getBuild().allChanges()
+        else:
+            changes = props.getBuild().getSourceStamp(self.codebase).changes
+        for change in changes:
+            if ("event.type" in change.properties and
+               change.properties["event.type"] == "patchset-created"):
+                    downloads.append("%s %s/%s" % (change.properties["event.change.project"],
+                                                   change.properties["event.change.number"],
+                                                   change.properties["event.patchSet.number"]))
+        return downloads
+
+
 class Repo(Source):
     """ Class for Repo with all the smarts """
     name = 'repo'
-    renderables = ["manifestUrl", "manifestUrl", "manifestFile", "tarball", "jobs",
+    renderables = ["manifestURL", "manifestFile", "tarball", "jobs",
                    "syncAllBranches", "updateTarballAge", "manifestOverrideUrl",
-                   "repoDownloads", "repoDownloadsFromChangeSource"]
+                   "repoDownloads"]
 
     ref_not_found_re = re.compile(r"fatal: Couldn't find remote ref")
     cherry_pick_error_re = re.compile(r"|".join([r"Automatic cherry-pick failed",
@@ -82,7 +105,7 @@ class Repo(Source):
     mirror_sync_sleep = 60  # wait 1min between retries (thus default total retry time is 10min)
 
     def __init__(self,
-                 manifestUrl=None,
+                 manifestURL=None,
                  manifestBranch="master",
                  manifestFile="default.xml",
                  tarball=None,
@@ -91,11 +114,10 @@ def __init__(self,
                  updateTarballAge=7*24.0*3600.0,
                  manifestOverrideUrl=None,
                  repoDownloads=None,
-                 repoDownloadsFromChangeSource=True,
                  **kwargs):
         """
-        @type  manifestUrl: string
-        @param manifestUrl: The URL which points at the repo manifests repository.
+        @type  manifestURL: string
+        @param manifestURL: The URL which points at the repo manifests repository.
 
         @type  manifestBranch: string
         @param manifestBranch: The manifest branch to check out by default.
@@ -118,54 +140,28 @@ def __init__(self,
 
         @type repoDownloads: list of strings
         @param repoDownloads: optional repo download to perform after the repo sync
-                 repoDownloadsFromChangeSource=False,
 
-        @type repoDownloadsFromChangeSource: boolean
-        @param repoDownloadsFromChangeSource: do we compute the list of repo downloads
-                                              from changeSource information?
         """
-        self.manifestUrl = manifestUrl
+        self.manifestURL = manifestURL
         self.manifestBranch = manifestBranch
         self.manifestFile = manifestFile
         self.tarball = tarball
         self.jobs = jobs
         self.syncAllBranches = syncAllBranches
         self.updateTarballAge = updateTarballAge
         self.manifestOverrideUrl = manifestOverrideUrl
+        if repoDownloads is None:
+            repoDownloads = []
         self.repoDownloads = repoDownloads
-        self.repoDownloadsFromChangeSource = repoDownloadsFromChangeSource
         Source.__init__(self, **kwargs)
 
-        assert self.manifestUrl is not None
+        assert self.manifestURL is not None
 
     def computeSourceRevision(self, changes):
         if not changes:
             return None
         return changes[-1].revision
 
-    def buildDownloadList(self):
-        """taken the changesource and forcebuild property,
-        build the repo download command to send to the slave
-        making this a defereable allow config to tweak this
-        in order to e.g. manage dependancies
-        """
-        downloads = self.repoDownloads
-        if downloads is None:
-            downloads = []
-        if self.repoDownloadsFromChangeSource:
-            # download patches based on GerritChangeSource events
-            for change in self.build.allChanges():
-                if ("event.type" in change.properties and
-                   change.properties["event.type"] == "patchset-created"):
-                        downloads.append("%s %s/%s" % (change.properties["event.change.project"],
-                                                       change.properties["event.change.number"],
-                                                       change.properties["event.patchSet.number"]))
-
-        self.repoDownloads = downloads
-        if downloads:
-            self.setProperty("computed_repo_downloads", downloads, "repo step")
-        return defer.succeed(None)
-
     def filterManifestPatches(self):
         """
         Patches to manifest projects are a bit special.
@@ -178,12 +174,12 @@ def filterManifestPatches(self):
         manifest_related_downloads = []
         for download in self.repoDownloads:
             project, ch_ps = download.split(" ")[-2:]
-            if (self.manifestUrl.endswith("/"+project) or
-               self.manifestUrl.endswith("/"+project+".git")):
+            if (self.manifestURL.endswith("/"+project) or
+               self.manifestURL.endswith("/"+project+".git")):
                 ch, ps = map(int, ch_ps.split("/"))
                 branch = "refs/changes/%02d/%d/%d" % (ch % 100, ch, ps)
                 manifest_related_downloads.append(
-                    ["git", "fetch", self.manifestUrl, branch])
+                    ["git", "fetch", self.manifestURL, branch])
                 manifest_related_downloads.append(
                     ["git", "cherry-pick", "FETCH_HEAD"])
             else:
@@ -231,8 +227,6 @@ def startVC(self, branch, revision, patch):
     def doStartVC(self):
         self.stdio_log = self.addLogForRemoteCommands("stdio")
 
-        yield self.buildDownloadList()
-
         self.filterManifestPatches()
 
         if self.repoDownloads:
@@ -274,7 +268,7 @@ def doRepoSync(self, forceClobber=False):
             yield self.doClobberStart()
         yield self.doCleanup()
         yield self._repoCmd(['init',
-                             '-u', self.manifestUrl,
+                             '-u', self.manifestURL,
                              '-b', self.manifestBranch,
                              '-m', self.manifestFile])
 
@@ -299,7 +293,7 @@ def doRepoSync(self, forceClobber=False):
             command.append('-c')
         self.step_status.setText(["repo sync"])
         self.stdio_log.addHeader("synching manifest %s from branch %s from %s\n"
-                                 % (self.manifestFile, self.manifestBranch, self.manifestUrl))
+                                 % (self.manifestFile, self.manifestBranch, self.manifestURL))
         yield self._repoCmd(command)
 
         command = ['manifest', '-r', '-o', 'manifest-original.xml']

master/buildbot/test/unit/test_steps_source_repo.py

@@ -79,7 +79,7 @@ def mySetupStep(self, **kwargs):
             kwargs.update(
                 dict(repoDownloads=repo.RepoDownloadsFromProperties(["repo_download", "repo_download2"])))
         self.setupStep(
-            repo.Repo(manifestUrl='git://myrepo.com/manifest.git',
+            repo.Repo(manifestURL='git://myrepo.com/manifest.git',
                       manifestBranch="mb",
                       manifestFile="mf",
                       **kwargs))
@@ -501,7 +501,7 @@ def test_repo_downloads_fail2(self):
 
     def test_repo_downloads_from_change_source(self):
         """basic repo download from change source, and check that repo_downloaded is updated"""
-        self.mySetupStep()
+        self.mySetupStep(repoDownloads=repo.RepoDownloadsFromChangeSource())
         chdict = TestGerritChangeSource.expected_change
         change = Change(None, None, None, properties=chdict['properties'])
         self.build.allChanges = lambda x=None: [change]
@@ -517,6 +517,26 @@ def test_repo_downloads_from_change_source(self):
             "repo_downloaded", "564/12 0123456789abcdef ", "Source")
         return self.myRunStep(status_text=["update"])
 
+    def test_repo_downloads_from_change_source_codebase(self):
+        """basic repo download from change source, and check that repo_downloaded is updated"""
+        self.mySetupStep(repoDownloads=repo.RepoDownloadsFromChangeSource("mycodebase"))
+        chdict = TestGerritChangeSource.expected_change
+        change = Change(None, None, None, properties=chdict['properties'])
+        # getSourceStamp is faked by SourceStepMixin
+        ss = self.build.getSourceStamp("")
+        ss.changes = [change]
+        self.expectnoClobber()
+        self.expectRepoSync()
+        self.expectCommands(
+            self.ExpectShell(command=['repo', 'download', 'pr', '4321/12'])
+            + 0
+            + Expect.log(
+                'stdio', stderr="test/bla refs/changes/64/564/12 -> FETCH_HEAD\n")
+            + Expect.log('stdio', stderr="HEAD is now at 0123456789abcdef...\n"))
+        self.expectProperty(
+            "repo_downloaded", "564/12 0123456789abcdef ", "Source")
+        return self.myRunStep(status_text=["update"])
+
     def test_update_fail1(self):
         """ fail at cleanup: ignored"""
         self.mySetupStep()

master/docs/manual/cfg-buildsteps.rst

@@ -788,7 +788,7 @@ for new and old projects.
 
 The Repo step takes the following arguments:
 
-``manifestUrl``
+``manifestURL``
     (required): the URL at which the Repo's manifests source repository is available.
 
 ``manifestBranch``
@@ -813,7 +813,8 @@ The Repo step takes the following arguments:
     simultaneously while syncing. Passed to repo sync subcommand with "-j".
 
 ``syncAllBranches``
-    (optional, defaults to ``False``): renderable boolean to control the policy of repo sync -c
+    (optional, defaults to ``False``): renderable boolean to control whether ``repo``
+    syncs all branches. i.e. ``repo sync -c``
 
 ``updateTarballAge``
     (optional, defaults to "one week"):
@@ -850,10 +851,29 @@ This feature allows integrators to build with several pending interdependent cha
 which at the moment cannot be described properly in Gerrit, and can only be described
 by humans.
 
-This Source step integrates with :bb:chsrc:`GerritChangeSource`, and will
+.. py:class:: buildbot.steps.source.repo.RepoDownloadsFromChangeSource
+
+``RepoDownloadsFromChangeSource`` can be used as a renderable of the ``repoDownload`` parameter
+
+This rendereable integrates with :bb:chsrc:`GerritChangeSource`, and will
 automatically use the :command:`repo download` command of repo to
 download the additionnal changes introduced by a pending changeset.
 
+.. note:: you can use the two above Rendereable in conjuction by using the class ``buildbot.process.properties.FlattenList``
+
+for example::
+
+   from buildbot.steps.source.repo import Repo, RepoDownloadsFromChangeSource,
+   from buildbot.steps.source.repo import RepoDownloadsFromProperties
+   from buildbot.process.properties import FlattenList
+
+   factory.addStep(Repo(manifestUrl='git://mygerrit.org/manifest.git',
+                        repoDownloads=FlattenList([RepoDownloadsFromChangeSource(),
+                                                   RepoDownloadsFromProperties("repo_downloads")
+                                                   ]
+                                                  )
+                        ))
+
 .. index:: double: Gerrit integration; Repo Build Step