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
Run archiving through CLI sub-processes on OS X #7681
Conversation
The logic is: does `ps` return a list of pids? If yes, then it's supported, if not we still do additional checks because the current unix user might not see other processes (because of permissions?).
f820175
to
08a8d89
Compare
}); | ||
|
||
return $ids; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required for this PR, but do you think it would be useful to have the check done above as a diagnostic? We could display process ids found by the command, maybe it would help users debug or reduce support requests on our end...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be useful, but do we get a lot of support request for this?
Looks good to me, merging. Not sure if this should go into the CHANGELOG? Maybe it's important that CliMulti works on OS X now, what do you think @mattab ? |
Enable archiving through CLI sub-processes on OS X by adding test for whether ps program outputs PIDs in correct format. If ps looks like it works, we assume we can use CLI sub-processes.
*/ | ||
static function getRunningProcesses() | ||
{ | ||
$ids = explode("\n", trim(`ps ex 2>/dev/null | awk '{print $1}'`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will now be executed before self::commandExists('ps')
- i'm wondering how it fails when ps
is not defined on the system. to simulate case where the command does not exist, can you eg. replace ps
by psxxx
and check that there is no error displayed on screen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did and it showed a message, so I redirected the error output to /dev/null. Now I realize though that awk actually gets executed even if the return code of the first command isn't 0, so I'll add the same redirection too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, another soution could be to move up if (self::commandExists('ps') && self::returnsSuccessCode('ps') && self::commandExists('awk')) {
and negate the statement - so getRunningProcesses
would only be called when ps & awk are defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidenote: I don't think self::returnsSuccessCode('ps')
is necessary anymore.
Dear ALL Many Thx |
Added support for CliMulti (
Process::isSupported()
) on OS X whereps
works but there is no/proc
on the filesystem.The logic is: does
ps
return a list of pids? If yes, then it's supported, if not we still do additional checks because the current unix user might not see other processes (because of permissions?).