Skip to content

Remove checking for /proc existence #17846

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

Merged
merged 1 commit into from
Aug 11, 2021

Conversation

avkarenow
Copy link
Contributor

Description:

Currently the '-e' option in ps is not used and looks like /proc is no more needed to detect a running process.
The change can make CliMulti usable on some systems like FreeBSD, OpenBSD or macOS.

Tested on FreeBSD without /proc mounted.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@sgiehl sgiehl added the Needs Review PRs that need a code review label Aug 2, 2021
@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Aug 10, 2021
@sgiehl sgiehl removed the Stale The label used by the Close Stale Issues action label Aug 10, 2021
@sgiehl sgiehl requested a review from diosmosis August 10, 2021 08:47
@diosmosis
Copy link
Member

I'm not sure we can just remove this code, since we added it for a reason and can't test every possible system that might use Matomo. Instead we could provide a DI config option to disable the check, and check for more OSes (that are verified not to need the check), plus a faq that we link to. Thoughts @tsteur ?

@tsteur
Copy link
Member

tsteur commented Aug 10, 2021

I just checked it was added in c076fdb because of #5041

If I understand it right we used to use ps -e and ps -ex which required proc to be there. We are now only using ps x so I reckon we can likely indeed remove it like @avkarenow described as well.

@avkarenow can you confirm ps x works for you and when removing these lines then the archiving still works for you?

@avkarenow
Copy link
Contributor Author

Everything works fine on FreeBSD 12.2. I run it once every day without any problem with archiving on two systems.

@tsteur
Copy link
Member

tsteur commented Aug 11, 2021

I reckon it could be good merging this 👍 If there was any regression afterwards then we may need to change it again but wouldn't expect this to regress

@diosmosis diosmosis added this to the 4.5.0 milestone Aug 11, 2021
@diosmosis diosmosis merged commit 99136cb into matomo-org:4.x-dev Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Development

Successfully merging this pull request may close these issues.

4 participants