Merge lp:~gary/tarmac/fix693595 into lp:~launchpad/tarmac/lp-tarmac

[Gary Poster]

[r=matsubara] fix bug 693595 so we can use the commit message plugin currently configured on sapote for SMM.

This branch fixes bug 693595 (in Launchpad's Tarmac fork), so that we can use the commit message plugin currently configured on sapote for SMM. It has test code to show the change.

In the course of fixing this, I discovered that the .throw method of the generator protocol was not being implemented properly. The yield following a thrown exception is always used as the return value to the "throw" call, and the code did not handle this correctly. I needed to fix this error for my changes. This error would have had at least one other effect: after a pre-commit hook rejected a branch, the next branch's pre-commit hook would not have been called. I have not tested it explicitly, though perhaps I should.

Another problem that this fixes is that proposals that had been rejected in pre-commit hooks were being sent as participating proposals to the hook that tests the branch. I don't have an explicit test for that, though I probably should.

A last problem addressed is that pidfiles were being created in a loop, and not deleted until the process died. This caused problems when the loop actually looped. I am still not happy with how our pidfiles are created, but I only handled the high-level problem here by destroying the pidfile at the end of each loop. Tests existed for this, actually, but other logic errors hid the problem.

[lp:~gary/tarmac/fix693595]

390. By Gary Poster on 2011-01-03

respond to review with fixes

[Diogo Matsubara (matsubara)]

I discussed the changes with Gary on IRC. Here's the summary. Gary's going to change the small issues I brought up and push the changes. I'm approving the review based on the conversation.

<matsubara> gary_poster, about the review, there's only one thing I didn't quite understand, in lines 150 to 161 how's that yield raising an error?
<matsubara> is that when _fire_merge_hook is called and then generator.throw(e) ?
<gary_poster> yes matsubara
<matsubara> gary_poster, on line 212 you missed a new line between def update_proposal() and the last statement from the previous method
* gary_poster goes to diff, one sec
<gary_poster> matsubara, agreed
<matsubara> gary_poster, lines 119 says the bundle merge hooks is supposed to raise an exception but it doesn't do that anymore
<matsubara> lines 118 to 119
<gary_poster> matsubara: ah, right. I don't think it did before I came along either, but maybe I'm wrong. but yes, that needs to change. I'll make those two changes in just a sec and push a new version for you
<matsubara> cool. thanks

review: Approve

[Gary Poster (gary)]

Changes made. thank you!

=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2010-12-17 14:19:01 +0000
+++ tarmac/bin/commands.py	2011-01-03 21:02:11 +0000
@@ -2,7 +2,6 @@
 import httplib2
 import logging
 import os
-import time
 
 from bzrlib.commands import Command
 from bzrlib.help import help_commands
@@ -15,7 +14,7 @@
 from tarmac.log import set_up_debug_logging, set_up_logging
 from tarmac.exceptions import TarmacCommandError, TarmacMergeError
 from tarmac.plugin import load_plugins
-from tarmac.pidfile import get_pid, make_pidfile
+from tarmac.pidfile import make_pidfile, remove_pidfile, PidfileExistsError
 
 
 class TarmacCommand(Command):
@@ -237,61 +236,56 @@
     """
 
     def _do_merges(self, branch_url, imply_commit_message):
-        existing_pid = get_pid()
-        if existing_pid is None:
+        try:
             make_pidfile()
-        else:
-            # Check how long the lock has been there.
-            lock_creation_time = os.stat(self.config.PID_FILE)[-1]
-            now = time.time()
-            locked_period = now - lock_creation_time
-            # If the locked_period is greater than the allowed, it probably 
-            # means the test suite is stuck.
-            message = 'Tarmac lock file in place for %d seconds, PID: %d'
-            if locked_period > int(self.config.allowed_lock_period):
-                self.logger.info(message % (locked_period, get_pid()))
-                raise TarmacCommandError(message % (locked_period, get_pid()))
+        except PidfileExistsError, e:
+            message = "Is there another instance of Tarmac Running?  %s" % e
+            if e.lifetime > int(self.config.allowed_lock_period):
+                # If the locked_period is greater than the allowed, it
+                # probably means the test suite is stuck.
+                self.logger.info(message)
+                raise TarmacCommandError(message)
             else:
-                self.logger.info(message % (locked_period, get_pid()))
+                self.logger.info(message)
                 return
-
-        target = Branch.create(
-            self.launchpad.branches.getByUrl(url=branch_url), self.config,
-            imply_commit_message=imply_commit_message)
-
-        if not target.get_merge_proposals():
-            return
-
-        self.logger.debug('Firing tarmac_pre_merge hook')
-        tarmac_hooks['tarmac_pre_merge'].fire(self, target)
-
         try:
-            # revno to restore to if bundle merge fails.
-            restore_revno = target.tree.branch.revno() + 1
-            results = target.merge_branches()
-            # Use a set to collect the proposals. We don't want
-            # duplicates since results generator will return the (source,
-            # proposal) tuple twice, once for each pre and post commit hook.
-            collected_proposals = set()
-            while True:
-                try:
-                    (hook, source, proposal) = results.next()
-                    collected_proposals.add((source, proposal))
-                    self._fire_merge_hook(
-                        results, hook, target, source, proposal)
-                except StopIteration:
-                    break
-            tarmac_hooks['tarmac_bundle_merge'].fire(
-                self, target, restore_revno, collected_proposals)
-
-        # This except is here because we need the else and can't have it
-        # without an except as well.
-        except:
-            raise
-        else:
-            self.logger.debug('Firing tarmac_post_merge hook')
-            tarmac_hooks['tarmac_post_merge'].fire(self, target)
-            # Push target.tree.branch to location branch_url.
-            target.push()
+            target = Branch.create(
+                self.launchpad.branches.getByUrl(url=branch_url), self.config,
+                imply_commit_message=imply_commit_message)
+
+            if not target.get_merge_proposals():
+                return
+
+            self.logger.debug('Firing tarmac_pre_merge hook')
+            tarmac_hooks['tarmac_pre_merge'].fire(self, target)
+
+            try:
+                # revno to restore to if bundle merge fails.
+                restore_revno = target.tree.branch.revno() + 1
+                results = target.merge_branches()
+                collected_proposals = []
+                while True:
+                    try:
+                        (hook, source, proposal) = results.next()
+                        if hook == 'tarmac_post_commit':
+                            # We only want the proposals that have not been
+                            # rejected.
+                            collected_proposals.append((source, proposal))
+                        self._fire_merge_hook(
+                            results, hook, target, source, proposal)
+                    except StopIteration:
+                        break
+                if collected_proposals:
+                    # We actually have something to do!
+                    # This hook is probably the one found in
+                    # plugins/bundlecommand.py.
+                    tarmac_hooks['tarmac_bundle_merge'].fire(
+                        self, target, restore_revno, collected_proposals)
+                    self.logger.debug('Firing tarmac_post_merge hook')
+                    tarmac_hooks['tarmac_post_merge'].fire(self, target)
+                    # Push target.tree.branch to location branch_url.
+                    target.push()
+            finally:
+                target.cleanup()
         finally:
-            target.cleanup()
+            remove_pidfile()

=== modified file 'tarmac/branch.py'
--- tarmac/branch.py	2010-12-08 20:37:32 +0000
+++ tarmac/branch.py	2011-01-03 21:02:11 +0000
@@ -175,7 +175,7 @@
                     'source_branch': proposal.source_branch.bzr_identity})
             source = Branch.create(
                 proposal.source_branch, TarmacConfig(), target=self)
-
+            generator_exception_thrown = False
             try:
                 self.check_prerequisite(proposal)
 
@@ -195,12 +195,22 @@
                     str(proposal.reviewed_revid))
                 # At this point we need to yield control back to the caller,
                 # so the proper hook can be called.
-                yield ("tarmac_pre_commit", source, proposal)
+                try:
+                    yield ("tarmac_pre_commit", source, proposal)
+                except:
+                    # Someone called .throw on us.  They are objecting in
+                    # some way to the merge. We could just yield back to
+                    # the caller now, and then raise in the next
+                    # iteration, but instead we'll try to handle the
+                    # error first, and then yield.  Therefore, we need
+                    # to remember that we are in this state, and then
+                    # re-raise.
+                    generator_exception_thrown = True
+                    raise
 
             except TarmacMergeError, failure:
                 self.update_proposal_with_error(proposal, failure)
                 self.cleanup()
-                continue
             except PointlessMerge:
                 self.logger.warn(
                     'Merging %(source)s into %(target)s would be '
@@ -208,23 +218,27 @@
                         'source': proposal.source_branch.display_name,
                         'target': proposal.target_branch.display_name})
                 self.cleanup()
-                continue
-
-            merge_url = get_review_url(proposal)
-            revprops = {'merge_url': merge_url}
-
-            commit_message = proposal.commit_message
-            if commit_message is None and self.imply_commit_message:
-                commit_message = proposal.description
-            self.commit(commit_message,
-                         revprops=revprops,
-                         authors=source.authors,
-                         reviews=self._get_reviews(proposal))
-
-            # At this point we need to yield control back to the caller,
-            # so the proper hook can be called.
-            yield ("tarmac_post_commit", source, proposal)
-            self.cleanup()
+            else:
+                merge_url = get_review_url(proposal)
+                revprops = {'merge_url': merge_url}
+
+                commit_message = proposal.commit_message
+                if commit_message is None and self.imply_commit_message:
+                    commit_message = proposal.description
+                self.commit(commit_message,
+                             revprops=revprops,
+                             authors=source.authors,
+                             reviews=self._get_reviews(proposal))
+
+                # At this point we need to yield control back to the caller,
+                # so the proper hook can be called.
+                yield ("tarmac_post_commit", source, proposal)
+                self.cleanup()
+            if generator_exception_thrown:
+                # Someone called .throw when we announced the pre_commit
+                # period.  We handled it, and it's time to yield control
+                # back to the caller.
+                yield
 
     def update_proposal(self, proposal, comment):
         """Set proposal to Needs review and update with given comment."""

=== modified file 'tarmac/pidfile.py'
--- tarmac/pidfile.py	2010-12-14 20:47:53 +0000
+++ tarmac/pidfile.py	2011-01-03 21:02:11 +0000
@@ -7,6 +7,7 @@
 
 import tempfile
 import os
+import time
 import atexit
 import sys
 from signal import signal, SIGTERM
@@ -14,6 +15,17 @@
 from tarmac.config import TarmacConfig
 
 
+class PidfileExistsError(RuntimeError):
+    def __init__(self, pidfile, pid, lifetime):
+        self.pidfile = pidfile
+        self.pid = pid
+        self.lifetime = lifetime
+
+    def __str__(self):
+        return (
+            'File %r reports that PID %r has existed for %d seconds' % (
+                self.pidfile, self.pid, self.lifetime))
+
 def make_pidfile():
     """Write the current process id to a PID file.
 
@@ -24,21 +36,27 @@
     inside it.
     """
     pidfile = TarmacConfig().PID_FILE
-    if os.path.exists(pidfile):
-        raise RuntimeError("PID file %s already exists. Already running?" %
-                pidfile)
-
-    atexit.register(remove_pidfile)
-    def remove_pidfile_handler(*ignored):
-        sys.exit(-1 * SIGTERM)
-    signal(SIGTERM, remove_pidfile_handler)
-
     fd, tempname = tempfile.mkstemp(dir=os.path.dirname(pidfile))
     outf = os.fdopen(fd, 'w')
     outf.write(str(os.getpid())+'\n')
     outf.flush()
     outf.close()
-    os.rename(tempname, pidfile)
+    # This introduces a race condition.  We're hoping it doesn't happen.
+    if os.path.exists(pidfile):
+        os.remove(tempname)
+        raise PidfileExistsError(tempname, get_pid(), get_pidfile_lifetime())
+    else:
+        os.rename(tempname, pidfile)
+
+
+# This could be used if the pidfile creation were not within a loop.
+#
+# def install_pidfile_removal():
+#
+#    atexit.register(remove_pidfile)
+#    def remove_pidfile_handler(*ignored):
+#        sys.exit(-1 * SIGTERM)
+#    signal(SIGTERM, remove_pidfile_handler)
 
 
 def remove_pidfile():
@@ -73,5 +91,11 @@
     except IOError:
         return None
     except ValueError:
-        raise ValueError("Invalid PID %s" % repr(pid))
-
+        raise ValueError("Invalid PID %r" % pid)
+
+
+def get_pidfile_lifetime():
+    """Return the number of seconds since the pidfile was created"""
+    creation_time = os.stat(TarmacConfig().PID_FILE)[-1]
+    now = time.time()
+    return now - creation_time

=== modified file 'tarmac/tests/test_commands.py'
--- tarmac/tests/test_commands.py	2010-12-17 14:19:01 +0000
+++ tarmac/tests/test_commands.py	2011-01-03 21:02:11 +0000
@@ -383,6 +383,17 @@
         self.install_plugin(plugin, hook_name)
         return plugin
 
+    def install_recorder_plugin(self, hook_name):
+        class RecorderPlugin(TarmacPlugin):
+            calls = []
+
+            def run(self, *args):
+                self.__class__.calls.append(args)
+
+        plugin = RecorderPlugin()
+        self.install_plugin(plugin, hook_name)
+        return plugin
+
     def install_plugin(self, plugin, hook_name):
         '''Install a plugin at the given hookpoint name.
 
@@ -434,15 +445,22 @@
     def test_plugin_merge_errors_are_handled(self):
         # Setup
         self.approve_all_proposals()
-        plugin = self.install_merge_error_injecting_plugin('tarmac_pre_commit')
+        pre_commit_plugin = self.install_merge_error_injecting_plugin(
+            'tarmac_pre_commit')
+        bundle_merge_plugin = self.install_recorder_plugin(
+            'tarmac_bundle_merge')
 
         # Act
         self.command.run(launchpad=self.launchpad)
 
         # Verify
-        self.assertTrue(plugin.has_run)
+        self.assertTrue(pre_commit_plugin.has_run)
         self.assertIsInstance(self.error, TarmacMergeError)
-        self.assertEqual(self.error.comment, plugin.long_message)
+        self.assertEqual(self.error.comment, pre_commit_plugin.long_message)
+        # No commits were accepted, so we should have aborted the merge,
+        # and not called the merge hook (which would usually run tests).
+        self.assertEqual(bundle_merge_plugin.calls, [])
+
 
 # XXX matsubara 2010-12-17: Test commented out because we're not handling
 # errors in the command itself but in the plugin. This is a temporary hack
@@ -461,3 +479,20 @@
 #        self.assertIsInstance(self.error, TarmacMergeError)
 #        self.assertEqual(self.error.comment, plugin.long_message)
 #
+
+
+# This is a debug helper.
+# nosetests eat stdout.  This is a way to work around it.  Use like this:
+# with pdb():
+#     ...stuff you want to pdb through...
+from contextlib import contextmanager
+@contextmanager
+def pdb():
+    import pdb, sys
+    test_stdout = sys.stdout
+    sys.stdout = sys.__stdout__
+    import pdb; pdb.set_trace()
+    try:
+        yield
+    finally:
+        sys.stdout = test_stdout