657828 - [Tracking bug] - Assisted/Auto Landing from Bugzilla to tip of $branch

Status: RESOLVED DUPLICATE of bug 1043010

(Release Engineering :: General, defect)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

See:
http://crashopensource.blogspot.com/2011/05/assistedautomated-landing-designing.html
https://wiki.mozilla.org/BugzillaAutoLanding

This is the tracking bug for all the bugs related to this system being developed.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Change readme, no header in diff

Testing out autoland in production

Comment 5

[Mozilla RelEng Bot]

Invalid autoland tag "[autoland:2429:-p linux -b o -u none]".

Comment 6

[Mozilla RelEng Bot]

Autoland Failure
Specified patches [2429] do not exist, or are not posted on this bug.

Comment 7

[Mozilla RelEng Bot]

Autoland Failure
Specified patches [2429] do not exist, or are not posted on this bug.

Comment 8

[Mozilla RelEng Bot]

Autoland Failure
Specified patches [2429] do not exist, or are not posted on this bug.

Comment 9

[Mozilla RelEng Bot]

Autoland Failure
Specified patches [2429] do not exist, or are not posted on this bug.

Comment 10

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Try run started, revision e84c96fdfed5. To cancel or monitor the job, see: https://build.mozilla.org/buildapi/self-serve/try/rev/e84c96fdfed5

Comment 11

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Try run started, revision f7b09e907dfc. To cancel or monitor the job, see: https://build.mozilla.org/buildapi/self-serve/try/rev/f7b09e907dfc

Comment 12

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Try run started, revision c894b5f7f0b4. To cancel or monitor the job, see: https://build.mozilla.org/buildapi/self-serve/try/rev/c894b5f7f0b4

Comment 13

[Mozilla RelEng Bot]

Invalid autoland tag "[autoland:590876:-p linux -b o -u none]".

Comment 14

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Try run started, revision ee6b77070ba0. To cancel or monitor the job, see: https://build.mozilla.org/buildapi/self-serve/try/rev/ee6b77070ba0

Comment 15

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Could not apply and push patchset:
Failed to push

Comment 16

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Could not apply and push patchset:
Failed to push

Comment 17

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Try run started, revision 3c478848fdf9. To cancel or monitor the job, see: https://build.mozilla.org/buildapi/self-serve/try/rev/3c478848fdf9

Comment 18

[Mozilla RelEng Bot]

Try run for 3c478848fdf9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3c478848fdf9
Results (out of 1 total builds):
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3c478848fdf9

Comment 19

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Try run started, revision 1c70b964b975. To cancel or monitor the job, see: https://build.mozilla.org/buildapi/self-serve/try/rev/1c70b964b975

Comment 20

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: ssh://hg.mozilla.org/try
Try run started, revision 4fc20c2655e3. To cancel or monitor the job, see: https://build.mozilla.org/buildapi/self-serve/try/rev/4fc20c2655e3

Comment 21

[Mozilla RelEng Bot]

Try run for 1c70b964b975 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1c70b964b975
Results (out of 14 total builds):
    exception: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1c70b964b975

Comment 22

[Mozilla RelEng Bot]

Try run for 4fc20c2655e3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4fc20c2655e3
Results (out of 14 total builds):
    exception: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4fc20c2655e3

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: autoland v1 (complete)

Hi Rail,

This is the complete autoland system as it stands right now (and as reviewed last week during code review) - Marc has integrated most of the feedback from everyone last week.  The code is also here on github: https://github.com/lsblakk/tools/commit/130227846bfbe990c7e9b3f5c60e0e3d4660005e if the binaries pose any problems.

The goal is to get something committed to hg.m.o/tools/scripts/autoland so we can begin working on changes and fixes with more commit history tracking the improvements.

Thanks in advance for your time on this rather large patch :)

Comment 24

[Rail Aliiev [:rail]]

Comment on attachment 596806 [details] [diff] [review]
autoland v1 (complete)

Review of attachment 596806 [details] [diff] [review]:
-----------------------------------------------------------------

First of all, I'm happy to see that we're so close to make this happen. Thanks a lot to Marc and Lukas! Great job!

Overall comments:

* I had not enough time to review every single file, sorry. :( I haven't looked to utils/ tests/ data/ yet. Marc told me that the white board related code will be replaced and I'll be glad to re-review the code.
* I had not enough time to review the logic. I'd like to do this in person with Marc once he's ready.

Let's start.

* Since this is a new brand code and it doesn't inherit buildbot code guidelines it would be great ti use PEP8 http://www.python.org/dev/peps/pep-0008/ standart for function and variable names, line width, etc, especially:
 * Use lowercase_with_words_separated_by_underscores() naming for functions
 * Limit all lines to a maximum of 79 characters
 * Use http://pypi.python.org/pypi/pep8 to check
* using pyflakes helps finding trivial mistakes

::: scripts/autoland/autoland_queue.py
@@ +1,2 @@
> +import time
> +import os, errno, sys

errno unused

@@ +30,5 @@
> +def get_first_autoland_tag(whiteboard):
> +    """
> +    Returns the first autoland tag in the whiteboard
> +    """
> +    r = re.compile('\[autoland(-[^\[\]:]+)?((:\d+(,\d+)*)|(:-[^\[\]:]+)){0,2}\]', re.I)

Use raw string with regexes. Compare output '\.' vs r'\.' and '\\' vs r'\\'. Raw strings are safer.

@@ +70,5 @@
> +                try:
> +                    int(v)
> +                except:
> +                    # well it's not valid then, don't include it
> +                    values.remove(v)

Don't iterate and alter at the same time, it's dangerous. Use a copy instead: 
for v in list(values) or for v in values[:]

@@ +95,5 @@
> +                                'result':flag['status']})
> +                break
> +    return reviews
> +
> +def get_patchset(bug_id, try_run, user_patches=[], review_comment=True):

Don't use [] as a default value, it may be dangerous for mutable types. Use None instead.

@@ +135,5 @@
> +            { ... }
> +        ]
> +    """
> +    patchset = []
> +    if user_patches: user_patches = user_patches[:]    # take a local copy of patches.

If you use user_patches=None above, you'll need:
if user_patches:
    user_patches = list(user_patches)
else:
    user_patches = []

@@ +137,5 @@
> +    """
> +    patchset = []
> +    if user_patches: user_patches = user_patches[:]    # take a local copy of patches.
> +    # grab the bug data
> +    bug_data = bz.request('bug/%s' % str(bug_id))

bug_data = bz.request('bug/%s' % bug_id) is enough, %s casts to str.

@@ +210,5 @@
> +
> +        # get the branches
> +        branches = get_branch_from_tag(tag)
> +        print "Getting branches: %s" % branches
> +        for branch in branches:

Should be
for branch in list(branches):

since you alter branches

@@ +256,5 @@
> +            log.error('No valid patches attached, nothing for Autoland to do here, removing this bug from the queue.')
> +            continue
> +
> +        ps.author = patches[0]['author']['email']
> +        ps.patches = ','.join(map(lambda x: str(x['id']), patches))

Or more readable :)
ps.patches = ','.join(str(x['id']) for x in patches)

@@ +324,5 @@
> +                     )
> +        patchset_id = db.PatchSetInsert(ps)
> +        print "PatchSetID: %s" % patchset_id
> +
> +    comment = msg.get('comment', None)

None is redundant here:
comment = msg.get('comment')

@@ +327,5 @@
> +
> +    comment = msg.get('comment', None)
> +    if comment:
> +        # Handle the posting of a comment
> +        bug_id = msg.get('bug_id', None)

The same here:
bug_id = msg.get('bug_id')

@@ +341,5 @@
> +            if ps == None:
> +                log.error('No corresponding patch set found for %s' % msg['patchsetid'])
> +                return
> +            ps = ps[0]
> +            print "Got patchset back from DB: %s" % ps

use log.info() instead

@@ +365,5 @@
> +            else:
> +                # close it!
> +                bz.remove_whiteboard_tag('\[autoland-in-queue\]', ps.bug_id)
> +                db.PatchSetDelete(ps)
> +                log.debug('Deleting patchset %s' % (ps.id))

Parentheses around ps.id are redundant. Grammar Nazi!

@@ +373,5 @@
> +            # Guaranteed patchset EOL
> +            ps = db.PatchSetQuery(PatchSet(id=msg['patchsetid']))[0]
> +            bz.remove_whiteboard_tag('\[autoland-in-queue\]', ps.bug_id)
> +            db.PatchSetDelete(ps)
> +            log.debug('Successful push to branch of patchset %s.' % (ps.id))

Grammar Nazi!

@@ +455,5 @@
> +    # get branch information so that message can contain branch_url
> +    branch = db.BranchQuery(Branch(name=patchset.branch))
> +    if not branch:
> +        # error, branch non-existent XXX -- SHould we email or otherwise let user know?
> +        log.error('Could not find %s in branches table.' % (patchset.branch))

Grammar Nazi!

@@ +491,5 @@
> +    bug. In case of failure, the comments attempt count is updated, to be
> +    picked up again later.
> +    If we have attempted 5 times, get rid of the comment and log it.
> +    """
> +    comments = db.CommentGetNext(limit=5)   # Get up to 5 comments

limit and comment.attempts == 5 below should be moved to the beginning of the file or into the configuration file.

@@ +528,5 @@
> +    mq.set_host(config['mq_host'])
> +    mq.set_exchange(config['mq_exchange'])
> +    mq.connect()
> +
> +    log.setLevel(logging.DEBUG)

Maybe move logging level to the config file?

@@ +539,5 @@
> +                # purge the autoland queue
> +                mq.purge_queue(config['mq_autoland_queue'], prompt=True)
> +                exit(0)
> +
> +    while True:

Won't this loop without something like sleep be hammering the database?

@@ +555,5 @@
> +                proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +                (out, err) = proc.communicate()
> +                print proc.returncode
> +                print out
> +                print err

Use logging instead of print.

::: scripts/autoland/hgpusher.py
@@ +3,5 @@
> +import subprocess
> +import logging
> +import logging.handlers
> +import shutil
> +from tempfile import mkdtemp

mkdtemp unused, pyflakes rules ;)

@@ +7,5 @@
> +from tempfile import mkdtemp
> +from mercurial import error, lock   # For lockfile on working dirs
> +
> +from util.hg import mercurial, apply_and_push, cleanOutgoingRevs, out, \
> +                    remove_path, HgUtilError, update, get_revision

cleanOutgoingRevs, out, remove_path unused

@@ +27,5 @@
> +        username=config['bz_username'], password=config['bz_password'])
> +ldap = ldap_utils.ldap_util(config['ldap_host'], int(config['ldap_port']),
> +        config['ldap_bind_dn'], config['ldap_password'])
> +
> +def run_hg(hg_args):

Consider to use util.commands.run_cmd instead with better output.

@@ +40,5 @@
> +    (out, err) = proc.communicate()
> +    rc = proc.returncode
> +    return (out, err, rc)
> +
> +def has_valid_header(filename):

Some thoughts:
* Not everybody uses hg export for bugs
* If I add a comment line in the beginning this function won't check if userline exists

@@ +50,5 @@
> +    Note: this forces developers to use 'hg export' rather than 'hg diff'
> +          if they want to be pushing to branch.
> +    """
> +    f = open(filename, 'r')
> +    userline = re.compile('# User [\w\s]+ <[\w\d._%+-]+@[\w\d.-]+\.\w{2,6}>$')

* use raw strings
* move it to the beginning of the files so it's compiled once?

@@ +52,5 @@
> +    """
> +    f = open(filename, 'r')
> +    userline = re.compile('# User [\w\s]+ <[\w\d._%+-]+@[\w\d.-]+\.\w{2,6}>$')
> +    # XXX: Don't think we want to enforce the comment to start with bug xxxx
> +    commentline = re.compile('^bug\s?(\d+|\w+)[:\s]', re.I)

the same here

@@ +62,5 @@
> +                print 'Bad header.'
> +                return False
> +        elif commentline.match(line):
> +            return True
> +        elif re.match('^$', line):

or simpler elif line == '': ?

@@ +103,5 @@
> +
> +    return True
> +
> +def import_patch(repo, patch, try_run, bug_id=None, user=None,
> +        try_syntax="-p win32 -b o -u none"):

default try_syntax should set to None or read it from the config

@@ +114,5 @@
> +    """
> +    cmd = ['import', '-R']
> +    cmd.append(repo)
> +    if user:
> +        cmd.append('-u %s' % (user))

I think this won't work, it should be
cmd.extend(['-u', user])
otherwise Popen passed "-u username" as one argument

@@ +120,5 @@
> +        try_syntax = ''
> +    if try_run:
> +        # if there is no try_syntax, try defaults will get triggered by the 'try: ' alone
> +        if config.get('staging', False):
> +            cmd.extend(['-m "try: %s -n bug %s"' % (try_syntax, bug_id)])

similar here:
cmd.extend(['-m', 'try: %s -n bug %s' % (try_syntax, bug_id)])

@@ +122,5 @@
> +        # if there is no try_syntax, try defaults will get triggered by the 'try: ' alone
> +        if config.get('staging', False):
> +            cmd.extend(['-m "try: %s -n bug %s"' % (try_syntax, bug_id)])
> +        else:
> +            cmd.extend(['-m "try: %s -n --post-to-bugzilla bug %s"' % (try_syntax, bug_id)])

similar here:
cmd.extend(['-m', 'try: %s -n --post-to-bugzilla bug %s' % (try_syntax, bug_id)])

@@ +124,5 @@
> +            cmd.extend(['-m "try: %s -n bug %s"' % (try_syntax, bug_id)])
> +        else:
> +            cmd.extend(['-m "try: %s -n --post-to-bugzilla bug %s"' % (try_syntax, bug_id)])
> +    cmd.append(patch)
> +    print cmd

use logging here

@@ +140,5 @@
> +    out by process_patchset.
> +    There should always be a comment posted.
> +    """
> +    active_repo = os.path.join('active/%s' % (data['branch']))
> +    try_run = (data['try_run'] == True)

try_run = data['try_run'] or try_run = bool(data['try_run']) are more readable, IMHO

@@ +145,5 @@
> +    if 'push_url' in data:
> +        push_url = data['push_url']
> +    else:
> +        push_url = data['branch_url']
> +    push_url = push_url.replace('https', 'ssh', 1)

How about http?

@@ +148,5 @@
> +        push_url = data['branch_url']
> +    push_url = push_url.replace('https', 'ssh', 1)
> +
> +    # The comment header. The comment is constructed incrementally at any possible
> +    # failure/success point.

The following line is hard to read, can you split it into smaller chunks?

@@ +154,5 @@
> +            % (', '.join(map(lambda x: str(x['id']), data['patches'])), data['branch'],
> +               (' => try' if try_run else ''))]
> +    comment = comment_hdr
> +
> +    class RETRY(Exception):

Use RetryException or RetryError instead

@@ +161,5 @@
> +    def cleanup_wrapper():
> +        # use an attribute full_clean in order to keep track of
> +        # whether or not a full cleanup is required.
> +        # This is done since cleanup_wrapper's scope doesn't let us
> +        # access process_patchset globals, given the way it is used.

What's the difference between using globals? global full_clean? maintaining this code will be a problem.

@@ +174,5 @@
> +            update(active_repo)
> +            log.debug('Update -C on active repo for: %s' % data['branch'])
> +        cleanup_wrapper.full_clean = not cleanup_wrapper.full_clean
> +
> +        clone_revision = clone_branch(data['branch'], data['branch_url'])

This function shouldn't try to clone repo, it should do just _clean_up_. In this case the following TODO will be gone. apply_patchset tries to clone, no need to duplicate it here.

@@ +214,5 @@
> +            if not valid_header:
> +                # This is a try run, since we haven't exited
> +                # so author header not needed. Place in the author information
> +                # from bugzilla as committer.
> +                user='%s <%s>' % (patch['author']['name'], patch['author']['email'])

user set only if the patch has invalid header and this a try run?

@@ +239,5 @@
> +        return (False, '\n'.join(comment))
> +
> +    try:
> +        # make 3 attempts so that 1st is on active clone,
> +        # 2nd attempt will be after an update -c,

-C, I think

@@ +249,5 @@
> +                            ssh_key=config['hg_ssh_key'],
> +                            force=try_run))     # Force only on try pushes
> +        revision = get_revision(active_repo)
> +        shutil.rmtree(active_repo)
> +    except (HgUtilError, RETRY) as error:

syntax error for old python, redefines imported "error"
except (HgUtilError, RETRY), e:

@@ +294,5 @@
> +    if not os.access(clean, os.F_OK):
> +        os.mkdir(clean)
> +    try:
> +        mercurial(remote, clean_repo)
> +    except subprocess.CalledProcessError as e:

The same here:
except subprocess.CalledProcessError, e:

@@ +309,5 @@
> +    try:
> +        print 'Cloning from %s -----> %s' % (clean_repo, active_repo)
> +        revision = mercurial(clean_repo, active_repo)
> +        log.info('[Clone] Cloned revision %s' %(revision))
> +    except subprocess.CalledProcessError as error:

the same here

@@ +322,5 @@
> +    Clear the directories for the given branch,
> +    effictively removing any changes as well as clearing out the clean repo.
> +    """
> +    clean_repo = os.path.join('clean/', branch)
> +    active_repo = os.path.join('active/', branch)

no need for trailing slashes here.

@@ +326,5 @@
> +    active_repo = os.path.join('active/', branch)
> +    if os.access(clean_repo, os.F_OK):
> +        shutil.rmtree(clean_repo)
> +    if os.access(active_repo, os.F_OK):
> +        shutil.rmtree(active_repo)

No need to check if directory exists (because it may be deleted by other processes), just remove them wrapping the call with try/except

@@ +375,5 @@
> +    if 'job_type' not in data:
> +        log.error('[HgPusher] Erroneous message: %s' % (message))
> +        return
> +    if data['job_type'] == 'command':
> +        pass

remove above statement?

@@ +381,5 @@
> +        # check that all necessary data is present
> +        if not valid_job_message(data):
> +            # comment?
> +            # XXX: This is a bit more important than this...
> +            print "Not valid job message %s" % data

Use logging

@@ +391,5 @@
> +            data['branch'] = 'mozilla-central'
> +            data['branch_url'] = data['branch_url'].replace('try','mozilla-central', 1)
> +
> +        clone_revision = None
> +        for attempts in range(3):

Why 3, not 300? :) Wrap with retry instead?

@@ +420,5 @@
> +            msg = { 'type' : 'error', 'action' : 'patchset.apply',
> +                    'patchsetid' : data['patchsetid'],
> +                    'bug_id' : data['bug_id'],
> +                    'comment' : comment }
> +            mq.send_message(msg, 'db')

The logic here looks a little bit twisted, consider to simplify it.

@@ +440,5 @@
> +                mq.purge_queue(config['mq_hgp_queue'], prompt=True)
> +                exit(0)
> +
> +    try:
> +        if not os.access(config['work_dir'], os.F_OK):

use more readable os.path.isdir()?

@@ +449,5 @@
> +        i = 0
> +        while True:
> +            hgp_lock = None
> +            work_dir = 'hgpusher.%d' % (i)
> +            if not os.access(work_dir, os.F_OK):

os.path.isdir()?

@@ +458,5 @@
> +                print "Working directory: %s" % (work_dir)
> +                os.chdir(work_dir)
> +                # get rid of active dir
> +                if os.access('active/', os.F_OK):
> +                    shutil.rmtree('active/')

Don't try to check, just use rmtree and catch exceptions.

@@ +472,5 @@
> +                hgp_lock.release()
> +                print "Released working directory"
> +                break
> +    except os.error, e:
> +        log.error('Error switching to working directory: %s' % e)

Not only chdir, but others may raise an exception here. Need more generic msg or handle them separately.

::: scripts/autoland/launch.sh
@@ +97,5 @@
> +    fi
> +done
> +
> +HGP_COUNT=`screen -ls | grep hgpusher | wc -l`
> +echo -e "\nThere are a total of $HGP_COUNT hgpushers running"

I would prefer to rewrite this w/o screen. Runing it under control of a supervisor (runit, daemontools, etc) would be better.

::: scripts/autoland/schedulerDBpoller.py
@@ +1,4 @@
> +try:
> +    import json
> +except ImportError:
> +    import simplejson as json

Try to import simplejson first, it's faster.

@@ +1,5 @@
> +try:
> +    import json
> +except ImportError:
> +    import simplejson as json
> +import sys, os, traceback, urllib2, urllib, re

re unused

@@ +16,5 @@
> +POSTED_BUGS='postedbugs.log'
> +POLLING_INTERVAL=14400 # 4 hours
> +TIMEOUT=43200 # 12 hours
> +MAX_POLLING_INTERVAL=172800 # 48 hours
> +COMPLETION_THRESHOLD=600 # 10 minutes

use space around =

@@ +51,5 @@
> +                        None, self.config.get('bz', 'username'), self.config.get('bz', 'password'))
> +
> +        # Set up Self-Serve API
> +        self.self_serve_api_url = self.config.get('self_serve', 'url')
> +        if user != None:

"if user:" is easier to read.

@@ +56,5 @@
> +            self.user = user
> +        else:
> +            self.user = self.config.get('self_serve', 'user')
> +        
> +        if password !=None:

if password:

@@ +71,5 @@
> +        now = time()
> +        if self.verbose:
> +            log.debug("Checking for timed out revision: %s" % revision)
> +        filename = os.path.join(self.cache_dir, revision)
> +        if os.path.exists(filename):

You can drop the check above since you handle exceptions plus this check is evil, because the file may be removed after check and before open().

@@ +75,5 @@
> +        if os.path.exists(filename):
> +            try:
> +                f = open(filename, 'r')
> +                entries = f.readlines()
> +                f.close()

since you're using ArgumentParser which requires python 2.6 (or 2.5 + installed argparse) it would be better to use with statement with open():

from __future__ import with_statement

with open(filename) as f:
    entries = f.readlines()

The same applies for other open() calls.

@@ +76,5 @@
> +            try:
> +                f = open(filename, 'r')
> +                entries = f.readlines()
> +                f.close()
> +            except IOError, e:

e unused

@@ +79,5 @@
> +                f.close()
> +            except IOError, e:
> +                log.error("Couldn't open cache file for rev: %s" % revision)
> +                raise
> +            first_entry = mktime(strptime(entries[0].split('|')[0], "%a, %d %b %Y %H:%M:%S %Z"))

the line above line may raise an exception

@@ +105,5 @@
> +        results = self.CalculateResults(buildrequests)
> +        if self.verbose:
> +            log.debug("RESULTS (OrangeFactorHandling): %s" % results)
> +        if results['total_builds'] == results['success'] + results['failure'] + results['other'] + results['skipped'] + results['exception']:
> +            # It's really complete, now check for success

+ is_complete = True

@@ +107,5 @@
> +            log.debug("RESULTS (OrangeFactorHandling): %s" % results)
> +        if results['total_builds'] == results['success'] + results['failure'] + results['other'] + results['skipped'] + results['exception']:
> +            # It's really complete, now check for success
> +            if results['total_builds'] == results['success']:
> +                is_complete = True

- is_complete = True

@@ +108,5 @@
> +        if results['total_builds'] == results['success'] + results['failure'] + results['other'] + results['skipped'] + results['exception']:
> +            # It's really complete, now check for success
> +            if results['total_builds'] == results['success']:
> +                is_complete = True
> +                final_status = "success"

I would prefer to see SUCCESS instead.

@@ +110,5 @@
> +            if results['total_builds'] == results['success']:
> +                is_complete = True
> +                final_status = "success"
> +                if self.verbose:
> +                    log.debug("Complete and a success")

BTW, "if self.verbose:" looks very ugly.
I would use log.debug() without it, but increase log level (or set to debug) if self.verbose is set

@@ +112,5 @@
> +                final_status = "success"
> +                if self.verbose:
> +                    log.debug("Complete and a success")
> +            else:
> +                is_complete = True

- is_complete = True

@@ +116,5 @@
> +                is_complete = True
> +                final_status = "failure"
> +                if self.verbose:
> +                    log.debug("Complete and a failure")
> +        elif results['total_builds'] - results['warnings'] == results['success'] and results['warnings'] <= (MAX_ORANGE * 2):

Why * 2? Comments are welcome.

@@ +119,5 @@
> +                    log.debug("Complete and a failure")
> +        elif results['total_builds'] - results['warnings'] == results['success'] and results['warnings'] <= (MAX_ORANGE * 2):
> +                # Get buildernames where result was warnings
> +                buildernames = {}
> +                for key, value in buildrequests.items():

key is unused, consider to use "for value in buildrequests.values():" instead. The same occurs in many places.

@@ +130,5 @@
> +                retry_count = 0
> +                retry_pass = 0
> +                for name, info in buildernames.items():
> +                    # If we have more than one result for a builder name, compare the results
> +                    if len(info) == 2:

condition doesn't match the comment, should be > 1 instead

@@ +146,5 @@
> +                                retry_pass += 1
> +                    # Unique buildername with warnings, attempt a rebuild
> +                    else:
> +                        for result, branch, bid in info:
> +                            if result == 'warnings':

WARNINGS would be better.

@@ +171,5 @@
> +                log.debug("Too many warnings! Final = failure")
> +            is_complete = True
> +            final_status = "failure"
> +
> +        return is_complete, final_status

It's not easy to read this method, consider to simplify it

@@ +181,5 @@
> +                                  uri=self.self_serve_api_url,
> +                                  user=self.user,
> +                                  passwd=self.password)
> +        auth_handler = urllib2.HTTPBasicAuthHandler(password_mgr)
> +        opener = urllib2.build_opener(auth_handler, urllib2.HTTPSHandler())       

notice that this accepts any certificate, consider to use something like http://hg.mozilla.org/build/buildbotcustom/file/39656499d3f2/steps/signing.py#l17

@@ +193,5 @@
> +        try:
> +            req = urllib2.Request(url, data)
> +            req.method = "POST"
> +            return json.loads(opener.open(req).read())
> +        except urllib2.HTTPError, e:

need to handle json.loads exceptions as well

@@ +200,5 @@
> +    
> +    def GetSingleAuthor(self, buildrequests):
> +        """Look through a list of buildrequests and return only one author from the changes if one exists"""
> +        author = None
> +        for key, value in buildrequests.items():

for value in buildrequests.values():

@@ +206,5 @@
> +          if author == None:
> +              author = br['authors']
> +        # if there's one author return it
> +        if len(author) == 1:
> +            return ''.join(author)

maybe return author[0]?

@@ +207,5 @@
> +              author = br['authors']
> +        # if there's one author return it
> +        if len(author) == 1:
> +            return ''.join(author)
> +        elif author != None:

elif author: is more readable

@@ +208,5 @@
> +        # if there's one author return it
> +        if len(author) == 1:
> +            return ''.join(author)
> +        elif author != None:
> +            log.error("More than one author for: %s" % br)

nothing to return? raise an exception? or ignore? maybe return earlier once you find an author?

@@ +213,5 @@
> +    
> +    def GetBugNumbers(self, buildrequests):
> +        """Look through a list of buildrequests and return bug number from push comments"""
> +        bugs = []
> +        for key,value in buildrequests.items():

for value in buildrequests.values():

@@ +217,5 @@
> +        for key,value in buildrequests.items():
> +            br = value.to_dict()
> +            for comment in br['comments']:
> +                # we only want the bug specified in try syntax
> +                if len(comment.split('try: ')) > 1:

if 'try: ' in comment:

@@ +219,5 @@
> +            for comment in br['comments']:
> +                # we only want the bug specified in try syntax
> +                if len(comment.split('try: ')) > 1:
> +                    comment = comment.split('try: ')[1]
> +                    bugs = self.bz.bugs_from_comments(comment)

"return bugs" here as a shortcut instead of continuing the loop?

@@ +232,5 @@
> +            auto: if a check for 'autoland-' in the comments returns True (for future branch landings)
> +            None: if not try request and Autoland system isn't tracking it
> +        """
> +    
> +        type = None

don't redefine builtin type!

@@ +233,5 @@
> +            None: if not try request and Autoland system isn't tracking it
> +        """
> +    
> +        type = None
> +        for key,value in buildrequests.items():

for value in buildrequests.values():

@@ +236,5 @@
> +        type = None
> +        for key,value in buildrequests.items():
> +            br = value.to_dict()
> +            for comments in br['comments']:
> +                if 'try: ' in comments:

yes!

@@ +239,5 @@
> +            for comments in br['comments']:
> +                if 'try: ' in comments:
> +                    if flag_check:
> +                        if '--post-to-bugzilla' in comments:
> +                            type = "try"

I would prefer constant TRY here.

@@ +243,5 @@
> +                            type = "try"
> +                    else:
> +                        type = "try"
> +                if 'autoland-' in comments:
> +                    type = "auto"

type = AUTO

@@ +258,5 @@
> +            'exception': 0,
> +            'other': 0,
> +            'total_builds': 0
> +        }
> +        for key,value in buildrequests.items():

for value in buildrequests.values():

@@ +265,5 @@
> +            if br['results_str'].lower() in results.keys():
> +                results[br['results_str'].lower()] += 1
> +            else:
> +                results['other'] += 1
> +        results['total_builds'] = sum(results.values())

data duplication? can calculate this easily.

@@ +275,5 @@
> +            log.debug("REPORT: %s" % report)
> +
> +        message = """Try run for %s is complete.
> +Detailed breakdown of the results available here:
> +    https://tbpl.mozilla.org/?tree=%s&rev=%s

You can use textwrap.dedent to keep indentation.

@@ +281,5 @@
> +                                            revision, report['total_builds'])
> +        for key, value in report.items():
> +            if value > 0 and key != 'total_builds':
> +                message += "    %s: %d\n" % (key, value)
> +        if author != None:

if author:

@@ +283,5 @@
> +            if value > 0 and key != 'total_builds':
> +                message += "    %s: %d\n" % (key, value)
> +        if author != None:
> +            message += """Builds (or logs if builds failed) available at:
> +http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/%(author)s-%(revision)s""" % locals()

pass variables explicitly, locals() is more expensive.
moving the url template to the config file would be better as well

@@ +300,5 @@
> +                f.close()
> +                if self.verbose:
> +                    log.debug("WROTE TO %s: %s" % (filename, revision))
> +                self.RemoveCache(revision)
> +            except:

Don't catch everything (even ctrl+c)

@@ +308,5 @@
> +        # attach '.done' to the cache file so we're not tracking it anymore
> +        # delete original cache file
> +        cache_file = os.path.join(self.cache_dir, revision)
> +        if os.path.exists(cache_file):
> +            os.rename(cache_file, cache_file + '.done')

don't use os.patch.exists, use os.rename and catch exceptions.

@@ +310,5 @@
> +        cache_file = os.path.join(self.cache_dir, revision)
> +        if os.path.exists(cache_file):
> +            os.rename(cache_file, cache_file + '.done')
> +            if os.path.exists(cache_file):
> +                os.remove(cache_file)

the same here

@@ +337,5 @@
> +    def WriteToCache(self, incomplete):
> +        """ Writes results of incomplete build to cache dir in a file that is named with the revision """
> +        try:
> +            assert isinstance(incomplete, dict)
> +        except AssertionError, e:

e unused

@@ +343,5 @@
> +            raise
> +
> +        if not os.path.isdir(self.cache_dir):
> +            if not self.dry_run:
> +                os.mkdir(self.cache_dir)

may raise an exception

@@ +360,5 @@
> +                    f.write("%s|%s\n" % (strftime("%a, %d %b %Y %H:%M:%S %Z", localtime()), results))
> +                    if self.verbose:
> +                        log.debug("WROTE TO %s: %s|%s\n" % (filename, strftime("%a, %d %b %Y %H:%M:%S %Z", localtime()), results))
> +                    f.close()
> +                except:

don't catch everything

@@ +387,5 @@
> +            'interrupted': 0,
> +            'misc': 0,
> +            'status_string': "",
> +        }
> +        for key,value in buildrequests.items():

for value in buildrequests.values():

@@ +397,5 @@
> +        total_complete = status['misc'] + status['interrupted'] + status['cancelled'] + status['complete']
> +        if status['total_builds'] == total_complete:
> +            is_complete = True
> +            timeout_complete = []
> +            for key,value in buildrequests.items():

for value in buildrequests.values():

@@ +423,5 @@
> +            with the buildrequests per revision
> +        """
> +        rev_dict = {}
> +        buildrequests = self.scheduler_db.GetBuildRequests(None, self.branch, starttime, endtime)
> +        for key, value in buildrequests.items():

for value in buildrequests.values():

@@ +427,5 @@
> +        for key, value in buildrequests.items():
> +            # group buildrequests by revision
> +            br = value.to_dict()
> +            revision = br['revision']
> +            if not rev_dict.has_key(revision):

I'd prefer
if revision not in rev_dict:

@@ +431,5 @@
> +            if not rev_dict.has_key(revision):
> +                rev_dict[revision] = {}
> +        return rev_dict
> +
> +    def ProcessCompletedRevision(self, revision, message, bug, status_str, type):

don't redefine builtin type!

@@ +435,5 @@
> +    def ProcessCompletedRevision(self, revision, message, bug, status_str, type):
> +        """ Posts to bug and sends msg to autoland mq with completion status """
> +
> +        bug_post = False
> +        dupe = False

Dupe seems redundant, use posted instead.

@@ +437,5 @@
> +
> +        bug_post = False
> +        dupe = False
> +        result = False
> +        action = type + '.run'

type!

@@ +439,5 @@
> +        dupe = False
> +        result = False
> +        action = type + '.run'
> +
> +        if status_str == 'timed out':

TIMED_OUT

@@ +466,5 @@
> +                        'bug_id' : bug,
> +                        'revision': revision }
> +                self.mq.send_message(msg, routing_key='db')
> +            
> +        elif not self.dry_run and not dupe:

... and not posted:

@@ +474,5 @@
> +            else:
> +                log.debug("BZ POST FAILED message: %s bug: %s, couldn't notify bug. Try again later." % (message, bug))
> +            
> +        
> +        return bug_post

the logic in this function looks a little bit twisted. it would be better to simplify it.

@@ +490,5 @@
> +            'is_complete': False,
> +            'discard': False,
> +        }
> +        buildrequests = self.scheduler_db.GetBuildRequests(revision, self.branch)
> +        type = self.ProcessPushType(revision, buildrequests, flag_check)

type!

@@ +501,5 @@
> +            info['message'] = self.GenerateResultReportMessage(revision, results, self.GetSingleAuthor(buildrequests))
> +            if self.verbose:
> +                log.debug("POLL_BY_REVISION: MESSAGE: %s" % info['message'])
> +            for bug in bugs:
> +                if info['message'] != None and self.dry_run == False:

The following is easier to read, IMHO:
if info['message'] and not self.dry_run:

@@ +507,5 @@
> +                                                    message=info['message'], bug=bug, 
> +                                                    status_str=info['status']['status_string'], 
> +                                                    type=type)
> +                elif self.dry_run:
> +                    log.debug("DRY RUN: Would have posted %s to %s" % (info['message'], bug))

nothing to do if not dry run and not info['message']?

@@ +513,5 @@
> +        elif info['is_complete']:
> +            log.debug("Nothing to do here for %s" % revision)
> +            info['discard'] = True
> +        else:
> +            if bugs != None and not self.dry_run:

if bugs and not self.dry_run:

@@ +518,5 @@
> +                # Cache it
> +                log.debug("Writing %s to cache" % revision)
> +                incomplete = {}
> +                incomplete[revision] = info['status']
> +                self.WriteToCache(incomplete)

self.WriteToCache may raise an exeption

@@ +526,5 @@
> +        return info
> +    
> +    def PollByTimeRange(self, starttime, endtime):
> +        # Get all the unique revisions in the specified timeframe range
> +        rev_report = self.GetRevisions(starttime,endtime)

Grammar Nazi mode: need a space after ,

@@ +534,5 @@
> +            log.debug("INCOMPLETE REVISIONS IN CACHE %s" % (revisions))
> +        rev_report.update(revisions)
> +        # Clear out complete revisions from the rev_report keys
> +        for rev in completed_revisions:
> +            if rev_report.has_key(rev):

if rev in rev_report:

Find all has_key and replace with this construction.

@@ +586,5 @@
> +            if incomplete[rev]['status']['status_string'] == 'timed out':
> +                del incomplete[rev]
> +
> +        # Store the incomplete revisions for the next run if there's a bug
> +        self.WriteToCache(incomplete)

self.WriteToCache may raise an exception

Comment 25

[Marc Jessome[:mjessome]]

> @@ +365,5 @@
> > +            else:
> > +                # close it!
> > +                bz.remove_whiteboard_tag('\[autoland-in-queue\]', ps.bug_id)
> > +                db.PatchSetDelete(ps)
> > +                log.debug('Deleting patchset %s' % (ps.id))
> 
> Parentheses around ps.id are redundant. Grammar Nazi!
I'm not too sure about this. In my opinion, having the brackets makes the syntax for 1 formatting string to be more similar to the syntax for multiple. Also, if someone want's to add more information, it is easier to add with the brackets there than without.
Thoughts?

> @@ +539,5 @@
> > +                # purge the autoland queue
> > +                mq.purge_queue(config['mq_autoland_queue'], prompt=True)
> > +                exit(0)
> > +
> > +    while True:
> 
> Won't this loop without something like sleep be hammering the database?
> 
I was very sternly told during review in Hawaii not to sleep in that loop. What would be my best thing to do here?


> @@ +27,5 @@
> > +        username=config['bz_username'], password=config['bz_password'])
> > +ldap = ldap_utils.ldap_util(config['ldap_host'], int(config['ldap_port']),
> > +        config['ldap_bind_dn'], config['ldap_password'])
> > +
> > +def run_hg(hg_args):
> 
> Consider to use util.commands.run_cmd instead with better output.
The issue with using run_cmd is that I need the stderr output, which it doesn't provide.

> 
> @@ +40,5 @@
> > +    (out, err) = proc.communicate()
> > +    rc = proc.returncode
> > +    return (out, err, rc)
> > +
> > +def has_valid_header(filename):
> 
> Some thoughts:
> * Not everybody uses hg export for bugs
If it is a branch landing, we need their commit message. The only way we have (right now) of getting a commit message is through the use of an exported patch header.

> * If I add a comment line in the beginning this function won't check if
> userline exists
Fixed.

 
> @@ +50,5 @@
> > +    Note: this forces developers to use 'hg export' rather than 'hg diff'
> > +          if they want to be pushing to branch.
> > +    """
> > +    f = open(filename, 'r')
> > +    userline = re.compile('# User [\w\s]+ <[\w\d._%+-]+@[\w\d.-]+\.\w{2,6}>$')
> 
> * use raw strings
> * move it to the beginning of the files so it's compiled once?
I've done raw strings, and compiled -- but I just want to question if having it global is the best idea?

> @@ +145,5 @@
> > +    if 'push_url' in data:
> > +        push_url = data['push_url']
> > +    else:
> > +        push_url = data['branch_url']
> > +    push_url = push_url.replace('https', 'ssh', 1)
> 
> How about http?
Our hg urls are all https, so in the database we'll always have an https address.
e following line is hard to read, can you split it into smaller chunks?

> @@ +214,5 @@
> > +            if not valid_header:
> > +                # This is a try run, since we haven't exited
> > +                # so author header not needed. Place in the author information
> > +                # from bugzilla as committer.
> > +                user='%s <%s>' % (patch['author']['name'], patch['author']['email'])
> 
> user set only if the patch has invalid header and this a try run?
Correct. If the header is invalid, it is not allowed to push to branch since we don't have a commit message. If it is a try run, we can simply fill in information from their bugzilla account information.

> @@ +375,5 @@
> > +    if 'job_type' not in data:
> > +        log.error('[HgPusher] Erroneous message: %s' % (message))
> > +        return
> > +    if data['job_type'] == 'command':
> > +        pass
> 
> remove above statement?
This will be used soon for a "shutdown" command.

>
> @@ +420,5 @@
> > +            msg = { 'type' : 'error', 'action' : 'patchset.apply',
> > +                    'patchsetid' : data['patchsetid'],
> > +                    'bug_id' : data['bug_id'],
> > +                    'comment' : comment }
> > +            mq.send_message(msg, 'db')
> 
> The logic here looks a little bit twisted, consider to simplify it.
Not sure that I follow?

> ::: scripts/autoland/launch.sh
> @@ +97,5 @@
> > +    fi
> > +done
> > +
> > +HGP_COUNT=`screen -ls | grep hgpusher | wc -l`
> > +echo -e "\nThere are a total of $HGP_COUNT hgpushers running"
> 
> I would prefer to rewrite this w/o screen. Runing it under control of a
> supervisor (runit, daemontools, etc) would be better.
I'll see what I can come up with.

> @@ +75,5 @@
> > +        if os.path.exists(filename):
> > +            try:
> > +                f = open(filename, 'r')
> > +                entries = f.readlines()
> > +                f.close()
> 
> since you're using ArgumentParser which requires python 2.6 (or 2.5 +
> installed argparse) it would be better to use with statement with open():
> 
> from __future__ import with_statement
> 
> with open(filename) as f:
>     entries = f.readlines()
> 
> The same applies for other open() calls.
> 
We're running on 2.6 so we don't need the from __future__ import. Would it be a good idea to include it anyways?

> @@ +108,5 @@
> > +        if results['total_builds'] == results['success'] + results['failure'] + results['other'] + results['skipped'] + results['exception']:
> > +            # It's really complete, now check for success
> > +            if results['total_builds'] == results['success']:
> > +                is_complete = True
> > +                final_status = "success"
> 
> I would prefer to see SUCCESS instead.
> 
> 
> @@ +116,5 @@
> > +                is_complete = True
> > +                final_status = "failure"
> > +                if self.verbose:
> > +                    log.debug("Complete and a failure")
> > +        elif results['total_builds'] - results['warnings'] == results['success'] and results['warnings'] <= (MAX_ORANGE * 2):
> 
> Why * 2? Comments are welcome.
On retries, original warnings are still counted. The list of oranges is then compared to the previous list of oranges to confirm results.

> @@ +171,5 @@
> > +                log.debug("Too many warnings! Final = failure")
> > +            is_complete = True
> > +            final_status = "failure"
> > +
> > +        return is_complete, final_status
> 
> It's not easy to read this method, consider to simplify it
Will look into this.

> 
> @@ +181,5 @@
> > +                                  uri=self.self_serve_api_url,
> > +                                  user=self.user,
> > +                                  passwd=self.password)
> > +        auth_handler = urllib2.HTTPBasicAuthHandler(password_mgr)
> > +        opener = urllib2.build_opener(auth_handler, urllib2.HTTPSHandler())       
> 
> notice that this accepts any certificate, consider to use something like
> http://hg.mozilla.org/build/buildbotcustom/file/39656499d3f2/steps/signing.
> py#l17
>
That link goes to a blank line, and I'm not sure I see what's being done with a certificate.

> @@ +208,5 @@
> > +        # if there's one author return it
> > +        if len(author) == 1:
> > +            return ''.join(author)
> > +        elif author != None:
> > +            log.error("More than one author for: %s" % br)
> 
> nothing to return? raise an exception? or ignore? maybe return earlier once
> you find an author?
Will look into...


> @@ +219,5 @@
> > +            for comment in br['comments']:
> > +                # we only want the bug specified in try syntax
> > +                if len(comment.split('try: ')) > 1:
> > +                    comment = comment.split('try: ')[1]
> > +                    bugs = self.bz.bugs_from_comments(comment)
> 
> "return bugs" here as a shortcut instead of continuing the loop?
>
I'm actually curious if this should be bugs.append() rather than bugs =

> 
> @@ +265,5 @@
> > +            if br['results_str'].lower() in results.keys():
> > +                results[br['results_str'].lower()] += 1
> > +            else:
> > +                results['other'] += 1
> > +        results['total_builds'] = sum(results.values())
> 
> data duplication? can calculate this easily.
> 
Not too sure...

> @@ +275,5 @@
> > +            log.debug("REPORT: %s" % report)
> > +
> > +        message = """Try run for %s is complete.
> > +Detailed breakdown of the results available here:
> > +    https://tbpl.mozilla.org/?tree=%s&rev=%s
> 
> You can use textwrap.dedent to keep indentation.
I split it into separate strings:
message = "Try run for %s is complete.\n" \
          "Detailed breakdown of the results available here:\n" \
          ...... and so on.

> @@ +474,5 @@
> > +            else:
> > +                log.debug("BZ POST FAILED message: %s bug: %s, couldn't notify bug. Try again later." % (message, bug))
> > +            
> > +        
> > +        return bug_post
> 
> the logic in this function looks a little bit twisted. it would be better to
> simplify it.
>
Will look into it...

> @@ +507,5 @@
> > +                                                    message=info['message'], bug=bug, 
> > +                                                    status_str=info['status']['status_string'], 
> > +                                                    type=type)
> > +                elif self.dry_run:
> > +                    log.debug("DRY RUN: Would have posted %s to %s" % (info['message'], bug))
> 
> nothing to do if not dry run and not info['message']?
Will look into it...

> @@ +518,5 @@
> > +                # Cache it
> > +                log.debug("Writing %s to cache" % revision)
> > +                incomplete = {}
> > +                incomplete[revision] = info['status']
> > +                self.WriteToCache(incomplete)
> 
> self.WriteToCache may raise an exeption
> 
It's logged in WriteToCache, I'm not sure if we want to catch here, or let it fall further?


> @@ +586,5 @@
> > +            if incomplete[rev]['status']['status_string'] == 'timed out':
> > +                del incomplete[rev]
> > +
> > +        # Store the incomplete revisions for the next run if there's a bug
> > +        self.WriteToCache(incomplete)
> 
> self.WriteToCache may raise an exception
>
See above.

Comment 26

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=2ca1b01cc276
Try run started, revision 2ca1b01cc276. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=2ca1b01cc276

Comment 27

[Mozilla RelEng Bot]

Try run for 2ca1b01cc276 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2ca1b01cc276
Results (out of 14 total builds):
    exception: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2ca1b01cc276

Comment 28

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=3a03cbc4b288
Try run started, revision 3a03cbc4b288. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=3a03cbc4b288

Comment 29

[Mozilla RelEng Bot]

Try run for 3a03cbc4b288 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3a03cbc4b288
Results (out of 14 total builds):
    exception: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3a03cbc4b288

Comment 30

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876, 596806
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=1c0fbfb126a5
Try run started, revision 1c0fbfb126a5. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=1c0fbfb126a5

Comment 31

[Mozilla RelEng Bot]

Try run for 1c0fbfb126a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1c0fbfb126a5
Results (out of 14 total builds):
    exception: 12
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1c0fbfb126a5

Comment 32

[Marc Jessome[:mjessome]]

Sorry for all the spam today, just needed to sort a few things out :)

Comment 33

[Mozilla RelEng Bot]

Try run for cff7392e425b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cff7392e425b
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cff7392e425b

Comment 34

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=1ba807a24be5
Try run started, revision 1ba807a24be5. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=1ba807a24be5

Comment 35

[Mozilla RelEng Bot]

Try run for 1ba807a24be5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1ba807a24be5
Results (out of 14 total builds):
    exception: 3
    failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1ba807a24be5

Comment 36

[Rail Aliiev [:rail]]

Attachment: comments

Comments are inline the patch.

Comment 37

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=e6715431d54c
Try run started, revision e6715431d54c. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=e6715431d54c

Comment 38

[Mozilla RelEng Bot]

Try run for e6715431d54c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e6715431d54c
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e6715431d54c

Comment 39

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 590876
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=cbda8fbbdd9c
Try run started, revision cbda8fbbdd9c. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=cbda8fbbdd9c

Comment 40

[Mozilla RelEng Bot]

Try run for cbda8fbbdd9c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cbda8fbbdd9c
Results (out of 1 total builds):
    exception: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cbda8fbbdd9c

Comment 41

[Lukas Blakk [:lsblakk] use ?needinfo]

Thought this went over to Hal, but regardless it's not mine.

Comment 42

[Rail Aliiev [:rail]]

Good news, the security review went great and we have only 3 action items to be fixed.

The review: 
https://wiki.mozilla.org/Security/Reviews/Autoland

Action items:

* autolander and patch review must not be the same person
* individuals in the autoland group must be educated to respect sec-approval needs (security team to educate sheriffs and release management folks).
* bug commit message and bug number must match (people fat finger this, or attackers could try to confuse us as to where a patch came from)

Unfortunately, I've been working on some other projects and had not enough time to work on this one. I'll update the bug when we actively start working on Autoland.

Comment 43

[Chris AtLee [:catlee]]

We're redefining autoland in terms of a commit based model (rather than patch-based). See bug 1043010 for more info.