Opened 8 years ago

Closed 7 years ago

#3431 enhancement closed fixed (fixed)

Use of popen2 deprecated in py2.6

Reported by: Thijs Triemstra Owned by:
Priority: normal Milestone: Python-2.6
Component: core Keywords:
Cc: Thijs Triemstra, khorn, kelly Branch: branches/remove-popen2-3431-2
branch-diff, diff-cov, branch-cov, buildbot
Author: kelly, exarkun

Description

Trial reports DeprecationWarnings for popen2 in [source:trunk/twisted/test/test_process.py]. Deprecated in py2.6.

DeprecationWarning: The popen2 module is deprecated.  Use the subprocess module.

Attachments (1)

test-process-warning.patch (1.9 KB) - added by gas 7 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 8 years ago by Jean-Paul Calderone

We could drop this entirely in favor of running a program with known output. Of course, knowing the stderr of a Python program is pretty hard... Or maybe impossible. Maybe it could be /bin/sh, though.

comment:2 Changed 8 years ago by Glyph

Owner: changed from Glyph to Thijs Triemstra

comment:3 Changed 8 years ago by truekonrads

Cc: truekonrads added

comment:4 Changed 7 years ago by Thijs Triemstra

#3986 was a duplicate of this.

comment:5 Changed 7 years ago by Markon

Uhm, I've edited this line:

lsOut = popen2.popen3("/bin/ls ZZXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX")[2].read()

to:

    lsOut = subprocess.Popen("/bin/ls ZZXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
                             shell=True, stderr=subprocess.PIPE).communicate()[1]

And tried to run the tests using trial test_process.py. It passes all the tests (except SKIPPED tests). But I've used Python 2.5. Can someone test this new line with Py 2.6? ( you've to just add import subprocess, and go to the lines >= 2280, where you can see lsOut) Bye!

comment:6 Changed 7 years ago by khorn

Cc: khorn added

comment:7 Changed 7 years ago by khorn

this is a duplicate of #3900

comment:8 Changed 7 years ago by Thijs Triemstra

Owner: changed from Thijs Triemstra to khorn

Un-assigning.

Changed 7 years ago by gas

Attachment: test-process-warning.patch added

comment:9 Changed 7 years ago by gas

Cc: kelly added
Keywords: Review added
Owner: khorn deleted

Have copied over kelly patch from #3900. It appears to work running trial in no extra errors running trial against test_process.py

comment:10 Changed 7 years ago by Jean-Paul Calderone

Keywords: review added; Review removed

Thanks. Note that the keywords field is case sensitive, unfortunately.

comment:11 Changed 7 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/remove-popen2-3431

(In [27975]) Branching to 'remove-popen2-3431'

comment:12 Changed 7 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to Jean-Paul Calderone

I guess the branch is ready for review so there we go.

  • I know it's for internal use but twisted/python/_release.py (and it's tests) still uses popen2, bonus points for fixing that one as well..

Modified files need to have an updated copyright header (2010).

comment:13 Changed 7 years ago by Jean-Paul Calderone

The branch has another issue as well. Somehow it fails a test in another module:

===============================================================================
[FAIL]: twisted.test.test_tcp_internals.PlatformAssumptionsTestCase.test_acceptOutOfFiles

Traceback (most recent call last):
  File "/var/lib/buildbot/bigdog24-twisted/ubuntu64-py2.5-select/Twisted/twisted/test/test_tcp_internals.py", line 97, in test_acceptOutOfFiles
    self.assertIn(exc.args[0], (EMFILE, ENOBUFS))
twisted.trial.unittest.FailTest: 9 not in (24, 105)
-------------------------------------------------------------------------------

comment:14 Changed 7 years ago by Thijs Triemstra

Author: exarkunexarkun, kelly

comment:15 Changed 7 years ago by Jean-Paul Calderone

The test failure comes from a pipe leak in some versions of pygtk. It's only related to the change in this branch in that somehow this change disturbs things slightly and causes the existing problem to become visible.

There is a problem with test_acceptOutOfFiles that should be fixed - it shouldn't close its port if it can't create any other sockets - but I think that will just re-mask the underlying problem.

comment:16 Changed 7 years ago by Jean-Paul Calderone

I replaced the other usage of popen2 as well. However, merging this would still turn 3 supported builders red.

Arguably they shouldn't really be supported - they have a version of pygtk which leaks file descriptors. What we can do about this, though, isn't obvious to me.

comment:17 Changed 7 years ago by Thijs Triemstra

#4294 was a duplicate of this.

comment:18 in reply to:  16 Changed 7 years ago by Thijs Triemstra

Keywords: review added
Owner: Jean-Paul Calderone deleted

Replying to exarkun:

I replaced the other usage of popen2 as well. However, merging this would still turn 3 supported builders red.

Arguably they shouldn't really be supported - they have a version of pygtk which leaks file descriptors. What we can do about this, though, isn't obvious to me.

Let's contact the maintainers of those buildslaves so they can upgrade their version of pygtk? I think those issues shouldn't block this ticket though and I'm putting it back up for review.

comment:19 Changed 7 years ago by truekonrads

Cc: truekonrads removed

comment:20 Changed 7 years ago by TimAllen

Owner: set to TimAllen

comment:21 Changed 7 years ago by TimAllen

Keywords: review removed
Owner: changed from TimAllen to Jean-Paul Calderone

Looks good to me.

comment:22 Changed 7 years ago by Jean-Paul Calderone

Author: exarkun, kellykelly, exarkun
Branch: branches/remove-popen2-3431branches/remove-popen2-3431-2

(In [29042]) Branching to 'remove-popen2-3431-2'

comment:23 Changed 7 years ago by Jean-Paul Calderone

(In [29075]) Raise the artificial limit to 512 to work-around the pygtk fd leak

refs #3431

See also https://bugs.launchpad.net/ubuntu/+source/gtk+2.0/+bug/363245

comment:24 Changed 7 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Made a few final changes. Nothing really huge. Would be nice for someone to take one last look, though. Most recent build results.

comment:25 Changed 7 years ago by Glyph

Owner: set to Jean-Paul Calderone

Just a few nits to pick, then merge:

  1. testStderr should be renamed test_stderr as long as you're touching it.
  2. "produces the correct result", really? testStderr's docstring should phrase that in a way that doesn't leave the "correct result" up to the imagination.

Other than that, looks good, merge.

comment:26 Changed 7 years ago by Glyph

Keywords: review removed

comment:27 Changed 7 years ago by Jean-Paul Calderone

(In [29077]) Fix testStderr a bit

refs #3431

comment:28 Changed 7 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [29078]) Merge remove-popen2-3431-2

Author: kelly, exarkun Reviewer: thijs, TimAllen, glyph Fixes: #3431

Remove the uses of the deprecated popen2 module, replacing them with the subprocess module.

Also, raise the fd limit set by a test in test_tcp_internals. This is really an unrelated change, but the popen2 changes in this branch shuffle things around just enough to make a separate existing issue more likely to arise. Raising this limit gives us some more time to fix the real cause of these other failures (pygtk file descriptor leak, see https://bugs.launchpad.net/ubuntu/+source/gtk+2.0/+bug/363245 for further details).

comment:29 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.