Opened 6 years ago

Closed 5 years ago

#3431 enhancement closed fixed (fixed)

Use of popen2 deprecated in py2.6

Reported by: thijs Owned by:
Priority: normal Milestone: Python-2.6
Component: core Keywords:
Cc: thijs, khorn, kelly Branch: branches/remove-popen2-3431-2
(diff, github, buildbot, log)
Author: kelly, exarkun Launchpad Bug:

Description

Trial reports DeprecationWarnings for popen2 in 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 5 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 6 years ago by exarkun

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 5 years ago by glyph

  • Owner changed from glyph to thijs

comment:3 Changed 5 years ago by truekonrads

  • Cc truekonrads added

comment:4 Changed 5 years ago by thijs

#3986 was a duplicate of this.

comment:5 Changed 5 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 5 years ago by khorn

  • Cc khorn added

comment:7 Changed 5 years ago by khorn

this is a duplicate of #3900

comment:8 Changed 5 years ago by thijs

  • Owner changed from thijs to khorn

Un-assigning.

Changed 5 years ago by gas

comment:9 Changed 5 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 5 years ago by exarkun

  • Keywords review added; Review removed

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

comment:11 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/remove-popen2-3431

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

comment:12 Changed 5 years ago by thijs

  • Keywords review removed
  • Owner set to exarkun

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 5 years ago by exarkun

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 5 years ago by thijs

  • Author changed from exarkun to exarkun, kelly

comment:15 Changed 5 years ago by exarkun

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 follow-up: Changed 5 years ago by 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.

comment:17 Changed 5 years ago by thijs

#4294 was a duplicate of this.

comment:18 in reply to: ↑ 16 Changed 5 years ago by thijs

  • Keywords review added
  • Owner exarkun 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 5 years ago by truekonrads

  • Cc truekonrads removed

comment:20 Changed 5 years ago by TimAllen

  • Owner set to TimAllen

comment:21 Changed 5 years ago by TimAllen

  • Keywords review removed
  • Owner changed from TimAllen to exarkun

Looks good to me.

comment:22 Changed 5 years ago by exarkun

  • Author changed from exarkun, kelly to kelly, exarkun
  • Branch changed from branches/remove-popen2-3431 to branches/remove-popen2-3431-2

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

comment:23 Changed 5 years ago by exarkun

(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 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 5 years ago by glyph

  • Owner set to exarkun

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 5 years ago by glyph

  • Keywords review removed

comment:27 Changed 5 years ago by exarkun

(In [29077]) Fix testStderr a bit

refs #3431

comment:28 Changed 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.