542000 - get_pids still depends on pywin32

Status: VERIFIED FIXED

(Testing :: Mozbase, defect)

Description

[Rain [:rain1] (inactive)]

http://github.com/mikeal/mozrunner/blob/master/mozrunner/__init__.py#L107

I don't seem to hit this while running Thunderbird tests, but the problem is still there.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I believe the problem we have here is the following line:

if os.name == 'nt' or sys.platform == 'cygwin':

Seems like it only occurs when you are using an old cygwin setup. Heather, can you please enlighten us on your setup?

Comment 2

[Heather Arthur [:harth]]

Running from Mozilla Build 1.4, and calling stop() on a Runner instance.

Comment 3

[Atul Varma [:atul]]

What's up with the references to win32api starting at line 127 of the same file?

http://github.com/mikeal/mozrunner/blob/master/mozrunner/__init__.py#L127

I'm not seeing that module imported anywhere, or a variable assigned to its name, etc.  Same with win32con, which is definitely a pywin32 module.  I guess those should both be pretty easy to migrate to ctypes, but still kinda weird that they're there...

Comment 4

[Atul Varma [:atul]]

I'm looking at alternatives to porting win32pdhutil.FindPerformanceAttributesByName() to ctypes, since it looks like doing that could involve a fair bit of work. It seems like it's just being used to get the process IDs of particular programs, which will then just be used to kill the programs in question--but there's a command-line utility that does the same thing, which comes with Windows XP and Server 2003 at the very least, at %windir%\system32\taskkill.exe.

Anyone have objections to just replacing these pywin32 calls with a command-line call to taskkill? Or do we still need to use mozrunner on win2k, which taskkill may not be shipped with?

Comment 5

[Atul Varma [:atul]]

Attachment: Proof-of-concept for a ctypes-based implementation of get_pids() and kill_process_by_name()

Never mind my last comment, I think we can accomplish the same thing using simpler win32 APIs. Here's a proof-of-concept that does the same thing using ctypes instead of pywin32.

Comment 6

[Atul Varma [:atul]]

I created this module on a Windows 7 VM, but need to test it out on an older version of Windows to make sure it works: MSDN docs seem to indicate that some of the API calls I found in psapi.dll may instead be found in kernel32.dll in earlier versions of Windows.

Comment 7

[cmtalbert]

I don't think mozmill automation needs to run on windows 2000, so don't let that block you.

Comment 8

[Atul Varma [:atul]]

Cool, thanks. I just ran the proof-of-concept code on my XP machine and it works without any changes, so I think I'll start integrating this into mozmill and issue a pull request to mikeal on github later today.

Comment 9

[Atul Varma [:atul]]

It looks like mikeal accepted the pull request and my fix is in his repo. Should we mark this bug as fixed?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks like http://github.com/mikeal/mozrunner/commit/80d1275c902c9afc08d481bbf52308c97d23a81f

Marking as fixed.

Comment 11

[Atul Varma [:atul]]

Awesome!

Um, I should note, though, that we have been experiencing a few other issues with Mozrunner, or at least with mozrunner.killableprocess.  The logic for killableprocess.Popen.wait(), in particular, seems unreliable and/or incorrect, and I've been working on a "better" version in bug 554842. While the patch is against the Jetpack SDK right now, I think it might be useful to integrate it into mozrunner proper. It also comes with some simple doctests that actually fail without the "better" version.

Another mozrunner-related bug I'm looking into is mentioned in bug 554570 comment 2.

I should probably just make a tracking bug for all Jetpack SDK-related mozrunner issues.

Comment 12

[cmtalbert]

(In reply to comment #11)
> Awesome!
> 
> Um, I should note, though, that we have been experiencing a few other issues
> with Mozrunner, or at least with mozrunner.killableprocess.  The logic for
> killableprocess.Popen.wait(), in particular, seems unreliable and/or incorrect,
> and I've been working on a "better" version in bug 554842. While the patch is
> against the Jetpack SDK right now, I think it might be useful to integrate it
> into mozrunner proper. It also comes with some simple doctests that actually
> fail without the "better" version.
> 
Awesome
> Another mozrunner-related bug I'm looking into is mentioned in bug 554570
> comment 2.
> 
cool.
> I should probably just make a tracking bug for all Jetpack SDK-related
> mozrunner issues.
That'd be nice, but also put [p1] in the whiteboard list of any bugs that you need fixed for jetpack so that they will get picked up in our queries for high priority mozmill system bugs.

Comment 13

[cmtalbert]

(In reply to comment #12)

> That'd be nice, but also put [p1] in the whiteboard list of any bugs that you
> need fixed for jetpack so that they will get picked up in our queries for high
> priority mozmill system bugs.
Actually [jetpack-p1] would be best. Thx.

Comment 14

[Heather Arthur [:harth]]

Verified using Mozrunner 2.4.2 with my regression range finder script. Thanks Atul!