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

Proposed by Gary Poster on 2011-01-03
Status: Merged
Approved by: Gary Poster on 2011-01-03
Approved revision: 390
Merged at revision: 389
Proposed branch: lp:~gary/tarmac/fix693595
Merge into: lp:~launchpad/tarmac/lp-tarmac
Diff against target: 364 lines (+157/-90)
4 files modified
tarmac/bin/commands.py (+49/-55)
tarmac/branch.py (+34/-20)
tarmac/pidfile.py (+36/-12)
tarmac/tests/test_commands.py (+38/-3)
To merge this branch: bzr merge lp:~gary/tarmac/fix693595
Reviewer Review Type Date Requested Status
Diogo Matsubara (community) 2011-01-03 Approve on 2011-01-03
Review via email: mp+45039@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
lp:~gary/tarmac/fix693595 updated on 2011-01-03
390. By Gary Poster on 2011-01-03

respond to review with fixes

Diogo Matsubara (matsubara) wrote :

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) wrote :

Changes made. thank you!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tarmac/bin/commands.py'
2--- tarmac/bin/commands.py 2010-12-17 14:19:01 +0000
3+++ tarmac/bin/commands.py 2011-01-03 21:02:11 +0000
4@@ -2,7 +2,6 @@
5 import httplib2
6 import logging
7 import os
8-import time
9
10 from bzrlib.commands import Command
11 from bzrlib.help import help_commands
12@@ -15,7 +14,7 @@
13 from tarmac.log import set_up_debug_logging, set_up_logging
14 from tarmac.exceptions import TarmacCommandError, TarmacMergeError
15 from tarmac.plugin import load_plugins
16-from tarmac.pidfile import get_pid, make_pidfile
17+from tarmac.pidfile import make_pidfile, remove_pidfile, PidfileExistsError
18
19
20 class TarmacCommand(Command):
21@@ -237,61 +236,56 @@
22 """
23
24 def _do_merges(self, branch_url, imply_commit_message):
25- existing_pid = get_pid()
26- if existing_pid is None:
27+ try:
28 make_pidfile()
29- else:
30- # Check how long the lock has been there.
31- lock_creation_time = os.stat(self.config.PID_FILE)[-1]
32- now = time.time()
33- locked_period = now - lock_creation_time
34- # If the locked_period is greater than the allowed, it probably
35- # means the test suite is stuck.
36- message = 'Tarmac lock file in place for %d seconds, PID: %d'
37- if locked_period > int(self.config.allowed_lock_period):
38- self.logger.info(message % (locked_period, get_pid()))
39- raise TarmacCommandError(message % (locked_period, get_pid()))
40+ except PidfileExistsError, e:
41+ message = "Is there another instance of Tarmac Running? %s" % e
42+ if e.lifetime > int(self.config.allowed_lock_period):
43+ # If the locked_period is greater than the allowed, it
44+ # probably means the test suite is stuck.
45+ self.logger.info(message)
46+ raise TarmacCommandError(message)
47 else:
48- self.logger.info(message % (locked_period, get_pid()))
49+ self.logger.info(message)
50 return
51-
52- target = Branch.create(
53- self.launchpad.branches.getByUrl(url=branch_url), self.config,
54- imply_commit_message=imply_commit_message)
55-
56- if not target.get_merge_proposals():
57- return
58-
59- self.logger.debug('Firing tarmac_pre_merge hook')
60- tarmac_hooks['tarmac_pre_merge'].fire(self, target)
61-
62 try:
63- # revno to restore to if bundle merge fails.
64- restore_revno = target.tree.branch.revno() + 1
65- results = target.merge_branches()
66- # Use a set to collect the proposals. We don't want
67- # duplicates since results generator will return the (source,
68- # proposal) tuple twice, once for each pre and post commit hook.
69- collected_proposals = set()
70- while True:
71- try:
72- (hook, source, proposal) = results.next()
73- collected_proposals.add((source, proposal))
74- self._fire_merge_hook(
75- results, hook, target, source, proposal)
76- except StopIteration:
77- break
78- tarmac_hooks['tarmac_bundle_merge'].fire(
79- self, target, restore_revno, collected_proposals)
80-
81- # This except is here because we need the else and can't have it
82- # without an except as well.
83- except:
84- raise
85- else:
86- self.logger.debug('Firing tarmac_post_merge hook')
87- tarmac_hooks['tarmac_post_merge'].fire(self, target)
88- # Push target.tree.branch to location branch_url.
89- target.push()
90+ target = Branch.create(
91+ self.launchpad.branches.getByUrl(url=branch_url), self.config,
92+ imply_commit_message=imply_commit_message)
93+
94+ if not target.get_merge_proposals():
95+ return
96+
97+ self.logger.debug('Firing tarmac_pre_merge hook')
98+ tarmac_hooks['tarmac_pre_merge'].fire(self, target)
99+
100+ try:
101+ # revno to restore to if bundle merge fails.
102+ restore_revno = target.tree.branch.revno() + 1
103+ results = target.merge_branches()
104+ collected_proposals = []
105+ while True:
106+ try:
107+ (hook, source, proposal) = results.next()
108+ if hook == 'tarmac_post_commit':
109+ # We only want the proposals that have not been
110+ # rejected.
111+ collected_proposals.append((source, proposal))
112+ self._fire_merge_hook(
113+ results, hook, target, source, proposal)
114+ except StopIteration:
115+ break
116+ if collected_proposals:
117+ # We actually have something to do!
118+ # This hook is probably the one found in
119+ # plugins/bundlecommand.py.
120+ tarmac_hooks['tarmac_bundle_merge'].fire(
121+ self, target, restore_revno, collected_proposals)
122+ self.logger.debug('Firing tarmac_post_merge hook')
123+ tarmac_hooks['tarmac_post_merge'].fire(self, target)
124+ # Push target.tree.branch to location branch_url.
125+ target.push()
126+ finally:
127+ target.cleanup()
128 finally:
129- target.cleanup()
130+ remove_pidfile()
131
132=== modified file 'tarmac/branch.py'
133--- tarmac/branch.py 2010-12-08 20:37:32 +0000
134+++ tarmac/branch.py 2011-01-03 21:02:11 +0000
135@@ -175,7 +175,7 @@
136 'source_branch': proposal.source_branch.bzr_identity})
137 source = Branch.create(
138 proposal.source_branch, TarmacConfig(), target=self)
139-
140+ generator_exception_thrown = False
141 try:
142 self.check_prerequisite(proposal)
143
144@@ -195,12 +195,22 @@
145 str(proposal.reviewed_revid))
146 # At this point we need to yield control back to the caller,
147 # so the proper hook can be called.
148- yield ("tarmac_pre_commit", source, proposal)
149+ try:
150+ yield ("tarmac_pre_commit", source, proposal)
151+ except:
152+ # Someone called .throw on us. They are objecting in
153+ # some way to the merge. We could just yield back to
154+ # the caller now, and then raise in the next
155+ # iteration, but instead we'll try to handle the
156+ # error first, and then yield. Therefore, we need
157+ # to remember that we are in this state, and then
158+ # re-raise.
159+ generator_exception_thrown = True
160+ raise
161
162 except TarmacMergeError, failure:
163 self.update_proposal_with_error(proposal, failure)
164 self.cleanup()
165- continue
166 except PointlessMerge:
167 self.logger.warn(
168 'Merging %(source)s into %(target)s would be '
169@@ -208,23 +218,27 @@
170 'source': proposal.source_branch.display_name,
171 'target': proposal.target_branch.display_name})
172 self.cleanup()
173- continue
174-
175- merge_url = get_review_url(proposal)
176- revprops = {'merge_url': merge_url}
177-
178- commit_message = proposal.commit_message
179- if commit_message is None and self.imply_commit_message:
180- commit_message = proposal.description
181- self.commit(commit_message,
182- revprops=revprops,
183- authors=source.authors,
184- reviews=self._get_reviews(proposal))
185-
186- # At this point we need to yield control back to the caller,
187- # so the proper hook can be called.
188- yield ("tarmac_post_commit", source, proposal)
189- self.cleanup()
190+ else:
191+ merge_url = get_review_url(proposal)
192+ revprops = {'merge_url': merge_url}
193+
194+ commit_message = proposal.commit_message
195+ if commit_message is None and self.imply_commit_message:
196+ commit_message = proposal.description
197+ self.commit(commit_message,
198+ revprops=revprops,
199+ authors=source.authors,
200+ reviews=self._get_reviews(proposal))
201+
202+ # At this point we need to yield control back to the caller,
203+ # so the proper hook can be called.
204+ yield ("tarmac_post_commit", source, proposal)
205+ self.cleanup()
206+ if generator_exception_thrown:
207+ # Someone called .throw when we announced the pre_commit
208+ # period. We handled it, and it's time to yield control
209+ # back to the caller.
210+ yield
211
212 def update_proposal(self, proposal, comment):
213 """Set proposal to Needs review and update with given comment."""
214
215=== modified file 'tarmac/pidfile.py'
216--- tarmac/pidfile.py 2010-12-14 20:47:53 +0000
217+++ tarmac/pidfile.py 2011-01-03 21:02:11 +0000
218@@ -7,6 +7,7 @@
219
220 import tempfile
221 import os
222+import time
223 import atexit
224 import sys
225 from signal import signal, SIGTERM
226@@ -14,6 +15,17 @@
227 from tarmac.config import TarmacConfig
228
229
230+class PidfileExistsError(RuntimeError):
231+ def __init__(self, pidfile, pid, lifetime):
232+ self.pidfile = pidfile
233+ self.pid = pid
234+ self.lifetime = lifetime
235+
236+ def __str__(self):
237+ return (
238+ 'File %r reports that PID %r has existed for %d seconds' % (
239+ self.pidfile, self.pid, self.lifetime))
240+
241 def make_pidfile():
242 """Write the current process id to a PID file.
243
244@@ -24,21 +36,27 @@
245 inside it.
246 """
247 pidfile = TarmacConfig().PID_FILE
248- if os.path.exists(pidfile):
249- raise RuntimeError("PID file %s already exists. Already running?" %
250- pidfile)
251-
252- atexit.register(remove_pidfile)
253- def remove_pidfile_handler(*ignored):
254- sys.exit(-1 * SIGTERM)
255- signal(SIGTERM, remove_pidfile_handler)
256-
257 fd, tempname = tempfile.mkstemp(dir=os.path.dirname(pidfile))
258 outf = os.fdopen(fd, 'w')
259 outf.write(str(os.getpid())+'\n')
260 outf.flush()
261 outf.close()
262- os.rename(tempname, pidfile)
263+ # This introduces a race condition. We're hoping it doesn't happen.
264+ if os.path.exists(pidfile):
265+ os.remove(tempname)
266+ raise PidfileExistsError(tempname, get_pid(), get_pidfile_lifetime())
267+ else:
268+ os.rename(tempname, pidfile)
269+
270+
271+# This could be used if the pidfile creation were not within a loop.
272+#
273+# def install_pidfile_removal():
274+#
275+# atexit.register(remove_pidfile)
276+# def remove_pidfile_handler(*ignored):
277+# sys.exit(-1 * SIGTERM)
278+# signal(SIGTERM, remove_pidfile_handler)
279
280
281 def remove_pidfile():
282@@ -73,5 +91,11 @@
283 except IOError:
284 return None
285 except ValueError:
286- raise ValueError("Invalid PID %s" % repr(pid))
287-
288+ raise ValueError("Invalid PID %r" % pid)
289+
290+
291+def get_pidfile_lifetime():
292+ """Return the number of seconds since the pidfile was created"""
293+ creation_time = os.stat(TarmacConfig().PID_FILE)[-1]
294+ now = time.time()
295+ return now - creation_time
296
297=== modified file 'tarmac/tests/test_commands.py'
298--- tarmac/tests/test_commands.py 2010-12-17 14:19:01 +0000
299+++ tarmac/tests/test_commands.py 2011-01-03 21:02:11 +0000
300@@ -383,6 +383,17 @@
301 self.install_plugin(plugin, hook_name)
302 return plugin
303
304+ def install_recorder_plugin(self, hook_name):
305+ class RecorderPlugin(TarmacPlugin):
306+ calls = []
307+
308+ def run(self, *args):
309+ self.__class__.calls.append(args)
310+
311+ plugin = RecorderPlugin()
312+ self.install_plugin(plugin, hook_name)
313+ return plugin
314+
315 def install_plugin(self, plugin, hook_name):
316 '''Install a plugin at the given hookpoint name.
317
318@@ -434,15 +445,22 @@
319 def test_plugin_merge_errors_are_handled(self):
320 # Setup
321 self.approve_all_proposals()
322- plugin = self.install_merge_error_injecting_plugin('tarmac_pre_commit')
323+ pre_commit_plugin = self.install_merge_error_injecting_plugin(
324+ 'tarmac_pre_commit')
325+ bundle_merge_plugin = self.install_recorder_plugin(
326+ 'tarmac_bundle_merge')
327
328 # Act
329 self.command.run(launchpad=self.launchpad)
330
331 # Verify
332- self.assertTrue(plugin.has_run)
333+ self.assertTrue(pre_commit_plugin.has_run)
334 self.assertIsInstance(self.error, TarmacMergeError)
335- self.assertEqual(self.error.comment, plugin.long_message)
336+ self.assertEqual(self.error.comment, pre_commit_plugin.long_message)
337+ # No commits were accepted, so we should have aborted the merge,
338+ # and not called the merge hook (which would usually run tests).
339+ self.assertEqual(bundle_merge_plugin.calls, [])
340+
341
342 # XXX matsubara 2010-12-17: Test commented out because we're not handling
343 # errors in the command itself but in the plugin. This is a temporary hack
344@@ -461,3 +479,20 @@
345 # self.assertIsInstance(self.error, TarmacMergeError)
346 # self.assertEqual(self.error.comment, plugin.long_message)
347 #
348+
349+
350+# This is a debug helper.
351+# nosetests eat stdout. This is a way to work around it. Use like this:
352+# with pdb():
353+# ...stuff you want to pdb through...
354+from contextlib import contextmanager
355+@contextmanager
356+def pdb():
357+ import pdb, sys
358+ test_stdout = sys.stdout
359+ sys.stdout = sys.__stdout__
360+ import pdb; pdb.set_trace()
361+ try:
362+ yield
363+ finally:
364+ sys.stdout = test_stdout

Subscribers

People subscribed via source and target branches

to all changes: