win32 iptest: Use subprocess.Popen() instead of os.system(). #2148

bfroehle · 2012-07-17T00:38:00Z

(Do not merge, needs testing in Windows).

The call to os.system in iptest prevents us from easily setting $IPYTHONDIR to a temporary directory. In theory we should be able to use subprocess.Popen instead of os.system in Windows. This would unify the code structure and make it easy to pass in a different environment.

Note that Python < 2.7 does not have os.kill. I've attempted to work around this by using ctypes to kill any living processes.

I haven't actually tested any of this in Windows. Perhaps @jdmarch or @jstenar or somebody else using windows could run some tests with it.

fperez · 2012-07-20T13:06:06Z

@jdmarch, @jstenar, any chance you could take this one for a spin on Windows and let us know what happens?

jdmarch · 2012-07-20T20:32:36Z

Will do, but it might not be until c 25 July.

minrk · 2012-07-21T04:14:17Z

IPython/testing/iptest.py

+    def _kill(pid):
+        """Attempt to kill the process on a best effort basis."""
+        if hasattr(os, 'kill'):
+            os.kill(pid, signal.SIGKILL)


Windows doesn't have signal.SIGKILL (TERM is equivalent to KILL on Windows).

Perhaps reorganizing a bit so that you are calling the p.terminate() / p.kill() methods on the Popen object itself, which hide the platform differences.

Okay, thanks for the update. Is there a kill or terminate method on Popen objects in Python 2.6 in Windows?

Might be time for me to dig out an old copy of windows xp for testing....

Yes, the kill/terminate methods should be defined everywhere we run, and on Windows kill is just an alias to terminate:

http://docs.python.org/library/subprocess.html#subprocess.Popen.terminate

jstenar · 2012-07-21T08:28:27Z

I can take a look on sunday

jstenar · 2012-07-22T19:45:17Z

I ran the testsuite with python 2.7 without errors.

I got one failure on python 3.2 but I suspect it is not related to this PR

Has anyone else seen this?

FAIL: IPython.core.tests.test_profile.test_list_bundled_profiles
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\python32\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "C:\python32\lib\site-packages\ipython-0.14.dev-py3.2.egg\IPython\core\tests\test_profile.py", line 151, in test_list_bundled_profiles
    nt.assert_equals(bundled, bundled_true)
AssertionError: Lists differ: ['cluster', 'math', 'pysh', 'p... != ['cluster', 'math', 'pysh', 's...

First differing element 3:
python3
sympy

First list contains 1 additional elements.
First extra element 4:
sympy

- ['cluster', 'math', 'pysh', 'python3', 'sympy']
?                            -----------

+ ['cluster', 'math', 'pysh', 'sympy']
    """Fail immediately, with the given message."""
>>  raise self.failureException("Lists differ: ['cluster', 'math', 'pysh', 'p... != ['cluster', 'math', 'pysh', 's...\n\nFirst differing element 3:\npython3\nsympy\n\nFirst list contains 1 additional elements.\nFirst extra element 4:\nsympy\n\n- ['cluster', 'math', 'pysh', 'python3', 'sympy']\n?                            -----------\n\n+ ['cluster', 'math', 'pysh', 'sympy']")

takluyver · 2012-07-22T21:18:39Z

I've seen that, I think it's just a leftover in the installation from when we had a separate Python 3 profile. If you remove the installation and clear the build/ and dist/ folders in the IPython source, then install again, it should have gone away.

jstenar · 2012-07-24T08:55:54Z

Following takluyver's suggestion the tests pass on python3.2 as well.

jdmarch · 2012-07-24T11:28:37Z

All tests pass on WinXP, Python 2.7, except for (unrelated) gh-2018

fperez · 2012-07-25T04:40:02Z

OK, thanks everyone for the windows testing, will merge now as code otherwise looks solid. Thanks, @bfroehle for this cleanup!

win32 iptest: Use subprocess.Popen() instead of os.system(). The call to `os.system` in `iptest` prevents us from easily setting `$IPYTHONDIR` to a temporary directory. In theory we should be able to use `subprocess.Popen` instead of `os.system` in Windows. This would unify the code structure and make it easy to pass in a different environment. Note that Python < 2.7 does not have `os.kill`. I've attempted to work around this by using `ctypes` to kill any living processes.

minrk · 2012-07-25T18:40:31Z

Why did this get merged without addressing the comments about using the Popen methods for terminate/kill?

This does not work on Win7, due to an AttributeError on signal.SIGKILL.

fperez · 2012-07-25T18:51:19Z

On Wed, Jul 25, 2012 at 11:40 AM, Min RK
reply@reply.github.com
wrote:

Why did this get merged without addressing the comments about using the Popen methods for terminate/kill?

ouch, my fault. I misread the discussion...

Will try to whip up something now.

bfroehle · 2012-07-25T22:00:44Z

@minrk: Is that what you had in mind for dealing with a lack of signal.SIGKILL in win32: bfroehle/fix_os_kill_win32

minrk · 2012-07-25T22:03:38Z

Precisely, yes.

Fixes iptest brokenness caused by ipython#2148.

@bfroehle

Work around lack of os.kill in win32. Fixes iptest brokenness on win32 caused by my having merged #2148 too hastily. Extra credit to @bfroehle and @minrk for working/testing the fix quickly.

Create a unique & temporary IPYTHONDIR for each testing group. Following #2148 (unification of Windows / Unix code in iptest), and in progress towards #1880 (Add parallelism to iptest & test_pr), this pull request launches each iptest test group with a unique & temporary `IPYTHONDIR`. This has two benefits: * Insulates the test suite from any craziness in your own configuration. (Try adding `import sys; sys.exit()` to your config file...). * Allows multiple test suites to be launched in parallel without the worry of conflicts.

win32 iptest: Use subprocess.Popen() instead of os.system(). The call to `os.system` in `iptest` prevents us from easily setting `$IPYTHONDIR` to a temporary directory. In theory we should be able to use `subprocess.Popen` instead of `os.system` in Windows. This would unify the code structure and make it easy to pass in a different environment. Note that Python < 2.7 does not have `os.kill`. I've attempted to work around this by using `ctypes` to kill any living processes.

Fixes iptest brokenness caused by ipython#2148.

@bfroehle

Work around lack of os.kill in win32. Fixes iptest brokenness on win32 caused by my having merged ipython#2148 too hastily. Extra credit to @bfroehle and @minrk for working/testing the fix quickly.

Create a unique & temporary IPYTHONDIR for each testing group. Following ipython#2148 (unification of Windows / Unix code in iptest), and in progress towards ipython#1880 (Add parallelism to iptest & test_pr), this pull request launches each iptest test group with a unique & temporary `IPYTHONDIR`. This has two benefits: * Insulates the test suite from any craziness in your own configuration. (Try adding `import sys; sys.exit()` to your config file...). * Allows multiple test suites to be launched in parallel without the worry of conflicts.

win32 iptest: Use subprocess.Popen() instead of os.system().

26f122a

bfroehle mentioned this pull request Jul 17, 2012

iptest: Use a temporary directory for IPYTHONDIR #1888

Closed

minrk reviewed Jul 21, 2012
View reviewed changes

ghost assigned bfroehle Jul 21, 2012

fperez merged commit 6ed621a into ipython:master Jul 25, 2012

bfroehle mentioned this pull request Jul 25, 2012

Create a unique & temporary IPYTHONDIR for each testing group. #2202

Merged

bfroehle added a commit to bfroehle/ipython that referenced this pull request Jul 25, 2012

Work around lack of os.kill in win32.

aa359dd

Fixes iptest brokenness caused by ipython#2148.

bfroehle mentioned this pull request Jul 25, 2012

Work around lack of os.kill in win32. #2204

Merged

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Work around lack of os.kill in win32.

d8abfc6

Fixes iptest brokenness caused by ipython#2148.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

win32 iptest: Use subprocess.Popen() instead of os.system(). #2148

win32 iptest: Use subprocess.Popen() instead of os.system(). #2148

bfroehle commented Jul 17, 2012

fperez commented Jul 20, 2012

jdmarch commented Jul 20, 2012

minrk Jul 21, 2012

bfroehle Jul 21, 2012

minrk Jul 21, 2012

jstenar commented Jul 21, 2012

jstenar commented Jul 22, 2012

takluyver commented Jul 22, 2012

jstenar commented Jul 24, 2012

jdmarch commented Jul 24, 2012

fperez commented Jul 25, 2012

minrk commented Jul 25, 2012

fperez commented Jul 25, 2012

bfroehle commented Jul 25, 2012

minrk commented Jul 25, 2012

win32 iptest: Use subprocess.Popen() instead of os.system(). #2148

win32 iptest: Use subprocess.Popen() instead of os.system(). #2148

Conversation

bfroehle commented Jul 17, 2012

fperez commented Jul 20, 2012

jdmarch commented Jul 20, 2012

minrk Jul 21, 2012

Choose a reason for hiding this comment

bfroehle Jul 21, 2012

Choose a reason for hiding this comment

minrk Jul 21, 2012

Choose a reason for hiding this comment

jstenar commented Jul 21, 2012

jstenar commented Jul 22, 2012

takluyver commented Jul 22, 2012

jstenar commented Jul 24, 2012

jdmarch commented Jul 24, 2012

fperez commented Jul 25, 2012

minrk commented Jul 25, 2012

fperez commented Jul 25, 2012

bfroehle commented Jul 25, 2012

minrk commented Jul 25, 2012