Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Jul 26, 2012

Conversation

bfroehle
Copy link
Contributor

Fixes iptest brokenness caused by #2148.

Fixes iptest brokenness caused by ipython#2148.
@fperez
Copy link
Member

fperez commented Jul 25, 2012

Great, thanks for pitching in so fast. I got busy this morning with other things and was just going to start actually coding this...

except (OSError, NotImplementedError):
print('Cleaning stale PID: %d' % subp.pid)
subp.kill()
except: # (OSError, WindowsError) ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe print a message here with just the exception in case a stale process fails to clean up? That way the user may know there's something worth looking at manually.

@fperez
Copy link
Member

fperez commented Jul 25, 2012

The logic looks clean to me, I just have one small suggestion about improving feedback at the end. @minrk, do you have a windows VM to test this one on?

Sorry again for the mixup!

@bfroehle
Copy link
Contributor Author

I added a warning message if the process could not be killed (i.e. subp.poll() returns None). Okay, let's try some testing now. :)

@fperez
Copy link
Member

fperez commented Jul 26, 2012

@minrk, @jstenar, @jdmarch: I think you're our only 3 devs with Win VMs/machines available. If any of you can confirm this passes the test suite on Windows, we'll go ahead and merge.

@minrk
Copy link
Member

minrk commented Jul 26, 2012

Testing in a VM now...

@fperez
Copy link
Member

fperez commented Jul 26, 2012

BTW, I may soon get a box where we can set up a permanent windows VM for CI and remote access by all of us. I'll ping the list when/if it happens.

@fperez
Copy link
Member

fperez commented Jul 26, 2012

Test results for commit 8f4c54b merged into master
Platform: linux2

  • python2.7: OK
  • python3.2: OK (libraries not available: cython matplotlib oct2py pymongo rpy2 wx wx.aui)

Not available for testing: python2.6

@minrk
Copy link
Member

minrk commented Jul 26, 2012

Works fine on my Win7 VM. Thanks, @bfroehle!

@fperez
Copy link
Member

fperez commented Jul 26, 2012

OK, merging this to undo my bad karma score for the day. Thanks everyone. I'll eat my hat tonight :)

fperez added a commit that referenced this pull request Jul 26, 2012
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.
@fperez fperez merged commit b2fc42b into ipython:master Jul 26, 2012
@jdmarch
Copy link

jdmarch commented Jul 26, 2012

For the record, works on WinXP with Python 2.7.

@fperez
Copy link
Member

fperez commented Jul 26, 2012

Great, thanks for that check @jdmarch! Good to know we're back in win32's good graces :)

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants