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

Laravel Valet and PHP Monitor both run services using sudo and break permissions #85

Closed
nicoverbruggen opened this issue Jan 24, 2022 · 2 comments
Assignees
Labels
waiting for valet fix This issue requires a change to Laravel Valet before it can be resolved.

Comments

@nicoverbruggen
Copy link
Owner

nicoverbruggen commented Jan 24, 2022

Here we are again. While working on my workstation laptop today for DIVE, I did my usual maintenance upgrades (running brew upgrade periodically) and I ran into the permission issue again.

Laravel Valet runs the PHP, nginx and dnsmasq services using brew services start $processName, and in PHP Monitor, I emulate the same behaviour.

This is considered a "bad practice" by maintainers of Homebrew (or at least less than ideal), and I'm not sure anything can be done about it. I'm not entirely sure what the constraints were that led to the running the processes as root but I'm assuming running as root is required here... for dnsmasq I believe.

Unfortunately, this causes the system to take ownership of various folders (with the root account) which means that brew cleanup will no longer successfully do its job. And brew cleanup runs automatically.

This causes issues like this one which I stumbled upon after searching some more about running Homebrew services as root. In this example, this was probably caused by Valet, but users using PHP Monitor will experience the same issue.

For example:

Pruned 0 symbolic links and 9 directories from /opt/homebrew
Error: Could not cleanup old kegs! Fix your permissions on:
  /opt/homebrew/Cellar/nginx/1.21.4
  /opt/homebrew/Cellar/php@8.0/8.0.14
  /opt/homebrew/Cellar/php@8.0/8.0.14.reinstall

Side-effects of breaking permissions:

  • Specific PHP versions vanish upon upgrading (sometimes) due to confusion between regular and .reinstall folders (see above for an example)
  • I have to manually run cleanup, like rm -rf /opt/homebrew/Cellar/nginx/1.21.4

A possible workaround is to set HOMEBREW_NO_INSTALL_CLEANUP, but I consider that a sub-optimal solution.

I wonder if there is something I can do that runs the following steps:

  • Figure out what versions of PHP are installed
  • Remove all PHP installations, nginx and dnsmasq via Homebrew
  • Clean up /opt/homebrew/Cellar for these affected packages
  • Reinstall all PHP installations via Homebrew
  • Re-link PHP

Alternatively, a script that fixes permissions prior to upgrading Homebrew stuff could be useful. I could then call brewup.sh (the script) and have it run cleanup + upgrades.

@nicoverbruggen nicoverbruggen added the investigation needed Further investigation is needed at some point label Jan 24, 2022
@nicoverbruggen nicoverbruggen self-assigned this Jan 24, 2022
@nicoverbruggen
Copy link
Owner Author

nicoverbruggen commented Jan 24, 2022

I'll see if I can't get a proof of concept script up and running, and if it works it might be worth adding this to Valet (cli). I've already cloned the repo and have a symlinked local clone running.

I'll also inquire first if this is something the team is interested in.

I dream of:

  • running valet use php@x.x can clean up permissions immediately before installing a specific version if it's missing (avoiding issues with brew cleanup)
  • a separate command used to turn off valet can perhaps also fix permissions upon shutdown (valet stop)
  • a separate command could be invoked (valet cleanup && brew upgrade to flawlessly update your php dependencies)

Alternatively, I'll add this functionality to PHP Monitor only if there is no demand for this in Valet itself.

@nicoverbruggen nicoverbruggen added waiting for valet fix This issue requires a change to Laravel Valet before it can be resolved. and removed investigation needed Further investigation is needed at some point labels Mar 29, 2022
@nicoverbruggen
Copy link
Owner Author

nicoverbruggen commented Mar 29, 2022

I've just opened a PR in the official Valet repo with an enhancement to help combat this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for valet fix This issue requires a change to Laravel Valet before it can be resolved.
Projects
None yet
Development

No branches or pull requests

1 participant