Merge branch 'pull385'

* pull385:
  fix pyflakes
  clean up returnValue calls
  Move method to base scheduler
  Should use username not owner
  Change the reason text to not be web specific
  Move webstatus specific code to b.s.w.builder
  Use a specific error message instead of ValueError
  get_from_post -> getFromKwargs
  Remove Python2.4ism
  Refactor so that ForceScheduler isn't as tightly coupled to HTML
  Use a dict() for r.args (the real one is a dict anyway)
  Break out property gathering and scheduling into seperate functions

master/buildbot/schedulers/base.py

@@ -254,7 +254,6 @@ def gotChange(self, change, important):
 
     ## starting bulids
 
-    @defer.deferredGenerator
     def addBuildsetForLatest(self, reason='', external_idstring=None,
                         branch=None, repository='', project='',
                         builderNames=None, properties=None):
@@ -263,16 +262,45 @@ def addBuildsetForLatest(self, reason='', external_idstring=None,
         repository, and project.  This will create a relative sourcestamp for
         the buildset.
 
-        This method will add any properties provided to the scheduler
-        constructor to the buildset, and will call the master's addBuildset
-        method with the appropriate parameters.
+        @param reason: reason for this buildset
+        @type reason: unicode string
+        @param external_idstring: external identifier for this buildset, or None
+        @param branch: branch to build (note that None often has a special meaning)
+        @param repository: repository name for sourcestamp
+        @param project: project name for sourcestamp
+        @param builderNames: builders to name in the buildset (defaults to
+            C{self.builderNames})
+        @param properties: a properties object containing initial properties for
+            the buildset
+        @type properties: L{buildbot.process.properties.Properties}
+        @returns: (buildset ID, buildrequest IDs) via Deferred
+        """
+
+        return self.addBuildSetForSourceStampDetails(
+            reason = reason,
+            external_idstring = external_idstring,
+            branch = branch,
+            repository = repository,
+            project = project,
+            builderNames = builderNames,
+            properties = properties
+            )
+
+    @defer.deferredGenerator
+    def addBuildSetForSourceStampDetails(self, reason='', external_idstring=None,
+                        branch=None, repository='', project='', revision=None,
+                        builderNames=None, properties=None):
+        """
+        Given details about the source code to build, create a source stamp and
+        then add a buildset for it.
 
         @param reason: reason for this buildset
         @type reason: unicode string
         @param external_idstring: external identifier for this buildset, or None
         @param branch: branch to build (note that None often has a special meaning)
         @param repository: repository name for sourcestamp
         @param project: project name for sourcestamp
+        @param revision: revision to build - default is latest
         @param builderNames: builders to name in the buildset (defaults to
             C{self.builderNames})
         @param properties: a properties object containing initial properties for
@@ -286,7 +314,7 @@ def addBuildsetForLatest(self, reason='', external_idstring=None,
         setid = wfd.getResult()
 
         wfd = defer.waitForDeferred(self.master.db.sourcestamps.addSourceStamp(
-                branch=branch, revision=None, repository=repository,
+                branch=branch, revision=revision, repository=repository,
                 project=project, sourcestampsetid=setid))
         yield wfd
         wfd.getResult()
@@ -415,3 +443,4 @@ def addBuildsetForSourceStamp(self, ssid=None, setid=None, reason='', external_i
                                         external_idstring=external_idstring))
         yield wfd
         yield wfd.getResult()
+

master/buildbot/schedulers/forcesched.py

@@ -16,16 +16,14 @@
 import traceback
 import re
 from twisted.internet import defer
-try:
-    import email.utils as email_utils
-    email_utils = email_utils
-except ImportError:
-    # Python-2.4 capitalization
-    import email.Utils as email_utils
+import email.utils as email_utils
 
 from buildbot.process.properties import Properties
 from buildbot.schedulers import base
 
+class ValidationError(ValueError):
+    pass
+
 class BaseParameter(object):
     name = ""
     label = ""
@@ -45,19 +43,19 @@ def __init__(self, name, label=None, regex=None, **kw):
         # all other properties are generically passed via **kw
         self.__dict__.update(kw)
 
-    def update_from_post(self, master, properties, changes, req):
-        args = req.args.get(self.name, [])
+    def getFromKwargs(self, kwargs):
+        args = kwargs.get(self.name, [])
         if len(args) == 0:
             if self.required:
-                raise ValueError("'%s' needs to be specified" % (self.label))
+                raise ValidationError("'%s' needs to be specified" % (self.label))
             if self.multiple:
                 args = self.default
             else:
                 args = [self.default]
         if self.regex:
             for arg in args:
                 if not self.regex.match(arg):
-                    raise ValueError("%s:'%s' does not match pattern '%s'"
+                    raise ValidationError("%s:'%s' does not match pattern '%s'"
                             % (self.label, arg, self.regex.pattern))
         try:
             arg = self.parse_from_args(args)
@@ -68,9 +66,12 @@ def update_from_post(self, master, properties, changes, req):
                 traceback.print_exc()
             raise e
         if arg == None:
-            raise ValueError("need %s: no default provided by config"
+            raise ValidationError("need %s: no default provided by config"
                     % (self.name,))
-        properties[self.name] = arg
+        return arg
+
+    def updateFromKwargs(self, master, properties, changes, kwargs):
+        properties[self.name] = self.getFromKwargs(kwargs)
 
     def parse_from_args(self, l):
         if self.multiple:
@@ -117,10 +118,8 @@ class IntParameter(StringParameter):
 class BooleanParameter(BaseParameter):
     type = "bool"
 
-    def update_from_post(self, master, properties, changes, req):
-        # damn html's ungeneric checkbox implementation...
-        checkbox = req.args.get("checkbox", [""])
-        properties[self.name] = self.name in checkbox
+    def getFromKwargs(self, kwargs):
+        return self.name in kwargs and kwargs[self.name] == [True]
 
 
 class UserNameParameter(StringParameter):
@@ -138,7 +137,7 @@ def parse_from_arg(self, s):
         if self.need_email:
             e = email_utils.parseaddr(s)
             if e[0]=='' or e[1] == '':
-                raise ValueError("%s: please fill in email address in the "
+                raise ValidationError("%s: please fill in email address in the "
                         " form User <email@email.com>" % (self.label,))
         return s
 
@@ -150,26 +149,29 @@ class ChoiceStringParameter(BaseParameter):
 
     def parse_from_arg(self, s):
         if self.strict and not s in self.choices:
-            raise ValueError("'%s' does not belongs to list of available choices '%s'"%(s, self.choices))
+            raise ValidationError("'%s' does not belongs to list of available choices '%s'"%(s, self.choices))
         return s
 
 
 class InheritBuildParameter(ChoiceStringParameter):
     name = "inherit"
     compatible_builds = None
 
-    def update_from_post(self, master, properties, changes, req):
-        arg = req.args.get(self.name, [""])[0]
+    def getFromKwargs(self, kwargs):
+        raise ValidationError("InheritBuildParameter can only be used by properties")
+
+    def updateFromKwargs(self, master, properties, changes, kwargs):
+        arg = kwargs.get(self.name, [""])[0]
         splitted_arg = arg.split(" ")[0].split("/")
         if len(splitted_arg) != 2:
-            raise ValueError("bad build: %s"%(arg))
+            raise ValidationError("bad build: %s"%(arg))
         builder, num = splitted_arg
         builder_status = master.status.getBuilder(builder)
         if not builder_status:
-            raise ValueError("unknown builder: %s in %s"%(builder, arg))
+            raise ValidationError("unknown builder: %s in %s"%(builder, arg))
         b = builder_status.getBuild(int(num))
         if not b:
-            raise ValueError("unknown build: %d in %s"%(num, arg))
+            raise ValidationError("unknown build: %d in %s"%(num, arg))
         props = {self.name:(arg.split(" ")[0])}
         for name, value, source in b.getProperties().asList():
             if source == "Force Build Form":
@@ -183,17 +185,20 @@ def update_from_post(self, master, properties, changes, req):
 class AnyPropertyParameter(BaseParameter):
     type = "anyproperty"
 
-    def update_from_post(self, master, properties, changes, req):
+    def getFromKwargs(self, kwargs):
+        raise ValidationError("AnyPropertyParameter can only be used by properties")
+
+    def updateFromKwargs(self, master, properties, changes, kwargs):
         validation = master.config.validation
-        pname = req.args.get("%sname" % self.name, [""])[0]
-        pvalue = req.args.get("%svalue" % self.name, [""])[0]
+        pname = kwargs.get("%sname" % self.name, [""])[0]
+        pvalue = kwargs.get("%svalue" % self.name, [""])[0]
         if not pname:
             return
         pname_validate = validation['property_name']
         pval_validate = validation['property_value']
         if not pname_validate.match(pname) \
                 or not pval_validate.match(pvalue):
-            raise ValueError("bad property name='%s', value='%s'" % (pname, pvalue))
+            raise ValidationError("bad property name='%s', value='%s'" % (pname, pvalue))
         properties[pname] = pvalue
 
 
@@ -236,69 +241,73 @@ def startService(self):
     def stopService(self):
         pass
 
-    def forceWithWebRequest(self, owner, builder_name, req):
-        """Called by the web UI.
-        Authentication is already done, thus owner is passed as argument
+    @defer.inlineCallbacks
+    def gatherPropertiesAndChanges(self, **kwargs):
+        properties = {}
+        changeids = []
+
+        for param in self.forcedProperties:
+            yield defer.maybeDeferred(param.updateFromKwargs, self.master, properties, changeids, kwargs)
+
+        changeids = map(lambda a: type(a)==int and a or a.number, changeids)
+
+        real_properties = Properties()
+        for pname, pvalue in properties.items():
+            real_properties.setProperty(pname, pvalue, "Force Build Form")
+
+        defer.returnValue((real_properties, changeids))
+
+    @defer.inlineCallbacks
+    def force(self, owner, builder_name, **kwargs):
+        """
         We check the parameters, and launch the build, if everything is correct
         """
         if not builder_name in self.builderNames:
             # in the case of buildAll, this method will be called several times
             # for all the builders
             # we just do nothing on a builder that is not in our builderNames
-            return defer.succeed(None)
-        master = self.master
-        properties = {}
-        changeids = []
+            defer.returnValue(None)
+            return
+
+        # Currently the validation code expects all kwargs to be lists
+        # I don't want to refactor that now so much sure we comply...
+        kwargs = dict((k, [v]) if not isinstance(v, list) else (k,v) for k,v in kwargs.items())
+
         # probably need to clean that out later as the IProperty is already a
         # validation mechanism
 
-        validation = master.config.validation
+        validation = self.master.config.validation
         if self.branch.regex == None:
             self.branch.regex = validation['branch']
         if self.revision.regex == None:
             self.revision.regex = validation['revision']
 
-        for param in self.all_fields:
-            if owner and param==self.username:
-                continue # dont enforce username if auth
-            param.update_from_post(master, properties, changeids, req)
+        reason = self.reason.getFromKwargs(kwargs)
+        branch = self.branch.getFromKwargs(kwargs)
+        revision = self.revision.getFromKwargs(kwargs)
+        repository = self.repository.getFromKwargs(kwargs)
+        project = self.project.getFromKwargs(kwargs)
 
-        changeids = map(lambda a: type(a)==int and a or a.number, changeids)
-        # everything is validated, we can create our source stamp, and buildrequest
-        reason = properties[self.reason.name]
-        branch = properties[self.branch.name]
-        revision = properties[self.revision.name]
-        repository = properties[self.repository.name]
-        project = properties[self.project.name]
         if owner is None:
-            owner =  properties[self.username.name]
+            owner = self.username.getFromKwargs(kwargs)
+
+        properties, changeids = yield self.gatherPropertiesAndChanges(**kwargs)
+
+        properties.setProperty("reason", reason, "Force Build Form")
+        properties.setProperty("owner", owner, "Force Build Form")
+
+        r = ("A build was forced by '%s': %s" % (owner, reason))
+
+        # everything is validated, we can create our source stamp, and buildrequest
+        res = yield self.addBuildSetForSourceStampDetails(
+            reason = r,
+            branch = branch,
+            repository = repository,
+            revision = revision,
+            project = project,
+            builderNames = [builder_name],
+            properties = properties,
+            )
+
+        defer.returnValue(res)
 
-        std_prop_names = [self.branch.name, 
-                          self.revision.name, self.repository.name,
-                          self.project.name, self.username.name]
-        real_properties = Properties()
-        for pname, pvalue in properties.items():
-            if not pname in std_prop_names:
-                real_properties.setProperty(pname, pvalue, "Force Build Form")
-
-        real_properties.setProperty("owner", owner, "Force Build Form")
-
-        r = ("The web-page 'force build' button was pressed by '%s': %s"
-             % (owner, reason)) 
-
-        d = master.db.sourcestampsets.addSourceStampSet()
-        def add_master_with_setid(sourcestampsetid):
-            master.db.sourcestamps.addSourceStamp(
-                                    sourcestampsetid = sourcestampsetid,
-                                    branch=branch,
-                                    revision=revision, project=project, 
-                                    repository=repository,changeids=changeids)
-            return sourcestampsetid
-
-        d.addCallback(add_master_with_setid)
-        def got_setid(sourcestampsetid):
-            return self.addBuildsetForSourceStamp(builderNames=[builder_name],
-                                    setid=sourcestampsetid, reason=r,
-                                    properties=real_properties)
-        d.addCallback(got_setid)
-        return d

master/buildbot/status/web/builder.py

@@ -23,7 +23,9 @@
     path_to_build, path_to_slave, path_to_builder, path_to_change, \
     path_to_root, ICurrentBox, build_get_class, \
     map_branches, path_to_authzfail, ActionResource
-from buildbot.schedulers.forcesched import ForceScheduler, InheritBuildParameter
+from buildbot.schedulers.forcesched import ForceScheduler
+from buildbot.schedulers.forcesched import InheritBuildParameter
+from buildbot.schedulers.forcesched import ValidationError
 from buildbot.status.web.build import BuildsResource, StatusResourceBuild
 from buildbot import util
 
@@ -138,13 +140,21 @@ def performAction(self, req):
             defer.returnValue((path_to_builder(req, self.builder_status),
                                "forcescheduler arg not found"))
             return
+
+        args = {}
+        # damn html's ungeneric checkbox implementation...
+        for cb in req.args.get("checkbox", []):
+            args[cb] = True
+        args.update(req.args)
+
+        builder_name = self.builder_status.getName()
+
         for sch in master.allSchedulers():
             if schedulername == sch.name:
                 try:
-                    yield sch.forceWithWebRequest(owner,
-                            self.builder_status.getName(), req)
+                    yield self.force(owner, builder_name, **args)
                     msg = ""
-                except Exception, e:
+                except ValidationError, e:
                     msg = html.escape(e.message.encode('ascii','ignore'))
                 break