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

Run archiving through CLI sub-processes on OS X #7681

Merged
merged 1 commit into from Apr 15, 2015
Merged

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Apr 15, 2015

Added support for CliMulti (Process::isSupported()) on OS X where ps 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?).

@mnapoli mnapoli added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review labels Apr 15, 2015
@mnapoli mnapoli added this to the Piwik 2.13.0 milestone Apr 15, 2015
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?).
});

return $ids;
}
Copy link
Member

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...

Copy link
Contributor Author

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?

@diosmosis diosmosis removed the Needs Review PRs that need a code review label Apr 15, 2015
@diosmosis
Copy link
Member

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 ?

diosmosis added a commit that referenced this pull request Apr 15, 2015
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.
@diosmosis diosmosis merged commit d188d22 into master Apr 15, 2015
@diosmosis diosmosis deleted the cli-multi-support branch April 15, 2015 09:34
*/
static function getRunningProcesses()
{
$ids = explode("\n", trim(`ps ex 2>/dev/null | awk '{print $1}'`));
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

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.

mnapoli added a commit that referenced this pull request Apr 15, 2015
@wklueter
Copy link

Dear ALL
I had created a ticket
Re: [piwik] [client xxx.xx] sh: ps: not found, referer: http://www.yyyy.de (#7815) and i get info to use this ticket, but i understand it not, because in announcement of version: 2.13.0,ticket 7815 is closed at piwik-team, but it still exist. Can you pls send me info for understanding?

Many Thx
Willi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants