pkrumins / node-tree-kill

kill trees of processes
MIT License
333 stars 37 forks source link

Darwin and SunOS support #6

Closed Maciek416 closed 8 years ago

Maciek416 commented 9 years ago

This PR adds support for killing process trees in Darwin (i.e. Mac OS X) and SunOS (i.e. Joyent SmartOS, etc).

Notes:

billiegoose commented 9 years ago

Awesome... I definitely would like tree-kill to support Darwin and SunOS. I would prefer to do it without adding a bunch of conditional code based on OS-checking though. :-/ I'll look closer to see if this can be simplified without sacrificing support for all the OSes.

billiegoose commented 9 years ago

Here's what I'm thinking. Rather than insert OS-specific conditionals scatter shot throughout the code, let's keep all that we can in the top most function where we check for windows. Instead of the if/else we can use a switch statement on process.platform. We can keep the original buildProcessTree as it is and make a new one for the Darwin/SunOS version of ps.

I really don't want to break things unintentionally, and by keeping the different OSes siloed from each other, people can safely edit the version for the OS they use and be confident they didn't break the module for other OSes they can't test on.Does any of that make sense? I'll make a commit later today building out the structure I'm talking about. Time to start a 'develop' branch...

billiegoose commented 9 years ago

@Maciek416, take a look at 9331318 and see if that makes sense. Any chance you could fill in buildProcessTreeDarwin() and buildProcessTreeSunOS() and test it on those platforms for me?

Maciek416 commented 9 years ago

Hey @wmhilton, I'll try to take a look this week. Thanks for following up !

billiegoose commented 9 years ago

@Maciek416 any update?

UltCombo commented 8 years ago

Can't we use pgrep -P [PPID] in Darwin? From the man page:

DESCRIPTION The pgrep command searches the process table on the running system and prints the process IDs of all processes that match the criteria given on the command line.

The following options are available: [...] -P ppid Restrict matches to processes with a parent process ID in the comma-separated list ppid.

That is, pgrep -P parentPid will return a new-line separated list of child processes. Couldn't it be used to build the process tree?

UltCombo commented 8 years ago

In fact, I believe pgrep -P [PPID] should also work on Linux distros. Is there any reason to use ps instead of pgrep?

billiegoose commented 8 years ago

Is there any reason to use ps instead of pgrep ?

Well, ps is part of the POSIX standard. I don't think pgrep is. Can we be certain that pgrep is installed on every system? It could be a breaking change for some people... I'd rather not risk that.

That bring said, if it's already broken on OS/X, I'm fine with fixing it as long as we don't touch the Linux code.

billiegoose commented 8 years ago

Anybody want to insert some Darwin code in my lovely case statement? https://github.com/pkrumins/node-tree-kill/tree/develop?files=1

UltCombo commented 8 years ago

Well, ps is part of the POSIX standard. I don't think pgrep is. Can we be certain that pgrep is installed on every system? It could be a breaking change for some people... I'd rather not risk that.

Oh, I see. Makes sense.

Anybody want to insert some Darwin code in my lovely case statement?

I believe I should be able to submit a patch in 10 hours or so.

UltCombo commented 8 years ago

10