Merge lp:~broder/ubuntu-dev-tools/fix-785854 into lp:~ubuntu-dev/ubuntu-dev-tools/trunk
- fix-785854
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 1107 |
Proposed branch: | lp:~broder/ubuntu-dev-tools/fix-785854 |
Merge into: | lp:~ubuntu-dev/ubuntu-dev-tools/trunk |
Diff against target: |
486 lines (+147/-19) (has conflicts) 21 files modified
404main (+2/-1) ack-sync (+1/-1) backportpackage (+1/-1) debian/changelog (+11/-0) dgetlp (+2/-1) get-branches (+1/-1) import-bug-from-debian (+1/-1) lp-project-upload (+2/-1) pbuilder-dist (+1/-1) syncpackage (+1/-1) ubuntu-iso (+2/-1) ubuntutools/archive.py (+1/-1) ubuntutools/builder.py (+1/-1) ubuntutools/misc.py (+1/-1) ubuntutools/requestsync/common.py (+2/-1) ubuntutools/requestsync/mail.py (+1/-1) ubuntutools/sponsor_patch/patch.py (+2/-1) ubuntutools/sponsor_patch/sponsor_patch.py (+1/-1) ubuntutools/subprocess.py (+109/-0) ubuntutools/test/test_help.py (+1/-1) ubuntutools/test/test_pylint.py (+3/-1) Text conflict in debian/changelog |
To merge this branch: | bzr merge lp:~broder/ubuntu-dev-tools/fix-785854 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Barry Warsaw (community) | Approve | ||
Review via email: mp+65139@code.launchpad.net |
This proposal supersedes a proposal from 2011-05-24.
Commit message
Description of the change
Barry Warsaw (barry) wrote : Posted in a previous version of this proposal | # |
Evan Broder (broder) wrote : | # |
I don't know of any problems with making ubuntutools.
As for the design consideration, I really do think there's value in making it easy to drop in this replacement that has better defaults, since the fd- and signal-related bugs around subprocess are easy to forget about.
Certainly I don't know of any cases where you *wouldn't* want to reset the signal handlers (since anything that expects them to be set specially - like, say, a Python process - will just set them again on startup).
I think close_fds isn't quite as totally uncontroversial, but I've also seen way more weird problems caused by passing fds to children (e.g. the other end of process 1's stdin ends up in process 2's fd space, even though parent process has its copy of that fd, leading to process 1 hanging until process 2 exits). When you deliberately mean to pass fds through, I think it's fairly obvious what's going wrong.
So I think that it's good to make it incredibly easy to use ubuntutools.
Stefano Rivera (stefanor) wrote : | # |
Looks reasonable, although running inspect is a bit icky.
Do we not need some copyright attribution on code copied verbatim from Python?
As I understand the close_fds issue, closing them is sensible on Linux but possibly not on Windows. We can afford to not care about that case.
ubuntutools/
Barry Warsaw (barry) wrote : | # |
On Jun 21, 2011, at 08:18 AM, Stefano Rivera wrote:
>Do we not need some copyright attribution on code copied verbatim from
>Python?
Probably so. It would also be a good-citizen thing to do.
@Evan, thanks for following up. I'll defer to you on the design decisions.
Thanks for making it a subclass.
review approve
Preview Diff
1 | === modified file '404main' |
2 | --- 404main 2010-12-27 19:32:07 +0000 |
3 | +++ 404main 2011-06-19 21:57:28 +0000 |
4 | @@ -24,12 +24,13 @@ |
5 | # This script is used to check if a package and all its build |
6 | # dependencies are in main or not. |
7 | |
8 | -import subprocess |
9 | import sys |
10 | |
11 | import apt_pkg |
12 | import apt |
13 | |
14 | +from ubuntutools import subprocess |
15 | + |
16 | def process_deps(cache, deps): |
17 | """Takes a list of (build) dependencies and processes it.""" |
18 | |
19 | |
20 | === modified file 'ack-sync' |
21 | --- ack-sync 2011-05-23 21:44:45 +0000 |
22 | +++ ack-sync 2011-06-19 21:57:28 +0000 |
23 | @@ -22,7 +22,6 @@ |
24 | import lazr.restfulclient |
25 | import os |
26 | import re |
27 | -import subprocess |
28 | import sys |
29 | import logging |
30 | import glob |
31 | @@ -31,6 +30,7 @@ |
32 | from launchpadlib.launchpad import Launchpad |
33 | |
34 | from ubuntutools.config import UDTConfig |
35 | +from ubuntutools import subprocess |
36 | |
37 | COMMAND_LINE_SYNTAX_ERROR = 1 |
38 | VERSION_DETECTION_FAILED = 2 |
39 | |
40 | === modified file 'backportpackage' |
41 | --- backportpackage 2011-05-28 17:41:07 +0000 |
42 | +++ backportpackage 2011-06-19 21:57:28 +0000 |
43 | @@ -21,7 +21,6 @@ |
44 | import optparse |
45 | import os |
46 | import shutil |
47 | -import subprocess |
48 | import sys |
49 | import tempfile |
50 | |
51 | @@ -34,6 +33,7 @@ |
52 | from ubuntutools.config import UDTConfig, ubu_email |
53 | from ubuntutools.builder import get_builder |
54 | from ubuntutools.question import YesNoQuestion |
55 | +from ubuntutools import subprocess |
56 | |
57 | def error(msg): |
58 | Logger.error(msg) |
59 | |
60 | === modified file 'debian/changelog' |
61 | --- debian/changelog 2011-06-16 19:02:04 +0000 |
62 | +++ debian/changelog 2011-06-19 21:57:28 +0000 |
63 | @@ -47,7 +47,18 @@ |
64 | * mk-sbuild: |
65 | - maintainer_name isn't mandatory any more (LP: #787051) |
66 | |
67 | +<<<<<<< TREE |
68 | -- Benjamin Drung <bdrung@debian.org> Wed, 25 May 2011 18:27:46 +0200 |
69 | +======= |
70 | + [ Evan Broder ] |
71 | + * ubuntutools.subprocess: |
72 | + - New drop-in replacement wrapper module around subprocess that |
73 | + backports the restore_signals kwarg and defaults close_fds=True |
74 | + - Switch everything previously using subprocess to use |
75 | + ubuntutools.subprocess instead (LP: #785854) |
76 | + |
77 | + -- Evan Broder <evan@ebroder.net> Tue, 24 May 2011 20:21:47 +0200 |
78 | +>>>>>>> MERGE-SOURCE |
79 | |
80 | ubuntu-dev-tools (0.123) unstable; urgency=low |
81 | |
82 | |
83 | === modified file 'dgetlp' |
84 | --- dgetlp 2011-02-12 22:08:15 +0000 |
85 | +++ dgetlp 2011-06-19 21:57:28 +0000 |
86 | @@ -32,7 +32,6 @@ |
87 | import hashlib |
88 | import optparse |
89 | import os |
90 | -import subprocess |
91 | import sys |
92 | import urllib2 |
93 | |
94 | @@ -43,6 +42,8 @@ |
95 | "use this utility.") |
96 | sys.exit(1) |
97 | |
98 | +from ubuntutools import subprocess |
99 | + |
100 | USAGE = u"""Usage: %prog [-d|(-v|-q)] <Launchpad URL> |
101 | |
102 | This scripts simulates «dget»'s behaviour for files hosted at |
103 | |
104 | === modified file 'get-branches' |
105 | --- get-branches 2010-12-27 19:49:00 +0000 |
106 | +++ get-branches 2011-06-19 21:57:28 +0000 |
107 | @@ -25,10 +25,10 @@ |
108 | # |
109 | |
110 | import os |
111 | -import subprocess |
112 | import sys |
113 | from optparse import OptionParser |
114 | from ubuntutools.lp.lpapicache import PersonTeam |
115 | +from ubuntutools import subprocess |
116 | |
117 | def main(): |
118 | usage = "Usage: %prog [-d <directory>] -t <team> [-o <operation>]" |
119 | |
120 | === modified file 'import-bug-from-debian' |
121 | --- import-bug-from-debian 2011-02-28 22:30:54 +0000 |
122 | +++ import-bug-from-debian 2011-06-19 21:57:28 +0000 |
123 | @@ -25,7 +25,6 @@ |
124 | from optparse import OptionParser, SUPPRESS_HELP |
125 | import re |
126 | import sys |
127 | -import subprocess |
128 | |
129 | try: |
130 | import SOAPpy |
131 | @@ -37,6 +36,7 @@ |
132 | from launchpadlib.launchpad import Launchpad |
133 | |
134 | from ubuntutools.config import UDTConfig |
135 | +from ubuntutools import subprocess |
136 | |
137 | def main(): |
138 | bug_re = re.compile(r"bug=(\d+)") |
139 | |
140 | === modified file 'lp-project-upload' |
141 | --- lp-project-upload 2011-06-09 11:36:06 +0000 |
142 | +++ lp-project-upload 2011-06-19 21:57:28 +0000 |
143 | @@ -22,13 +22,14 @@ |
144 | |
145 | import datetime |
146 | import os |
147 | -import subprocess |
148 | import sys |
149 | import tempfile |
150 | |
151 | from launchpadlib.launchpad import Launchpad |
152 | from launchpadlib.errors import HTTPError |
153 | |
154 | +from ubuntutools import subprocess |
155 | + |
156 | def create_release(project, version): |
157 | '''Create new release and milestone for LP project.''' |
158 | |
159 | |
160 | === modified file 'pbuilder-dist' |
161 | --- pbuilder-dist 2011-05-23 21:41:00 +0000 |
162 | +++ pbuilder-dist 2011-06-19 21:57:28 +0000 |
163 | @@ -30,13 +30,13 @@ |
164 | # that the target distribution is always meant to be Ubuntu Hardy. |
165 | |
166 | import os |
167 | -import subprocess |
168 | import sys |
169 | |
170 | from devscripts.logger import Logger |
171 | |
172 | from ubuntutools.distro_info import DebianDistroInfo |
173 | import ubuntutools.misc |
174 | +from ubuntutools import subprocess |
175 | |
176 | class PbuilderDist: |
177 | def __init__(self, builder): |
178 | |
179 | === modified file 'syncpackage' |
180 | --- syncpackage 2011-05-23 21:41:00 +0000 |
181 | +++ syncpackage 2011-06-19 21:57:28 +0000 |
182 | @@ -25,7 +25,6 @@ |
183 | import optparse |
184 | import os |
185 | import shutil |
186 | -import subprocess |
187 | import sys |
188 | |
189 | from devscripts.logger import Logger |
190 | @@ -38,6 +37,7 @@ |
191 | from ubuntutools.requestsync.lp import getDebianSrcPkg, getUbuntuSrcPkg |
192 | from ubuntutools.lp import udtexceptions |
193 | from ubuntutools.lp.lpapicache import Launchpad |
194 | +from ubuntutools import subprocess |
195 | |
196 | |
197 | class Version(debian.debian_support.Version): |
198 | |
199 | === modified file 'ubuntu-iso' |
200 | --- ubuntu-iso 2010-12-27 13:01:03 +0000 |
201 | +++ ubuntu-iso 2011-06-19 21:57:28 +0000 |
202 | @@ -21,7 +21,8 @@ |
203 | # ################################################################## |
204 | |
205 | import sys |
206 | -import subprocess |
207 | + |
208 | +from ubuntutools import subprocess |
209 | |
210 | def extract(iso, path): |
211 | command = ['isoinfo', '-R', '-i', iso, '-x', path] |
212 | |
213 | === modified file 'ubuntutools/archive.py' |
214 | --- ubuntutools/archive.py 2011-05-30 13:55:15 +0000 |
215 | +++ ubuntutools/archive.py 2011-06-19 21:57:28 +0000 |
216 | @@ -31,7 +31,6 @@ |
217 | |
218 | import hashlib |
219 | import os.path |
220 | -import subprocess |
221 | import urllib2 |
222 | import urlparse |
223 | import re |
224 | @@ -45,6 +44,7 @@ |
225 | from ubuntutools.config import UDTConfig |
226 | from ubuntutools.lp.lpapicache import (Launchpad, Distribution, |
227 | SourcePackagePublishingHistory) |
228 | +from ubuntutools import subprocess |
229 | |
230 | class DownloadError(Exception): |
231 | "Unable to pull a source package" |
232 | |
233 | === modified file 'ubuntutools/builder.py' |
234 | --- ubuntutools/builder.py 2011-05-23 21:41:00 +0000 |
235 | +++ ubuntutools/builder.py 2011-06-19 21:57:28 +0000 |
236 | @@ -19,9 +19,9 @@ |
237 | # |
238 | |
239 | import os |
240 | -import subprocess |
241 | |
242 | from devscripts.logger import Logger |
243 | +from ubuntutools import subprocess |
244 | |
245 | def _build_preparation(result_directory): |
246 | """prepares the builder for building a package""" |
247 | |
248 | === modified file 'ubuntutools/misc.py' |
249 | --- ubuntutools/misc.py 2011-02-13 14:15:24 +0000 |
250 | +++ ubuntutools/misc.py 2011-06-19 21:57:28 +0000 |
251 | @@ -25,10 +25,10 @@ |
252 | import locale |
253 | import os |
254 | import os.path |
255 | -from subprocess import Popen, PIPE |
256 | import sys |
257 | |
258 | from ubuntutools.lp.udtexceptions import PocketDoesNotExistError |
259 | +from ubuntutools.subprocess import Popen, PIPE |
260 | |
261 | _system_distribution = None |
262 | def system_distribution(): |
263 | |
264 | === modified file 'ubuntutools/requestsync/common.py' |
265 | --- ubuntutools/requestsync/common.py 2010-12-22 22:01:39 +0000 |
266 | +++ ubuntutools/requestsync/common.py 2011-06-19 21:57:28 +0000 |
267 | @@ -24,9 +24,10 @@ |
268 | import urllib2 |
269 | import re |
270 | import tempfile |
271 | -import subprocess |
272 | from debian.changelog import Changelog |
273 | |
274 | +from ubuntutools import subprocess |
275 | + |
276 | def raw_input_exit_on_ctrlc(*args, **kwargs): |
277 | ''' |
278 | A wrapper around raw_input() to exit with a normalized message on Control-C |
279 | |
280 | === modified file 'ubuntutools/requestsync/mail.py' |
281 | --- ubuntutools/requestsync/mail.py 2011-03-05 22:48:24 +0000 |
282 | +++ ubuntutools/requestsync/mail.py 2011-06-19 21:57:28 +0000 |
283 | @@ -22,13 +22,13 @@ |
284 | |
285 | import os |
286 | import sys |
287 | -import subprocess |
288 | import smtplib |
289 | import socket |
290 | from debian.changelog import Version |
291 | from ubuntutools.archive import rmadison, FakeSPPH |
292 | from ubuntutools.distro_info import DebianDistroInfo |
293 | from ubuntutools.requestsync.common import raw_input_exit_on_ctrlc |
294 | +from ubuntutools import subprocess |
295 | from ubuntutools.lp.udtexceptions import PackageNotFoundException |
296 | |
297 | __all__ = [ |
298 | |
299 | === modified file 'ubuntutools/sponsor_patch/patch.py' |
300 | --- ubuntutools/sponsor_patch/patch.py 2010-12-27 14:20:49 +0000 |
301 | +++ ubuntutools/sponsor_patch/patch.py 2011-06-19 21:57:28 +0000 |
302 | @@ -16,7 +16,8 @@ |
303 | # OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. |
304 | |
305 | import os |
306 | -import subprocess |
307 | + |
308 | +from ubuntutools import subprocess |
309 | |
310 | class Patch(object): |
311 | def __init__(self, patch_file): |
312 | |
313 | === modified file 'ubuntutools/sponsor_patch/sponsor_patch.py' |
314 | --- ubuntutools/sponsor_patch/sponsor_patch.py 2011-06-03 17:18:38 +0000 |
315 | +++ ubuntutools/sponsor_patch/sponsor_patch.py 2011-06-19 21:57:28 +0000 |
316 | @@ -19,7 +19,6 @@ |
317 | import pwd |
318 | import re |
319 | import shutil |
320 | -import subprocess |
321 | import sys |
322 | |
323 | import debian.changelog |
324 | @@ -28,6 +27,7 @@ |
325 | |
326 | from devscripts.logger import Logger |
327 | |
328 | +from ubuntutools import subprocess |
329 | from ubuntutools.update_maintainer import update_maintainer |
330 | from ubuntutools.question import Question, YesNoQuestion, input_number |
331 | |
332 | |
333 | === added file 'ubuntutools/subprocess.py' |
334 | --- ubuntutools/subprocess.py 1970-01-01 00:00:00 +0000 |
335 | +++ ubuntutools/subprocess.py 2011-06-19 21:57:28 +0000 |
336 | @@ -0,0 +1,109 @@ |
337 | +"""Drop-in replacement for subprocess with better defaults |
338 | + |
339 | +This is an API-compatible replacement for the built-in subprocess |
340 | +module whose defaults better line up with our tastes. |
341 | + |
342 | +In particular, it: |
343 | + - Adds support for the restore_signals flag if subprocess itself |
344 | + doesn't support it |
345 | + - Defaults close_fds to True |
346 | +""" |
347 | + |
348 | + |
349 | +from __future__ import absolute_import |
350 | + |
351 | +import inspect |
352 | +import signal |
353 | +import subprocess |
354 | + |
355 | +from subprocess import PIPE, STDOUT, CalledProcessError |
356 | + |
357 | +__all__ = ['Popen', 'call', 'check_call', 'check_output', 'CalledProcessError', 'PIPE', 'STDOUT'] |
358 | + |
359 | + |
360 | +class Popen(subprocess.Popen): |
361 | + def __init__(self, *args, **kwargs): |
362 | + kwargs.setdefault('close_fds', True) |
363 | + |
364 | + if 'restore_signals' not in inspect.getargspec(subprocess.Popen.__init__)[0]: |
365 | + given_preexec_fn = kwargs.pop('preexec_fn', None) |
366 | + restore_signals = kwargs.pop('restore_signals', True) |
367 | + def preexec_fn(): |
368 | + if restore_signals: |
369 | + for sig in ('SIGPIPE', 'SIGXFZ', 'SIGXFSZ'): |
370 | + if hasattr(signal, sig): |
371 | + signal.signal(getattr(signal, sig), |
372 | + signal.SIG_DFL) |
373 | + |
374 | + if given_preexec_fn: |
375 | + given_preexec_fn() |
376 | + kwargs['preexec_fn'] = preexec_fn |
377 | + |
378 | + subprocess.Popen.__init__(self, *args, **kwargs) |
379 | + |
380 | + |
381 | +# call, check_call, and check_output are copied directly from the |
382 | +# subprocess module shipped with Python 2.7.1-5ubuntu2 |
383 | + |
384 | + |
385 | +def call(*popenargs, **kwargs): |
386 | + """Run command with arguments. Wait for command to complete, then |
387 | + return the returncode attribute. |
388 | + |
389 | + The arguments are the same as for the Popen constructor. Example: |
390 | + |
391 | + retcode = call(["ls", "-l"]) |
392 | + """ |
393 | + return Popen(*popenargs, **kwargs).wait() |
394 | + |
395 | + |
396 | +def check_call(*popenargs, **kwargs): |
397 | + """Run command with arguments. Wait for command to complete. If |
398 | + the exit code was zero then return, otherwise raise |
399 | + CalledProcessError. The CalledProcessError object will have the |
400 | + return code in the returncode attribute. |
401 | + |
402 | + The arguments are the same as for the Popen constructor. Example: |
403 | + |
404 | + check_call(["ls", "-l"]) |
405 | + """ |
406 | + retcode = call(*popenargs, **kwargs) |
407 | + if retcode: |
408 | + cmd = kwargs.get("args") |
409 | + if cmd is None: |
410 | + cmd = popenargs[0] |
411 | + raise CalledProcessError(retcode, cmd) |
412 | + return 0 |
413 | + |
414 | + |
415 | +def check_output(*popenargs, **kwargs): |
416 | + r"""Run command with arguments and return its output as a byte string. |
417 | + |
418 | + If the exit code was non-zero it raises a CalledProcessError. The |
419 | + CalledProcessError object will have the return code in the returncode |
420 | + attribute and output in the output attribute. |
421 | + |
422 | + The arguments are the same as for the Popen constructor. Example: |
423 | + |
424 | + >>> check_output(["ls", "-l", "/dev/null"]) |
425 | + 'crw-rw-rw- 1 root root 1, 3 Oct 18 2007 /dev/null\n' |
426 | + |
427 | + The stdout argument is not allowed as it is used internally. |
428 | + To capture standard error in the result, use stderr=STDOUT. |
429 | + |
430 | + >>> check_output(["/bin/sh", "-c", |
431 | + ... "ls -l non_existent_file ; exit 0"], |
432 | + ... stderr=STDOUT) |
433 | + 'ls: non_existent_file: No such file or directory\n' |
434 | + """ |
435 | + if 'stdout' in kwargs: |
436 | + raise ValueError('stdout argument not allowed, it will be overridden.') |
437 | + process = Popen(stdout=PIPE, *popenargs, **kwargs) |
438 | + output, unused_err = process.communicate() |
439 | + retcode = process.poll() |
440 | + if retcode: |
441 | + cmd = kwargs.get("args") |
442 | + if cmd is None: |
443 | + cmd = popenargs[0] |
444 | + raise CalledProcessError(retcode, cmd, output=output) |
445 | + return output |
446 | |
447 | === modified file 'ubuntutools/test/test_help.py' |
448 | --- ubuntutools/test/test_help.py 2011-05-21 15:53:16 +0000 |
449 | +++ ubuntutools/test/test_help.py 2011-06-19 21:57:28 +0000 |
450 | @@ -18,10 +18,10 @@ |
451 | import os |
452 | import select |
453 | import signal |
454 | -import subprocess |
455 | import time |
456 | |
457 | import setup |
458 | +from ubuntutools import subprocess |
459 | from ubuntutools.test import unittest |
460 | |
461 | BLACKLIST = { |
462 | |
463 | === modified file 'ubuntutools/test/test_pylint.py' |
464 | --- ubuntutools/test/test_pylint.py 2011-05-30 13:55:15 +0000 |
465 | +++ ubuntutools/test/test_pylint.py 2011-06-19 21:57:28 +0000 |
466 | @@ -15,10 +15,10 @@ |
467 | # PERFORMANCE OF THIS SOFTWARE. |
468 | |
469 | import re |
470 | -import subprocess |
471 | |
472 | import setup |
473 | from ubuntutools.test import unittest |
474 | +from ubuntutools import subprocess |
475 | |
476 | WHITELIST = [re.compile(': %s$' % x) for x in ( |
477 | # Wildcard import: |
478 | @@ -28,6 +28,8 @@ |
479 | # mox: |
480 | r"Instance of '.+' has no '(WithSideEffects|MultipleTimes|AndReturn)' " |
481 | r"member", |
482 | + # pylint doesn't like *args/**kwargs |
483 | + r"Instance of 'Popen' has no '.*' member", |
484 | )] |
485 | |
486 | class PylintTestCase(unittest.TestCase): |
Hi Evan,
I totally see why you've added this, and changed the code to use it. Thanks for making ubuntu-dev-tools better! Of course, once we migrate u-d-t to Python 3.2 this change won't be necessary <wink>.
I'm a little concerned that what you've written isn't entirely API compatible with Python's version. Python's subprocess.Popen is a class while ubuntutools. subprocess. Popen is a function. I have no idea whether people actually try to subclass subprocess.Popen, and within u-d-t it's easy enough to guarantee this, but is it possible there are would be external clients that could be broken by this? It makes me uncomfortable that your docs claim API compatibility when it's really not.
Two thoughts: could this be implemented as a subprocess.Popen subclass?
Or, would it make sense to just use a different API rather than promoting something that looks like stdlib subprocess.Popen but isn't quite? The way you have it does mean that existing code in u-d-t doesn't have to change, except for the import statement. I'm not sure whether that's good or not ;).
You might also call inspect. getargspec( ) at module scope and then not redefine, but simply expose the stdlib subprocess functions directly in ubuntutools. subprocess. Popen.
Anyway, just some thoughts. At the least, please do indicate in the docs about this difference. I'm happy to chat about alternatives in IRC if you want.